-
Notifications
You must be signed in to change notification settings - Fork 253
Add metric value type everywhere (as code-generation annotation) #2444
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
Conversation
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.
Pull Request Overview
Adds a new value_type
attribute to every metric and surfaces it in both the rendered registry table and documentation.
- Updated the registry template to include a “Value Type” column.
- Added
value_type
(int or double) to all metric definitions in YAML. - Regenerated all Markdown docs to show the new column.
Reviewed Changes
Copilot reviewed 62 out of 62 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
templates/registry/markdown/metric_table.j2 | Add “Value Type” column to metrics table macro |
model/**/*.yaml | Insert value_type into each metric group |
docs/system/*-metrics.md | Update Markdown tables to include Value Type column |
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 also have similar question as you about when cpu count should allow for millicores and when it shouldn't
E.g. jvm.cpu.count
is captured using https://docs.oracle.com/javase/8/docs/api/java/lang/Runtime.html#availableProcessors-- and so can only be an int
, but there could always be a new Java API in the future that is container aware and allows capturing in millicores (e.g. https://openjdk.org/jeps/8182070)
although I think it's not a breaking change to switch between int and double, since there's no difference in the OTLP output, so future proofing may not be so important?
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.
Verified for process
and system
.
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.
Thank you!
based on the SIG call discussion:
so, I repurposed this PR to use E.g. - id: metric.system.process.count
type: metric
metric_name: system.process.count
annotations:
code_generation:
metric_value_type: int |
See open-telemetry/weaver#817 (comment) - all is not lost. Since |
Fixes #591
This PR adds
value_type
(int or double) to all metric definitions and updates template to render it in the metrics table.All MD changes are auto-generated, all substantial changes are in yaml