feat(instrumentation-graphql): Add option to put all resolve spans under the same parent span

[nitrofski]

Which problem is this PR solving?

At the moment, GraphQL "resolve" spans (spans generated for each resolver/sub-resolver in a GraphQL request) are generated in a tree structure that reflects the GraphQL resolution tree. This sounds great in theory, but in practice it generates this kind of traces, where child spans start when their parent spans end.

 v root span                             | ----------------------------------------
 |  v resolve thing                      | ----------
 |  |  v resolve thing.field1            |           ----------
 |  |  \-    resolve thing.field1.nested |                     --------------------
 \- \-   resolve thing.field2            |           --------------------

Although it may look fine at a glance, it has issues:

• Collapsing a span with child spans in a trace viewer (maybe some of them handle this better?) results in some amount of time seemingly unaccounted for in the root span. E.g.:

   v root span                             | ----------------------------------------
   |  v resolve thing                      | ----------
   |  |  > resolve thing.field1            |           ----------
   \- \-   resolve thing.field2            |           --------------------
  

   v root span                             | ----------------------------------------
   \- > resolve thing                      | ----------
  

• Critical path in viewers like Jaeger does not generate properly

Short description of the changes

The solution is to never have a span end before all its child spans have ended. I can imagine two ways to achieve this:

1. Add an extra layer of spans for the tree structure, and put the "resolve" spans under their tree structure node.

    v root span                    | ----------------------------------------
    |  v thing                     | ----------------------------------------
    |  |    resolve                | ----------
    |  |  v thing.field1           |           ------------------------------
    |  |  |    resolve             |           ----------
    |  |  |  v thing.field1.nested |                     --------------------
    |  |  \- \-   resolve          |                     --------------------
    |  |  v thing.field2           |           --------------------
    \- \- \-   resolve             |           --------------------
   

2. Put all "resolve" spans under the same parent.

   v root span                      | ----------------------------------------
   |    resolve thing               | ----------
   |    resolve thing.field1        |           ----------
   |    resolve thing.field2        |           --------------------
   \-   resolve thing.field1.nested |                     --------------------
   

This PR implements option 2 for simplicity. This is opt-in behind a flatResolveSpans configuration.

I am open to work toward option 1 if preferred.

[nitrofski]

@david-luna This is the other change I would like to contribute. I'm open to discussion since this isn't as black-and-white as the bug fix we just merged.

[codecov]

Codecov Report

❌ Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.67%. Comparing base (b11e61e) to head (223fb7d).
⚠️ Report is 77 commits behind head on main.

Files with missing lines	Patch %	Lines
packages/instrumentation-graphql/src/utils.ts	94.11%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3085   +/-   ##
=======================================
  Coverage   92.67%   92.67%           
=======================================
  Files         237      237           
  Lines       11247    11251    +4     
  Branches     2334     2336    +2     
=======================================
+ Hits        10423    10427    +4     
  Misses        824      824           

Flag	Coverage Δ
instrumentation-graphql	94.15% <94.11%> (+0.05%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...ages/instrumentation-graphql/src/internal-types.ts	100.00% <ø> (ø)
packages/instrumentation-graphql/src/utils.ts	93.57% <94.11%> (+0.12%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nitrofski]

❌ Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.

Hmm. This is bothersome. The line missing coverage was subject to a simple refactor (field.span → span).

What's the correct resolution here? Revert refactor, add a test unrelated to the rest of the PR, or ignore?

[pichlermarc]

❌ Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.

Hmm. This is bothersome. The line missing coverage was subject to a simple refactor (field.span → span).

What's the correct resolution here? Revert refactor, add a test unrelated to the rest of the PR, or ignore?

Looks like this is just some confusing output from Codecov - it's passing our target of 80%, you you're good :)

[david-luna]

Sorry for taking too long to review. LGTM

cc: @obecny

[david-luna]

@nitrofski

All good but if we're adding a new config option we should update the README.md file.

[nitrofski]

@david-luna

Good call, thank you! I added the config and description to the existing table in the README.

[otelbot]

Thank you for your contribution @nitrofski! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey.