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

feat(instrumentation-graphql): add option to ignore resolver spans #1858

Merged
merged 9 commits into from
Jan 15, 2024

Conversation

seemk
Copy link
Contributor

@seemk seemk commented Dec 11, 2023

Which problem is this PR solving?

#1739

Short description of the changes

Add the option to ignore resolver spans.

Creating a span for each resolve can cause way too many spans to be created (e.g. 100k elements in the response equals at least 100k spans, most of them will be dropped). This causes huge latency increases - my test query with an in memory resolver went from 1.2s to 8-9s. With ignoreResolveSpans: true the response time went back to ~1.2s.

For reference, the Java implementation of GraphQL instrumentation ignores resolvers by default and there isn't even a possibility to enable creating spans from resolvers.

I think ignoring resolvers should be the default behaviour in the Node.js instrumentation as well, but for backwards compatibility I set the default value to false in this PR. Up for discussion whether to ignore resolvers by default or not.

@seemk seemk requested a review from a team December 11, 2023 14:28
Copy link

codecov bot commented Dec 11, 2023

Codecov Report

Merging #1858 (73e8e6e) into main (c54e9b6) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head 73e8e6e differs from pull request most recent head 72c88ab. Consider uploading reports for the commit 72c88ab to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1858      +/-   ##
==========================================
- Coverage   91.50%   91.47%   -0.03%     
==========================================
  Files         145      145              
  Lines        7425     7414      -11     
  Branches     1484     1486       +2     
==========================================
- Hits         6794     6782      -12     
- Misses        631      632       +1     
Files Coverage Δ
...try-instrumentation-graphql/src/instrumentation.ts 92.81% <100.00%> (+0.63%) ⬆️

... and 4 files with indirect coverage changes

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

I think this is a good change, thank you for taking care of this. 👍

Given that there's cases where performance is impacted this much, I think it would be reasonable to make ignoreResolveSpans: true the default in a future release.

@seemk
Copy link
Contributor Author

seemk commented Jan 15, 2024

@pichlermarc Are there any additional approves this PR needs?

@pichlermarc
Copy link
Member

@seemk, no other approvals needed. I'll merge this in. Thank you for your contribution!

@pichlermarc pichlermarc merged commit c365375 into open-telemetry:main Jan 15, 2024
15 checks passed
@dyladan dyladan mentioned this pull request Jan 13, 2024
@satazor
Copy link
Contributor

satazor commented Jan 24, 2024

Just noticed that this PR missed the documentation update on the README. I will make a PR to add it.

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.

4 participants