Skip to content

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

Merged
merged 11 commits into from
Jul 5, 2025

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Jun 27, 2025

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

@github-actions github-actions bot added the enhancement New feature or request label Jun 27, 2025
@lmolkova lmolkova requested a review from Copilot June 27, 2025 21:15
Copy link

@Copilot Copilot AI left a 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

Copy link
Member

@trask trask left a 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?

@github-project-automation github-project-automation bot moved this from Untriaged to Needs More Approval in Semantic Conventions Triage Jun 28, 2025
Copy link
Contributor

@braydonk braydonk left a 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.

Copy link
Member

@joaopgrassi joaopgrassi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@trask trask modified the milestone: v1.35.0 Jun 30, 2025
@lmolkova lmolkova moved this from Needs More Approval to Blocked in Semantic Conventions Triage Jul 1, 2025
@lmolkova lmolkova changed the title Update weaver and add metric value type everywhere Add metric value type everywhere (as code-generation annotation) Jul 2, 2025
@lmolkova
Copy link
Contributor Author

lmolkova commented Jul 2, 2025

based on the SIG call discussion:

so, I repurposed this PR to use annotations.code_generation.metric_value_type

E.g.

  - id: metric.system.process.count
    type: metric
    metric_name: system.process.count
    annotations:
      code_generation:
        metric_value_type: int

@github-project-automation github-project-automation bot moved this from Blocked to Needs More Approval in Semantic Conventions Triage Jul 3, 2025
@lmolkova lmolkova added this pull request to the merge queue Jul 5, 2025
Merged via the queue into open-telemetry:main with commit 3b64cb3 Jul 5, 2025
15 checks passed
@lmolkova lmolkova deleted the weaver-0.15.3 branch July 5, 2025 17:20
@jerbly
Copy link
Contributor

jerbly commented Jul 6, 2025

See open-telemetry/weaver#817 (comment) - all is not lost. Since annotations are part of the resolved registry they are also passed through to custom rego policies you can provide to live-check. So, it is still possible to check telemetry aligns with model.

spurplewang pushed a commit to spurplewang/semantic-conventions that referenced this pull request Jul 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Metrics semconv should include value type of instrument in yaml
10 participants