-
Notifications
You must be signed in to change notification settings - Fork 513
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(graphql): add ignoreTrivialResolveSpans config option #1256
feat(graphql): add ignoreTrivialResolveSpans config option #1256
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx for this change, few typos / nitpicks
plugins/node/opentelemetry-instrumentation-graphql/src/types.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-graphql/src/types.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
…elemetry-js-contrib into no-default-resolver-spans
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Thank you @obecny for the review. Very much appreciate it. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1256 +/- ##
==========================================
- Coverage 96.08% 95.47% -0.61%
==========================================
Files 14 21 +7
Lines 893 1369 +476
Branches 191 286 +95
==========================================
+ Hits 858 1307 +449
- Misses 35 62 +27
|
Which problem is this PR solving?
graphql instrumentation is creating a "resolve" span for each field in the query result.
this can be a HUGE amount of spans, out of which most are definitely not interesting.
Objects with many fields or lists can grow to hundreds or thousands spans per query!
Users usually don't write a resolver for each and every field. they have few resolvers to fetch data from database or http endpoint, and then they map the returned object fields using the default field resolver which just look for a property with the same name on the object.
https://graphql.org/learn/execution/#trivial-resolvers
Creating spans for these trivial property resolvers is not very interesting and increases noise and costs.
This PR adds a config option so that for fields that use the custom resolver which just resolve to a property, span creation is skipped.
Example:
This query will create 7 "resolve" spans. One for resolving the users list, and then 3 for resolving each object name and 3 for resolving the age.
When turning on the new config option, only the first one is recorded. The span describing resolving of the query field "name" from the property "name" on "{ name: 'John', age: 40 }" to 'John' is not instrumented.
Short description of the changes