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

fix(graphql): fix graphql.operation.name field #903

Merged
merged 4 commits into from Feb 24, 2022

Conversation

dchambers
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

  • Change graphql.operation.name to send the GraphQL operation-name (a critical field for GraphQL o11y) instead of the GraphQL operation-type.
  • Continue to send the GraphQL operation-type as graphql.operation.type, since this is still useful for segmenting read queries from write mutations, etc.

Checklist

  • Ran npm run test-all-versions for the edited package(s) on the latest commit if applicable.

The operation-name is there to help with o11y:

- https://graphql.org/learn/queries/#operation-name

The operation-type (previously sent in this field) is helpful for grouping too, so send as
`graphql.operation.type`.
@dchambers dchambers requested a review from a team as a code owner February 15, 2022 16:29
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 15, 2022

CLA Signed

The committers are authorized under a signed CLA.

@dchambers
Copy link
Contributor Author

@dyladan, might you be able to help move this PR out of its current state?

@dyladan
Copy link
Member

dyladan commented Feb 16, 2022

@dyladan, might you be able to help move this PR out of its current state?

Just need to wait for some reviews. It's only been 24 hours since it was opened and the contributors/reviewers are in many timezones so it can take a few days sometimes.

@codecov
Copy link

codecov bot commented Feb 16, 2022

Codecov Report

Merging #903 (aea5851) into main (bf39b90) will decrease coverage by 0.67%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #903      +/-   ##
==========================================
- Coverage   95.91%   95.23%   -0.68%     
==========================================
  Files          13       20       +7     
  Lines         856     1301     +445     
  Branches      178      265      +87     
==========================================
+ Hits          821     1239     +418     
- Misses         35       62      +27     
Impacted Files Coverage Δ
...nstrumentation-graphql/src/enums/AttributeNames.ts 100.00% <100.00%> (ø)
...try-instrumentation-graphql/src/instrumentation.ts 91.66% <100.00%> (ø)
...opentelemetry-instrumentation-graphql/src/utils.ts 93.40% <0.00%> (ø)
...opentelemetry-instrumentation-graphql/src/types.ts 100.00% <0.00%> (ø)
...tapackages/auto-instrumentations-node/src/utils.ts 97.87% <0.00%> (ø)
...entelemetry-instrumentation-graphql/src/symbols.ts 100.00% <0.00%> (ø)
.../opentelemetry-instrumentation-graphql/src/enum.ts 100.00% <0.00%> (ø)

@vmarchaud vmarchaud added the enhancement New feature or request label Feb 16, 2022
@dchambers
Copy link
Contributor Author

Hi @obecny 👋. You seem to be the last reviewer on the roster for this PR. Any chance you'd be able to take a look please?

@dchambers
Copy link
Contributor Author

Hi @obecny, it's been another week. Any chance you'd be able to look at this PR please?

@dyladan dyladan removed the request for review from obecny February 24, 2022 20:14
@dyladan
Copy link
Member

dyladan commented Feb 24, 2022

@obecny is no longer a maintainer on the project. sorry for the confusion

@Flarna
Copy link
Member

Flarna commented Feb 24, 2022

he is still part of the approver group and the owner of the graphql instrumentation.

@dchambers
Copy link
Contributor Author

@obecny is no longer a maintainer on the project. sorry for the confusion

I see, thank you @dyladan. Do you know who would now be the right person to ping regarding getting this PR merged in that case?

@dchambers
Copy link
Contributor Author

he is still part of the approver group and the owner of the graphql instrumentation.

This makes sense too @Flarna 👍 , because he's showing as the last person I need an approval from before I can merge.

@dyladan
Copy link
Member

dyladan commented Feb 24, 2022

I am the right person :)

@dyladan dyladan merged commit 5529261 into open-telemetry:main Feb 24, 2022
@dchambers
Copy link
Contributor Author

Thank you guys! 🙏

This was referenced Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants