fix: keep LLM tracing vendors off the global tracer provider, and pin it - #14359
fix: keep LLM tracing vendors off the global tracer provider, and pin it#14359ogabrielluiz wants to merge 3 commits into
Conversation
…lity There is one global tracer provider per process and set_tracer_provider is one-shot, so a vendor that registers itself globally takes the slot the application bootstrap needs and then receives every span on it. That is what happened in #13319, where Langfuse claimed the provider and FastAPIInstrumentor's HTTP spans started landing in Langfuse traces. Cover the six vendors that are supposed to hold the property, in both orders: vendor first, where the contest for the global slot happens, and bootstrap first, which is the real deployment order. Each case runs in a subprocess against loopback stubs, since the provider and most vendor clients are process-wide singletons. Both directions are asserted because they fail independently. Reverting the Langfuse fix leaves the provider's identity untouched, since the SDK cannot replace a provider that is already set, while its exporter still attaches to ours and starts receiving the service's spans. Traceloop is deliberately excluded: it does not hold this property and has its own test file.
WalkthroughAdded subprocess-based tests for six tracing vendors. The tests isolate vendor and application endpoints, verify provider ownership across initialization orders, and confirm that application spans reach only the application APM endpoint. ChangesTracing vendor coexistence
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
github-actions
Bot
added
test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/backend/tests/unit/services/tracing/test_vendor_provider_coexistence.py (1)
100-139: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftFixed 1-second sleep may mask the exact regression this suite targets.
report()waits a fixedtime.sleep(1)before inspectingbodies["vendor"]andbodies["apm"]. In_PROBE_BOOTSTRAP_FIRST,telemetry.tracer_provider.force_flush(5000)only forces flush on span processors registered on the application's own tracer provider. If a vendor SDK delivers spans through its own background thread or queue outside that provider's span-processor chain (a plausible path for non-OpenTelemetry vendors such as LangSmith, Opik, or Openlayer), that delivery is not covered byforce_flushand depends entirely on completing within the 1-second window.If a vendor's async flush interval exceeds 1 second,
vendor_saw_application_spanreadsFalseeven when application spans are actually leaking to the vendor, causingtest_application_spans_do_not_reach_the_vendorto pass on a real regression. This directly undermines the property the suite says it pins for issue#13319-stylebugs.Replace the fixed sleep with either an explicit flush/shutdown call on each vendor's client (where the SDK exposes one) before reading
bodies, or a bounded polling loop that only proceeds once no new vendor requests have arrived for a short quiescence window, instead of assuming 1 second is always sufficient.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/tests/unit/services/tracing/test_vendor_provider_coexistence.py` around lines 100 - 139, Replace the fixed time.sleep(1) in the harness report() function with explicit vendor-client flush/shutdown calls when available, or a bounded polling loop that waits for request activity to become quiescent before inspecting bodies. Ensure asynchronous vendor deliveries have enough time to reach the collectors so vendor_saw_application_span and apm_saw_application_span accurately reflect emitted spans without relying on a fixed one-second delay.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/backend/tests/unit/services/tracing/test_vendor_provider_coexistence.py`:
- Around line 100-139: Replace the fixed time.sleep(1) in the harness report()
function with explicit vendor-client flush/shutdown calls when available, or a
bounded polling loop that waits for request activity to become quiescent before
inspecting bodies. Ensure asynchronous vendor deliveries have enough time to
reach the collectors so vendor_saw_application_span and apm_saw_application_span
accurately reflect emitted spans without relying on a fixed one-second delay.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dc0a7db5-b9ed-411b-b226-c6e723d7fdb0
📒 Files selected for processing (1)
src/backend/tests/unit/services/tracing/test_vendor_provider_coexistence.py
erichare
left a comment
erichare
left a comment
There was a problem hiding this comment.
@ogabrielluiz a couple comments!
Code Review Summary
Found 2 critical issues that need to be fixed at head c7dd7d2a61:
🔴 Critical (Must Fix)
1. LangSmith’s OpenTelemetry modes are excluded from the coexistence test
FilePath: [test_vendor_provider_coexistence.py](
prefixes = (..., "LANGSMITH_", ...)Explanation
The harness removes LANGSMITH_TRACING_MODE, so it tests only LangSmith’s default REST transport. Production constructs the pinned langsmith==0.10.10 client with bare Client(), which honors LANGSMITH_TRACING_MODE=otel|hybrid.
Running this exact probe with either mode produced:
ready=True
bootstrap_installed=False
bootstrap_owns_global=False
LangSmith therefore claims the global provider in vendor-first order—the exact regression this PR claims to prevent—while the committed suite remains green.
Suggested Fix
- Add LangSmith
otelandhybridvariants. - Fix the exposed production behavior by using an isolated provider, or explicitly disable/document those modes in Langflow.
- Verify both application and vendor spans reach their intended collectors.
2. The unconditional LangWatch cases fail on Python 3.14
FilePath: [test_vendor_provider_coexistence.py](
@pytest.mark.parametrize("vendor", sorted(_VENDORS))Explanation
Langflow deliberately installs langwatch only on Python <3.14, but both parametrizations require tracer.ready is True.
An isolated Python 3.14 run reproduced:
10 passed, 2 failed
AssertionError: langwatch did not initialise, so this case pins nothing
Both failing cases are selected in the live Python 3.14 Group 5 CI shard; that job was still running when checked.
Suggested Fix
- Apply a parameter-level skip to LangWatch when the dependency is unavailable.
- Retain all other vendor coverage on Python 3.14.
✅ What's Good
- All 12 default-mode cases passed under Python 3.12 in 56.79 seconds.
- Subprocess isolation is appropriate for one-shot global providers.
- The positive APM assertion prevents a trivial “nothing exported” pass.
git diff --checkpassed, and the head is cleanly mergeable with currentrelease-1.12.0.
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
LANGSMITH_TRACING_MODE=otel|hybrid makes the LangSmith client build an OpenTelemetry pipeline, and when it is constructed without a provider it registers its own as the global one. set_tracer_provider is one-shot, so whichever side runs first wins: either LangSmith swallows the service's own HTTP and flow spans, or application observability cannot install at all. Passing a provider to our own Client() is not enough, because the tracing path goes through run_trees.get_cached_client, a module-level singleton built lazily on the first post. Seed that instead, which is also the function langchain calls, so this replaces a redundant second client rather than adding one. Also extend the coexistence tests: LangSmith's two OpenTelemetry transports are now covered rather than only its default REST one, and a vendor whose SDK is absent on the running interpreter is skipped instead of failing the ready guard. langwatch is declared python_version < "3.14", so the unconditional parametrisation was failing the 3.14 shard.
|
Hey @erichare, thanks, both were real. 1 is worse than a test gap. I reproduced it and then found the fix I first tried doesn't work: passing 2 fixed by skipping a vendor whose SDK isn't installed on the running interpreter, rather than only special-casing langwatch. Same outcome on 3.14, and it won't silently pass if another vendor's dependency goes missing. Both LangSmith OTel modes are now in the table. Reverting to the bare 341 passed, 3 skipped locally. On CodeRabbit's point about the fixed |
github-actions
Bot
added
test
Thanks for digging into this. #1 looks resolved: I think #2 needs one small adjustment. On |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release-1.12.0 #14359 +/- ##
==================================================
+ Coverage 54.30% 62.35% +8.05%
==================================================
Files 2300 2361 +61
Lines 228686 239862 +11176
Branches 15799 35715 +19916
==================================================
+ Hits 124188 149573 +25385
+ Misses 102670 88452 -14218
- Partials 1828 1837 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
The previous commit handed LangSmith a bare TracerProvider(). That stops it claiming the global provider, but it also silences it: unlike the Langfuse SDK, which calls add_span_processor on the provider it is given, LangSmith only takes tracers from it. A provider with no processor discards every span while the tracer still reports ready. Measured against a loopback stub bound to LANGCHAIN_ENDPOINT: base otel -> POST /otel 1078 bytes hybrid -> /otel + /runs/multipart bare provider otel -> nothing at all hybrid -> /runs/multipart only this commit otel -> POST /otel 1043 bytes hybrid -> /otel + /runs/multipart In otel mode there is no REST fallback, so the operator simply stopped seeing traces, with no log and no exception. Build the provider the way the SDK's own get_otlp_tracer_provider does, with one deliberate difference: it does not honour OTEL_EXPORTER_OTLP_ENDPOINT, which points at the operator's APM and is not where a vendor's prompt-bearing traces belong. Build it once per process, since TracerProvider registers an atexit shutdown and setup runs per flow run. The tests could not have caught this: nothing asserted the vendor still receives its own traces, so a disconnected vendor passed exactly like an isolated one. The probes now drive a real trace through each tracer and assert the vendor collector received data. Also drop a provider-identity assertion that could not fail, sanitise proxy variables that would swallow the loopback traffic, and wait for the collectors to go quiet rather than sleeping a fixed second.
ogabrielluiz
changed the title
github-actions
Bot
added
test
2 participants
What
One global tracer provider per process, and
set_tracer_provideris one-shot. A vendor that registers itself globally takes the slot the application observability bootstrap needs, and then receives every span on it. That is #13319: Langfuse claimed the global provider andFastAPIInstrumentor's HTTP spans started showing up in Langfuse traces.Two parts:
A fix in
langsmith.py.LANGSMITH_TRACING_MODE=otel|hybridmakes the LangSmith client build an OpenTelemetry pipeline, and a client constructed without a provider registers its own as the global one. So enabling that mode silently disabled application observability. The tracer now hands LangSmith an isolated provider, built once per process, exporting to LangSmith's own OTLP endpoint.Coverage for the rest. Two tests per vendor across both init orders: vendor-first, where the global slot is actually contested, and bootstrap-first, the real deployment order. Each case runs in its own subprocess against loopback stubs, since the global provider and most of these clients are process-wide singletons. No credentials, no network.
The isolated provider needs its own exporter
Worth spelling out, because the obvious version is wrong and fails silently.
langfuse.pypassestracer_provider=and the Langfuse SDK callsadd_span_processoron what it is given. LangSmith does not: it only takes tracers from the provider. So handing it a bareTracerProvider()stops it claiming the global one and discards every span, while the tracer still reports ready. Inotelmode there is no REST fallback, so traces just stop, with no log and no exception.Measured against a loopback stub bound to
LANGCHAIN_ENDPOINT:otelhybrid/otel, 1078 B/otel+/runs/multipart/runs/multipartonly/otel, 1043 B/otel+/runs/multipartIt deliberately does not honour
OTEL_EXPORTER_OTLP_ENDPOINT, which the SDK's own factory reads first. That variable points at the operator's APM, which is not where a vendor's prompt-bearing traces belong.Why both test directions
They fail independently. Reverting the #13319 fix leaves the provider's identity untouched, because the SDK cannot replace a provider that is already set, while its exporter attaches to ours anyway and starts receiving the service's spans.
The suite also asserts the vendor still receives its own traces. Without that, a vendor that was disconnected outright passes exactly like one that is correctly isolated, since the vendor collector is empty either way. That is not hypothetical: it is how the bare-provider bug above got through a green run.
Verification
Checked against real regressions rather than only observed green:
langfuse claimed the global provider, leaving the APM with noneandapplication telemetry leaked to langfuse.skip_open_telemetry_setup=True: same two failures.langsmith_otel received no traces at all, so isolation is not what passed.A vendor whose SDK is absent on the running interpreter is skipped rather than failing the ready guard, since
langwatchis declaredpython_version < "3.14".Not covered
Traceloop, which genuinely does not hold this property — that is #14357, with its own test file. And
native, which writes to Langflow's own database and imports no OpenTelemetry, so a case for it would pass without exercising anything.Known coarseness: the "vendor still received data" assertion is a byte count, so in
hybridmode a broken OTel leg is masked by the REST leg still posting. It catches the fully-silent case, which is the one that ships unnoticed.