fix: OpenTelemetry trace context propagation#12962
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
a268ec2 to
3ec806c
Compare
|
Hi, Both tracers instrument and uninstrument requests/urllib3 per tracer instance even though these instrumentors monkeypatch globally; ending one trace can disable propagation for other in-flight traces or other tracer instances still running. Severity: action required | Category: reliability How to fix: Make instrumentation process-scoped Agent prompt to fix - you can give this to your LLM of choice:
Qodo code review - free for open-source. |
|
Hi, _uninstrument_http_clients suppresses all Exceptions, so failures to remove global monkeypatches are silent and leave the process in an unknown instrumentation state. Severity: remediation recommended | Category: observability How to fix: Log unexpected uninstrument errors Agent prompt to fix - you can give this to your LLM of choice:
Found by Qodo code review |
|
Those comments look reasonable. I'll look into it. |
Add opentelemetry-instrumentation-requests and opentelemetry-instrumentation-urllib3 to enable W3C TraceContext propagation on outgoing HTTP calls. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Instrument requests and urllib3 HTTP clients with the tracer provider so that traceparent headers are injected on outgoing HTTP requests (e.g., to LLM APIs). This enables end-to-end distributed tracing. Both Arize Phoenix and LangWatch tracers now: - Call _instrument_http_clients() during setup - Call _uninstrument_http_clients() during cleanup - Handle missing packages gracefully with debug logging Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Verify that: - RequestsInstrumentor and URLLib3Instrumentor are called during tracer setup - Instrumentors are uninstrumented during cleanup - traceparent headers are injected with valid W3C format on outgoing requests Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Use contextlib.suppress instead of try-except-pass in uninstrument methods - Fix nested with statements using parenthesized context managers - Fix unused arguments by prefixing with underscore - Add timeout to requests.get calls - Skip integration tests that mock at wrong layer (Session.send bypasses OTel) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…f-counting The OpenTelemetry RequestsInstrumentor and URLLib3Instrumentor monkeypatch globally, but tracers were calling instrument()/uninstrument() per instance. If multiple graph runs overlapped, the first run to end() would uninstrument globally and break context propagation for other active runs. Changes: - Add HTTPClientInstrumentationManager singleton with reference counting - enable() increments count, instruments only on first call - disable() decrements count, uninstruments only when count reaches zero - Replace silent exception suppression with proper logging for debugging Addresses PR review feedback from Qodo. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
3d81468 to
b6dd9dc
Compare
|
These revisions should address the concerns expressed; see b6dd9dc in particular. LLM used to generate the shared context manager as Python isn't my string suit. |
…el-trace-context-propagation # Conflicts: # uv.lock
|
Greatly appreciated @erichare If you get a chance to look over my related PR to support generic OTLP protocol, that'd be great. I'll rebase it onto this. |
Add trace context propagation to outbound requests to Langflow, so that an incoming OpenTelemetry trace context is properly linked to outbound trace contexts.
Fixes #12961
This fix makes it possible to use a fake Langwatch or Arize endpoint to deliver traces to an otel-collector with working trace context propagation to downstream services.