-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add missing unit in metadata.yaml #23449
Conversation
Metric unit is required, see: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/cmd/mdatagen/metadata-schema.yaml#L72-L73. This PR adds the unit to all metrics where this is missing. I have inferred the unit from the metric name, and would appreciate that someone more familiar with the receivers in question review the chosen units.
receiver/redisreceiver/metadata.yaml
Outdated
@@ -200,14 +201,14 @@ metrics: | |||
redis.memory.fragmentation_ratio: | |||
enabled: true | |||
description: Ratio between used_memory_rss and used_memory | |||
unit: "" | |||
unit: "{ratio}" |
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.
Not sure I agree with this one. "ratio" describes the value in a way, but it's not a unit of measurement.
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.
Agreed, seeing that ratios are unitless I'm not sure what to put here 🤔 ? I've removed it for now.
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 believe "1" is the default unit, but it's ok to just omit it.
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'm working on validation of the metadata.yaml
(issue), and one of the checks that I currently have makes sure that the unit is not empty because it is a required field. This is an edge case, as the metric is unitless.
I've made a commit to put the unit to 1 for redis.memory.fragmentation_ratio
instead of leaving it empty -- let me know if this is OK ?
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 think 1 is the correct unit for ratio
This PR adds the bytes unit to metrics where this is missing. unit is a required field, see https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/cmd/mdatagen/metadata-schema.yaml#L72-L73. The rest of the units (non-breaking) are being addressed in a separate PR: open-telemetry#23449.
Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com>
Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com>
Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com>
Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com>
@mackjmr, thanks for this PR. In order to pass tests, you'll have to also update the "golden" files for the affected scrapers. The easiest way to do this is to regenerate the files. You can do this by temporarily inserting a call to |
Thanks for the explanation @djaglowski, I've updated the golden files 👍 |
@mackjmr, can you please split the PR into one per component and add changelog entries? |
@dmitryax split into 1 PR per component with changelog:
Closing this PR. |
Description:
Metric unit is required, see: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/cmd/mdatagen/metadata-schema.yaml#L72-L73.
This PR adds the unit to all metrics where this is missing.
I have inferred the unit from the metric name, and would appreciate that someone more familiar with the receivers in question review the chosen units.