- 
                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