opentelemetry-js-contrib
opentelemetry-js-contrib copied to clipboard
refactor(instr-graphql): use exported strings for attributes
Which problem is this PR solving?
- Updates #2025
Short description of the changes
- Update @opentelemetry/semantic-conventions to ^1.22
- Replace
SemanticAttributes.*with exported stringsSEMATTRS_*in test files
Note: I did not add the "semantic conventions" section in the readme file since this instrumentation is using it only in tests but not for span attribs
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 90.12%. Comparing base (
dfb2dff) to head (00af2fb). Report is 112 commits behind head on main.
:exclamation: Current head 00af2fb differs from pull request most recent head cd0ebd6. Consider uploading reports for the commit cd0ebd6 to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@
## main #2156 +/- ##
==========================================
- Coverage 90.97% 90.12% -0.86%
==========================================
Files 146 142 -4
Lines 7492 6995 -497
Branches 1502 1475 -27
==========================================
- Hits 6816 6304 -512
- Misses 676 691 +15
LGTM
Should we add some note in the README about this package not implementing any semantic convention at the moment?
I thought the absence of the section in the README was enough but I based on your comment I guess it would be better that all packages have a section about semconv
LGTM Should we add some note in the README about this package not implementing any semantic convention at the moment?
I thought the absence of the section in the README was enough but I based on your comment I guess it would be better that all packages have a section about semconv
That's my personal opinion 😄 Feel free to add one or resolve if you prefer the READMEs not to state it in this case
LGTM Should we add some note in the README about this package not implementing any semantic convention at the moment?
I thought the absence of the section in the README was enough but I based on your comment I guess it would be better that all packages have a section about semconv
That's my personal opinion 😄 Feel free to add one or resolve if you prefer the READMEs not to state it in this case
Oh interesting! It looks like graphql semantic conventions do exist, and were recently added to the registry in semantic-conventions repo, though do not appear to exist in the schema itself yet unless I am missing it.
Also seeing there was an issue opened (closed for stale) referencing unexpected attributes. So this does seem like something that we explicitly should state that we are not using semantic conventions here for graphql attributes... and a new issue should perhaps be opened to review that. I'm not clear on whether this would be included in the new generated semantic conventions if it doesn't exist in the schema yaml files, but it is something that appears to have expected conventions at this point.
@JamieDanielson I've added your suggestion. Thanks :)