-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[mdatagen] Add semconv reference for metrics in metadata schema #13920
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?
Conversation
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (94.05%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #13920 +/- ##
=======================================
Coverage 92.27% 92.27%
=======================================
Files 657 657
Lines 41111 41207 +96
=======================================
+ Hits 37936 38025 +89
- Misses 2173 2177 +4
- Partials 1002 1005 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ea7e141 to
51d5532
Compare
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 like this change a lot. would be great to get this in
I'll get back into finalising this and opening for review soonish! |
c46a42c to
c156b07
Compare
Signed-off-by: ChrsMark <[email protected]>
c156b07 to
fe3a27c
Compare
|
@codeboten @mx-psi I've opened this for review now. Feel free to take a look when you get the chance, thank's! |
cmd/mdatagen/internal/metadata.go
Outdated
| } | ||
|
|
||
| type SemanticConvention struct { | ||
| SemanticConventionRef string `mapstructure:"semconv_ref"` |
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 the semconv_ prefix here is redundant since this is already in the semantic_convention section.
| GeneratedPackageName: "metadata", | ||
| Type: "sample", | ||
| SemConvVersion: "1.9.0", | ||
| SemConvVersion: "1.37.0", |
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 am not sure what this field is, but it seems like we should either drop this or make sure that sem_conv_version and the version linked in semconv_ref are aligned somehow
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.
This is sth that already exists and links the whole component to a specific version of SemConv. It's not really used apart from a few places inside the templates that mdatagen populates i.e. logs.go.tmpl and metrics.go.tmpl.
The PR comes with a pattern checking that also verifies if the provided reference's version matches with this generic one: https://github.com/open-telemetry/opentelemetry-collector/pull/13920/files#diff-b33d4fa1a4595583d310c0a381ce3849aefb42419451584fd0e1c43bd16f5263R124-R128. (also covered by this test)
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.
Okay, sorry, another thing I missed. I think that makes sense, though maybe we can remove it entirely if it's not widely used?
I am worried that we won't be able to support transition scenarios where we want to target two semconv versions
Signed-off-by: ChrsMark <[email protected]>
This patch adds the missing stability definition for the `elasticsearch.node.open_files` metric. Spotted at open-telemetry/opentelemetry-collector#13920. Signed-off-by: ChrsMark <[email protected]>
Description
This PR adds support in mdatagen for defining SemConv reference for metrics.
Enhancing your metrics/signals in
metadata.yamllike bellow:Will provide the following generated docs:
system.cpu.time
Monotonic cumulative sum int metric enabled by default.
Validation is added so as to ensure that provided SemConv urls match the top_level semconv version as well as the metric's name.
Link to tracking issue
Related to #13910 and #13297.
Testing
Tuned
Documentation
Tuned