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(instr-undici): respect requireParent flag when INVALID_SPAN_CONTEXT is used #2273

Merged

Conversation

reberhardt7
Copy link
Contributor

Which problem is this PR solving?

This PR mirrors this fix in the http instrumentation: open-telemetry/opentelemetry-js#4788

The http instrumentation (and many other instrumentation libraries) create spans with INVALID_SPAN_CONTEXT to create NonRecordingSpans when we enable requireParent and no parent is present: https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/http.ts#L769

That works fine; those spans are not exported. However, when we create child spans, those end up becoming real spans that are exported. E.g. if we have an http server, when a request comes in, we create a span with INVALID_SPAN_CONTEXT, and then if the http request handler makes an outbound request, the instrumentation sees that it has a parent span (even though it is invalid) and creates a real child span, initiating a new trace.

The opentelemetry sdk creates new traces for descendants of INVALID_SPAN_CONTEXT: https://github.com/open-telemetry/opentelemetry-js/blob/ecc88a38d85d5d6adf847306d96e7be77b7df8d6/packages/opentelemetry-sdk-trace-base/src/Tracer.ts#L91

Short description of the changes

This checks whether the parent span is recording, and does not create a real child span if the parent isn't recording.

@reberhardt7 reberhardt7 requested a review from a team as a code owner June 12, 2024 23:30
Copy link

linux-foundation-easycla bot commented Jun 12, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: david-luna / name: David Luna (0702bcf)
  • ✅ login: reberhardt7 / name: Ryan Eberhardt (0506cea)

@david-luna
Copy link
Contributor

Hi @reberhardt7

1st of all thanks for contributing to OpenTelemetry :)

Could you please sign the CLA so we can proceed on moving your PR forward?

Thanks!

Copy link

codecov bot commented Jun 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.29%. Comparing base (dfb2dff) to head (0702bcf).
Report is 165 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2273      +/-   ##
==========================================
- Coverage   90.97%   90.29%   -0.68%     
==========================================
  Files         146      147       +1     
  Lines        7492     7246     -246     
  Branches     1502     1504       +2     
==========================================
- Hits         6816     6543     -273     
- Misses        676      703      +27     
Files Coverage Δ
plugins/node/instrumentation-undici/src/undici.ts 96.27% <100.00%> (ø)

... and 53 files with indirect coverage changes

@reberhardt7
Copy link
Contributor Author

@david-luna I've signed the CLA now. Thank you :)

@reberhardt7 reberhardt7 force-pushed the rebs/fix-undici-requireparent branch from cf10ea0 to 0506cea Compare June 14, 2024 01:33
@reberhardt7
Copy link
Contributor Author

@david-luna thanks for the review! Would you be the right person to take a look at this analogous instrumentation-http PR, or would that be someone else?

@david-luna david-luna merged commit b08f01f into open-telemetry:main Jun 19, 2024
19 checks passed
@dyladan dyladan mentioned this pull request Jun 14, 2024
@david-luna
Copy link
Contributor

@reberhardt7 I can give my review and notice the members of the SIG to get another approval

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.

None yet

3 participants