Allow flattening of a struct through derive(EncodeLabelSet) at any position #135

drbrain · 2023-04-15T01:31:47Z

Currently when deriving EncodeLabelSet and flattening a struct the flattened struct must appear last, and there must be only one flattened struct, from the test:

#[test]
fn flatten() {
    #[derive(EncodeLabelSet, Hash, Clone, Eq, PartialEq, Debug)]
    struct CommonLabels {
        a: u64,
        b: u64,
    }
    #[derive(EncodeLabelSet, Hash, Clone, Eq, PartialEq, Debug)]
    struct Labels {
        unique: u64,
        #[prometheus(flatten)]
        common: CommonLabels,
    }

    // …

If you place common before unique in struct Labels like this:

#[derive(EncodeLabelSet, Hash, Clone, Eq, PartialEq, Debug)]
struct Labels {
    #[prometheus(flatten)]
    common: CommonLabels,
    unique: u64,
}

Compilation fails:

error[E0382]: borrow of moved value: `encoder`
   --> derive-encode/tests/lib.rs:147:14
    |
147 |     #[derive(EncodeLabelSet, Hash, Clone, Eq, PartialEq, Debug)]
    |              ^^^^^^^^^^^^^^
    |              |
    |              value borrowed here after move
    |              move occurs because `encoder` has type `LabelSetEncoder<'_>`, which does not implement the `Copy` trait
    |

This occurs because ownership of encoder is given to the flattened struct

I think this would be a breaking change to fix, but fixing it would allow fields to appear in any order, or allow flattening of multiple structs without nesting them all one inside the other in tail position.

The text was updated successfully, but these errors were encountered:

mxinden · 2023-05-09T04:49:10Z

Thanks for debugging and the detailed issue.

I think this would be a breaking change to fix, but fixing it would allow fields to appear in any order, or allow flattening of multiple structs without nesting them all one inside the other in tail position.

Worth exploring passing &mut LabelSetEncoder. Not sure why I chose to require ownership.

Another alternative is to provide a better error message in the derive macro when the flattened property is not the last.

olix0r · 2023-12-07T02:40:17Z

Worth exploring passing &mut LabelSetEncoder.

My initial impression is that this would be a welcome API change. If we could impl<A: EncodeLabelSet, B: EncodeLabelSet> EncodeLabelSet for (A, B), that would generally make label composition more flexible.

cratelyn · 2024-10-15T21:29:05Z

@mxinden I see that you added the help wanted label.

Worth exploring passing &mut LabelSetEncoder. Not sure why I chose to require ownership.

Another alternative is to provide a better error message in the derive macro when the flattened property is not the last.

I would be happy to work on this! I'd like to follow the former path, leaving us with these signatures:

pub trait EncodeLabelSet {
    /// Encode oneself into the given encoder.
    fn encode(&self, encoder: &mut LabelSetEncoder) -> Result<(), std::fmt::Error>;
}

pub trait EncodeLabel {
    /// Encode oneself into the given encoder.
    fn encode(&self, encoder: &mut LabelEncoder) -> Result<(), std::fmt::Error>;
}

This seems especially reasonable because today EncodeLabelKey and EncodeLabelValue both accept a &mut LabelKeyEncoder and &mut LabelValueEncoder, respectively. Similarly, EncodeGaugeValue accepts a &mut GaugeValueEncoder, and EncodeCounterValue a &mut CounterValueEncoder.

While we are doing this, we should also update EncodeExemplarValue to accept a &mut ExemplarValueEncoder, to follow the pattern as well.

cratelyn · 2024-10-15T21:36:28Z

To help reduce the churn of breaking changes, presuming the above change is welcome, would it also make sense to update the signature of the Collector and EncodeMetric traits as well?

client_rust/src/collector.rs

Lines 38 to 41 in ad05f0f

    
           pub trait Collector: std::fmt::Debug + Send + Sync + 'static { 
        
               /// Once the [`Collector`] is registered, this method is called on each scrape. 
        
               fn encode(&self, encoder: DescriptorEncoder) -> Result<(), std::fmt::Error>; 
        
           }

client_rust/src/encoding.rs

Lines 43 to 46 in ad05f0f

    
           pub trait EncodeMetric { 
        
               /// Encode the given instance in the OpenMetrics text encoding. 
        
               // TODO: Lifetimes on MetricEncoder needed? 
        
               fn encode(&self, encoder: MetricEncoder) -> Result<(), std::fmt::Error>;

This would leave us with a consistent interface across the board, where encode trait methods for various entities only require a mutable reference to their respective encoders.

mxinden · 2024-10-27T19:06:16Z

This would leave us with a consistent interface across the board, where encode trait methods for various entities only require a mutable reference to their respective encoders.

That makes sense. Thank you.

While a breaking change, I doubt it would break many people. Thus I think it is worth it. Of course in a semver compliant next version.

@cratelyn thank you for the help!

cratelyn · 2024-10-29T17:13:15Z

@mxinden of course! i'll open up a PR soon, likely some time early next week. i will try and open up a fix to #231 in parallel, ~~so that hopefully we can group together breaking changes into a single release.~~

likewise, thanks for being open to a change! the strongly-typed label sets in this library is a really lovely ergonomic improvement from the predecessor library.

see prometheus#135. this adjusts the parameter of `encode()` so that it only mutably borrows the encoder. Signed-off-by: katelyn martin <[email protected]>