-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Override derived label value case #136
base: master
Are you sure you want to change the base?
Override derived label value case #136
Conversation
This applies to individual entries in the derived implementation Signed-off-by: Eric Hodel <[email protected]>
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 <[email protected]>
Signed-off-by: Eric Hodel <[email protected]>
6af464b
to
ecaf155
Compare
There was a problem hiding this comment.
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!
/// For variants you can use `#[prometheus(lower)]` or `#[prometheus(upper)]` to set the case for | ||
/// only that variant. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏
"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}\"" | ||
), | ||
)) |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
By default deriving label values for an enum:
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.
Would have labels for values
EnumLabel::One
andEnumLabel::Two
would be"one"
and"two"
respectively.The case can be overridden for a single variant as well:
Would have labels for values
EnumLabel::One
andEnumLabel::Two
would be"one"
and"Two"
respectively.