-
Notifications
You must be signed in to change notification settings - Fork 337
Remove logic adding unit to metrics name #877
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
base: main
Are you sure you want to change the base?
Remove logic adding unit to metrics name #877
Conversation
…nge tests accordingly Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
bwplotka
left a comment
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.
LGTM, just minor nit for tests.
expfmt/encode_test.go
Outdated
| { | ||
| metric: metric1, | ||
| format: FmtOpenMetrics_1_0_0, | ||
| format: FmtOpenMetrics_0_0_1, |
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.
copy pasta?
| // Without opting in this way, the unit will not be added to the metric name and, | ||
| // on top of that, the unit will not be passed onto the output, even if it | ||
| // were declared in the *dto.MetricFamily struct, i.e. even if in.Unit !=nil. | ||
| func WithUnit() EncoderOption { |
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.
The only potential question is around compatibility for this. For client_golang we should be fine, we don't surface this option to users.
But for potentially downstream common users this might be explictly breaking (compile error). I don't know about any evidences of wide usage, also it could be a damaging option (renaming metrics), so I vote we kill it.
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Following this discussion regarding OM unit support in client_golang, this PR removes the optional unit flag and the related logic that would automatically change the metric name if the UNIT field was populated but the unit not already included in the metric name. This is needed to complete the afore mentioned client_golang PR. cc @bwplotka .