Skip to content
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

[SEMANTIC CONVENTIONS] Upgrade to 1.21.0 #2248

Merged
merged 3 commits into from
Jul 28, 2023

Conversation

marcalff
Copy link
Member

@marcalff marcalff commented Jul 23, 2023

Fixes #2138

Changes

Upgrade:

  • Semantic conventions to 1.21.0
  • Build tools to 0.19.0

Adjust code per the following semantic convention changes:

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@marcalff marcalff requested a review from a team July 23, 2023 18:39
@codecov
Copy link

codecov bot commented Jul 23, 2023

Codecov Report

Merging #2248 (ff2e9da) into main (0c5f90d) will increase coverage by 0.03%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2248      +/-   ##
==========================================
+ Coverage   87.30%   87.32%   +0.03%     
==========================================
  Files         169      169              
  Lines        4897     4897              
==========================================
+ Hits         4275     4276       +1     
+ Misses        622      621       -1     

see 1 file with indirect coverage changes

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

LGTM Thanks for the upgrade. Not related to this PR, but the trace and resource header files are quite big now. Also, someone interested in only DB-specific instrumentation will have HTTP-related static variables getting created and never used.
You know this part better, but is it feasible to generate domain-specific header files, say trace/semantic_convention_db.h, trace/semantic_convention_http.h etc? We can still have the existing trace/semantic_convention.h which includes all these domain-specific header files. If feasible, we can have an issue created for someone to pick.

@marcalff
Copy link
Member Author

LGTM Thanks for the upgrade. Not related to this PR, but the trace and resource header files are quite big now. Also, someone interested in only DB-specific instrumentation will have HTTP-related static variables getting created and never used. You know this part better, but is it feasible to generate domain-specific header files, say trace/semantic_convention_db.h, trace/semantic_convention_http.h etc? We can still have the existing trace/semantic_convention.h which includes all these domain-specific header files. If feasible, we can have an issue created for someone to pick.

This can be investigated.

The risk I see is that the build scripts and the header file layout produced will be more coupled to the internal code organization in the semantic-convention/model directory:

  • If a new yaml file is added, we may not notice it, forgetting to generate semconv for it
  • If a yaml file is split, moved, merged, renamed, etc, the header files generated for it will be affected, breaking C++ users who include this header file alone.

I would rather wait for a couple of semantic-conventions releases first, to make sure the code layout itself is stable, before we can consider this.

@ThomsonTan ThomsonTan merged commit 8f97cab into open-telemetry:main Jul 28, 2023
@marcalff marcalff deleted the fix_sem_conv_2138 branch September 5, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SEMANTIC CONVENTIONS] Upgrade to semconv version 1.21.0
3 participants