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.
github-actions
bot
added
receiver/aerospike
receiver/postgresql
receiver/redis
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.
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.
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.
👍 I believe "1" is the default unit, but it's ok to just omit it.
There was a problem hiding this comment.
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.
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 <[email protected]>
Co-authored-by: Daniel Jaglowski <[email protected]>
Co-authored-by: Daniel Jaglowski <[email protected]>
Co-authored-by: Daniel Jaglowski <[email protected]>
|
@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.