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

refactor(instr-graphql): use exported strings for attributes #2156

Merged
merged 6 commits into from
May 14, 2024

Conversation

david-luna
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

  • Update @opentelemetry/semantic-conventions to ^1.22
  • Replace SemanticAttributes.* with exported strings SEMATTRS_* 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

Copy link

codecov bot commented Apr 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.12%. Comparing base (dfb2dff) to head (00af2fb).
Report is 112 commits behind head on main.

❗ 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     

see 41 files with indirect coverage changes

Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

LGTM

Should we add some note in the README about this package not implementing any semantic convention at the moment?

@david-luna
Copy link
Contributor Author

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

@blumamir
Copy link
Member

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

@JamieDanielson
Copy link
Member

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.

Copy link
Member

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

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

On #2182 I tried out this wording; if we like it we can use it here.

## Semantic Conventions

This package does not currently generate any attributes from semantic conventions.

Also as far as the other attributes that do seem like they should be pulled from semantic conventions and possibly updated as well... let's deal with that separately. This is strictly documenting the current state.

@david-luna
Copy link
Contributor Author

@JamieDanielson I've added your suggestion. Thanks :)

@trentm trentm enabled auto-merge (squash) May 14, 2024 18:31
@trentm trentm disabled auto-merge May 14, 2024 18:31
@trentm trentm enabled auto-merge (squash) May 14, 2024 18:32
@trentm trentm merged commit b98e431 into open-telemetry:main May 14, 2024
8 checks passed
@david-luna david-luna deleted the dluna/graphql-semconv-strings branch June 21, 2024 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants