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(graphql): add ignoreTrivialResolveSpans config option #1256

Merged
merged 18 commits into from
Nov 6, 2022

Conversation

blumamir
Copy link
Member

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:

const schema = buildSchema(`
    type Query {
        users: [User!]!
    }

    type User {
        name: String
        age: Int
    }
`);

const rootValue = {
    // mock an async call to external data source
    users: Promise.resolve([{ name: 'John', age: 40 }, { name: 'Jane', age: 25 }, { name: 'Bob', age: 31 }]),
};

const source = '{ users { name, age} }';

await graphql({ schema, source, rootValue });

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

  • Added config option to instrumentation config
  • Documented it and other options in README
  • updated the resolver function patch to be aware if it's patching the defaultFieldResolver, and if so, to check if it's about to resolve a property (in contrast to function). If so - it will call the original function without any instrumentation.

@blumamir blumamir requested a review from a team October 27, 2022 18:53
@github-actions github-actions bot requested a review from obecny October 27, 2022 18:54
Copy link
Member

@obecny obecny left a 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

blumamir and others added 6 commits October 27, 2022 22:13
Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm

@blumamir
Copy link
Member Author

Thank you @obecny for the review. Very much appreciate it.

@codecov
Copy link

codecov bot commented Oct 27, 2022

Codecov Report

Merging #1256 (2ecb2fe) into main (c9923e3) will decrease coverage by 0.60%.
The diff coverage is 100.00%.

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     
Impacted Files Coverage Δ
...try-instrumentation-graphql/src/instrumentation.ts 92.85% <100.00%> (ø)
...opentelemetry-instrumentation-graphql/src/utils.ts 93.17% <100.00%> (ø)
...etry-instrumentation-graphql/src/internal-types.ts 100.00% <0.00%> (ø)
...entelemetry-instrumentation-graphql/src/symbols.ts 100.00% <0.00%> (ø)
...nstrumentation-graphql/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)
.../opentelemetry-instrumentation-graphql/src/enum.ts 100.00% <0.00%> (ø)
...tapackages/auto-instrumentations-node/src/utils.ts 98.18% <0.00%> (ø)

@blumamir blumamir merged commit aff84bb into open-telemetry:main Nov 6, 2022
@dyladan dyladan mentioned this pull request Nov 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants