Skip to content

Override derived label value case - #136

Open
drbrain wants to merge 3 commits into
prometheus:masterfrom
drbrain:override-derived-label-value-case
Open

Override derived label value case#136
drbrain wants to merge 3 commits into
prometheus:masterfrom
drbrain:override-derived-label-value-case

Conversation

@drbrain

Copy link
Copy Markdown

By default deriving label values for an enum:

#[derive(EncodeLabelValue,Hash,Clone,Eq,PartialEq,Debug)]enumEnumLabel{One,}

Results in a label that matches the variant identifier's, here "One".

To allow compatibility with metrics emitted from other processes, or compatibility when switching crates, this PR allows overriding the default case for EncodeLabelValue for an entire enum or for individual enum entries.

#[derive(EncodeLabelValue,Hash,Clone,Eq,PartialEq,Debug)]#[prometheus(value_case = "lower")]enumEnumLabel{One,Two,}

Would have labels for values EnumLabel::One and EnumLabel::Two would be "one" and "two" respectively.

The case can be overridden for a single variant as well:

#[derive(EncodeLabelValue,Hash,Clone,Eq,PartialEq,Debug)]enumEnumLabel{#[prometheus(lower)]One,Two,}

Would have labels for values EnumLabel::One and EnumLabel::Two would be "one" and "Two" respectively.

This applies to individual entries in the derived implementation
Signed-off-by: Eric Hodel <drbrain@segment7.net>
When deriving label values for an enum the default case of the label matches
the identifier case. This forced derived label values to use rust's
case rules which may not match up with metrics exported from other
programs, or metrics from other crates that export prometheus metrics.
This change adds the ability to set the case of a derived value label to
all-lowercase or all-uppercase for the entire struct in addition to for
an individual label from the prior commit.
Signed-off-by: Eric Hodel <drbrain@segment7.net>
Signed-off-by: Eric Hodel <drbrain@segment7.net>
@drbrain
drbrainforce-pushed the override-derived-label-value-case branch from 6af464b to ecaf155CompareApril 21, 2023 22:24

@mxindenmxinden left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry for the delay here. Well done. Thanks @drbrain!

Comment on lines +109 to +110
/// For variants you can use `#[prometheus(lower)]` or `#[prometheus(upper)]` to set the case for
/// only that variant.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a concrete use-case where one would upper or lower case a specific variant only?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not

it might be better to remove this and wait for a motivated individual to plumb serde in to label generation which could provide all manner of derive customization

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want me to remove per-variant case changing?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please remove. Thank you.

let body = match ast.clone().data {
syn::Data::Struct(_) => {
panic!("Can not derive EncodeLabel for struct.")
panic!("Can not derive EncodeLabelValue for struct.")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏

Comment on lines +229 to +237
"lower" => config.value_case = ValueCase::Lower,
"upper" => config.value_case = ValueCase::Upper,
invalid => {
return Err(syn::Error::new(
case.span(),
format!(
"value case may only be \"lower\" or \"upper\", not \"{invalid}\""
),
))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To de-duplicate this code with the one above in line 146, how about implementing FromStr for ValueCase?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will take me some time, but I think I can do this

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@drbrain@mxinden