fix: stop shipping application telemetry to the LLM tracing vendor - #14357
fix: stop shipping application telemetry to the LLM tracing vendor#14357ogabrielluiz wants to merge 3 commits into
Conversation
The Traceloop SDK takes no tracer provider. It adopts whichever provider is global and adds its exporter to that, so when application observability is enabled it attaches to ours and its exporter sees every span on it, including the service's own HTTP and flow spans. Those are what the operator pointed at their APM; they were also being shipped to api.traceloop.com. Wrap the provider's add_span_processor for the duration of Traceloop.init so whatever the SDK attaches is filtered, dropping the scopes the application allowlist claims and passing everything else through. The SDK's own pipeline is left intact: its endpoint scheme dispatch, TRACELOOP_HEADERS auth, metrics and prompt sync all still run, which is not the case when a processor is supplied to init instead. Known limitation, pinned by a test: if Traceloop initialises before the bootstrap it owns the global provider and there is nothing for us to wrap. In practice the bootstrap runs at startup and Traceloop only on the first flow run.
WalkthroughTraceloop now excludes Langflow application spans from vendor exports while forwarding vendor spans. Initialization verifies filter installation and fails closed when the SDK bypasses the hook. Subprocess tests cover routing and initialization-order scenarios. ChangesTraceloop telemetry routing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Application
participant TraceloopSetup
participant ApplicationScopeFilter
participant OperatorAPM
participant TraceloopVendor
Application->>TraceloopSetup: create application span
TraceloopSetup->>ApplicationScopeFilter: process span
ApplicationScopeFilter->>OperatorAPM: forward application span
ApplicationScopeFilter-->>TraceloopVendor: withhold application span
Application->>TraceloopSetup: create vendor span
TraceloopSetup->>ApplicationScopeFilter: process vendor span
ApplicationScopeFilter->>TraceloopVendor: forward vendor span
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 7 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (7 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 |
erichare
left a comment
erichare
left a comment
There was a problem hiding this comment.
@ogabrielluiz looks great, just a couple things, can you take a look?
Code Review Summary
Found 2 critical issues need to be fixed:
🔴 Critical (Must Fix)
1. Proxy-provider path still exports application spans to Traceloop
FilePath: [src/backend/base/langflow/services/tracing/traceloop.py](
langflow/src/backend/base/langflow/services/tracing/traceloop.py
Lines 99 to 103 in a3e8073
if not hasattr(provider, "add_span_processor"):
yield
returnExplanation
When startup has no OTLP trace endpoint, bootstrap_application_telemetry() leaves ProxyTracerProvider installed. This branch skips filtering, after which Traceloop installs its own concrete global provider. Existing proxy tracers and future FastAPI, ASGI, and langflow.observability tracers then delegate to that unfiltered provider.
A bootstrap-first/no-endpoint loopback probe on head a3e8073d produced:
apm=[] traceloop=['flow.execute']
That is a normal Traceloop-only configuration, not merely the documented vendor-before-bootstrap edge case.
Suggested Fix
- Apply the filter to Traceloop’s processor/exporter construction regardless of the initial provider type.
- Add a regression test that bootstraps without an OTLP endpoint, initializes Traceloop, and requires
flow.executeto remain absent from the vendor collector.
2. The process-wide patch can capture unrelated processors
FilePath: [src/backend/base/langflow/services/tracing/traceloop.py](
langflow/src/backend/base/langflow/services/tracing/traceloop.py
Lines 105 to 112 in a3e8073
provider.add_span_processor = add_span_processorExplanation
_INIT_LOCK coordinates only callers of this helper. Every other thread sees the replaced method during Traceloop.init. If another integration registers a processor then, it is permanently wrapped with _ApplicationScopeFilter.
A barrier-based reproduction showed the unrelated processor dropped langflow.observability while accepting the vendor span—the exact inverse of its intended boundary. An unrelated APM could therefore lose application telemetry and receive LLM/vendor spans instead.
Suggested Fix
- Avoid patching the shared provider method; intercept only Traceloop’s own processor factory or exporter.
- Add a concurrent-registration test proving an unrelated processor remains untouched.
Found 1 suggestion for improvement:
🟡 Suggestions (Should Consider)
1. Preserve existing provider hooks exactly
FilePath: [src/backend/base/langflow/services/tracing/traceloop.py](
langflow/src/backend/base/langflow/services/tracing/traceloop.py
Lines 105 to 112 in a3e8073
Explanation
Calling type(provider).add_span_processor(...) bypasses instance-level provider hooks, while unconditional del provider.add_span_processor removes any hook that existed before entry. Dynamic providers can also pass hasattr() but lack a class-level method, causing initialization to fail.
Suggested Fix
- If interception remains, capture the original bound callable and prior instance-attribute state, delegate through that callable, and restore the exact state in
finally.
✅ What's Good
- Current-head validation passed:
328 passed, 3 skipped, plus targeted Ruff, format, andgit diff --check. - The configured-APM happy-path test correctly demonstrates the original leak and verifies vendor spans still export.
- GitHub reports the PR mergeable. The backend CI job was skipped because this is a draft; one accessibility scan remains pending.
Two problems with patching the global provider's add_span_processor. When no OTLP endpoint is configured the bootstrap installs nothing and the global provider is still a proxy, so there was nothing to patch and the filter was skipped. Traceloop then registers its own concrete provider, the proxy resolves onto it, and the service's spans reach the vendor by the same route. That is the ordinary Traceloop-without-an-APM setup, not an edge case. The provider is also shared: a processor another integration registered while init was running would have been wrapped too, and would then have had this filter's boundary applied backwards. Wrap the SDK's get_default_span_processor for the duration of init instead. It applies whichever provider Traceloop ends up with, and touches nothing outside the SDK. As a side effect the vendor-first ordering no longer leaks either: the APM still receives nothing, but the span is dropped rather than exported.
|
Hey @erichare, thanks for the review. 1 is real and my comment on that branch was wrong. Reproduced it with no OTLP endpoint: the proxy resolves onto traceloop's provider once it registers, and the app span lands at the vendor. Fixed both by moving the filter off the provider. It now wraps traceloop's own Added Side effect: this also fixes the vendor-first ordering I'd written up as a known limitation. The APM still receives nothing there ( On the concurrent-registration test, with the provider patch gone I'm not sure what it would pin. Do you still want one? |
github-actions
Bot
added
bug
erichare
left a comment
erichare
left a comment
There was a problem hiding this comment.
@ogabrielluiz Thanks, that addresses my concerns. No concurrent-registration test needed now; it was specific to the shared provider patch. The no-APM regression and updated vendor-first test cover the important paths.
The filter is installed by wrapping a function of the SDK's, and traceloop-sdk is depended on across >=0.43.1,<1.0.0. If a release stops routing its exporter through get_default_span_processor the filter stops applying and nothing else changes: spans keep flowing, and the only difference is that the service's own telemetry is in them. CI cannot catch that, because CI resolves the lockfile while the version that breaks it is the one a user installs. Record whether the filter actually installed, and disable the integration with an explanatory error when an init that builds the SDK's pipeline did not get one. A broken LLM tracing integration is recoverable; silently shipping the operator's HTTP and flow spans to a third party is not. Also note in the module why this matters in every install rather than only where an APM is configured: instrument_fastapi_app runs unconditionally at startup, so HTTP server spans are always being produced.
github-actions
Bot
added
bug
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
github-actions
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/backend/base/langflow/services/tracing/traceloop.py (1)
140-151: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm the intended visibility of the fail-closed error.
_application_telemetry_withheldraisesRuntimeError. The constructor at Line 202 catches every exception and logs at debug level. Thelogger.errorcall at Line 150 is therefore the only operator-visible signal. That works, but the pairing oflogger.errorplusraiseinside the same block also produces a debug traceback for the same event. Consider raising without thelogger.errorcall and letting the caller log at error level for this specific failure, so the pinned-version guidance is not duplicated.🤖 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/base/langflow/services/tracing/traceloop.py` around lines 140 - 151, The fail-closed path in _application_telemetry_withheld currently logs the same RuntimeError at error and debug levels. Remove the local logger.error call and preserve the RuntimeError message, then update the constructor’s exception handling to recognize this specific failure and log it at error level while retaining debug logging for other exceptions.src/backend/tests/unit/services/tracing/test_traceloop_application_telemetry.py (2)
48-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the fixed sleep with polling to reduce flake risk.
reportwaits exactly one second, then reads the collector bodies once. On a loaded CI machine the HTTP handlers can still be in flight. The assertions then report an empty list, which reads as a filtering regression rather than a timing problem. Poll for the expected span names until a deadline instead.Note that a negative assertion, for example
result["traceloop"] == [], still needs a full wait, so keep a floor on the wait time.♻️ Proposed polling helper
-def report(**extra): - time.sleep(1) - seen = {} - for target in ("apm", "traceloop"): - seen[target] = sorted( - name.decode() - for name in (b"flow.execute", b"llm.call") - if any(name in body for body in bodies[target]) - ) - print("RESULT " + json.dumps({**seen, **extra})) +def _seen(): + seen = {} + for target in ("apm", "traceloop"): + seen[target] = sorted( + name.decode() + for name in (b"flow.execute", b"llm.call") + if any(name in body for body in bodies[target]) + ) + return seen + +def report(expected=0, **extra): + # Settle floor: negative assertions need the full wait, so never return early below it. + deadline = time.monotonic() + 5 + time.sleep(1) + seen = _seen() + while time.monotonic() < deadline and sum(len(v) for v in seen.values()) < expected: + time.sleep(0.1) + seen = _seen() + print("RESULT " + json.dumps({**seen, **extra}))Each probe then passes the number of span names it expects, for example
report(expected=1, ready=tracer._ready).🤖 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_traceloop_application_telemetry.py` around lines 48 - 57, Update report to replace the fixed time.sleep(1) with deadline-based polling that repeatedly checks the collector bodies for the expected span names and supports the proposed expected/ready inputs. Preserve a minimum wait duration so negative assertions still observe the full interval, and update callers to provide the expected span count and tracer readiness signal.
101-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the public
readyproperty.These probes read
tracer._ready. The probe at Line 170 readstracer.ready.readyis a public property that returns the same value. Use it in all three probes for consistency.♻️ Proposed change
- report(ready=tracer._ready) + report(ready=tracer.ready)- report(ready=tracer._ready, installed=telemetry.tracer_provider is not None) + report(ready=tracer.ready, installed=telemetry.tracer_provider is not None)Also applies to: 128-128
🤖 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_traceloop_application_telemetry.py` at line 101, Update all three telemetry probes in the tracing test to read the public tracer.ready property instead of the private tracer._ready attribute, including the report call and the probe referenced at line 128, while preserving the existing report behavior.
🤖 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/base/langflow/services/tracing/traceloop.py`:
- Around line 140-151: The fail-closed path in _application_telemetry_withheld
currently logs the same RuntimeError at error and debug levels. Remove the local
logger.error call and preserve the RuntimeError message, then update the
constructor’s exception handling to recognize this specific failure and log it
at error level while retaining debug logging for other exceptions.
In
`@src/backend/tests/unit/services/tracing/test_traceloop_application_telemetry.py`:
- Around line 48-57: Update report to replace the fixed time.sleep(1) with
deadline-based polling that repeatedly checks the collector bodies for the
expected span names and supports the proposed expected/ready inputs. Preserve a
minimum wait duration so negative assertions still observe the full interval,
and update callers to provide the expected span count and tracer readiness
signal.
- Line 101: Update all three telemetry probes in the tracing test to read the
public tracer.ready property instead of the private tracer._ready attribute,
including the report call and the probe referenced at line 128, while preserving
the existing report behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 18e1fb7c-28e9-423e-9a63-9eda7625ee04
📒 Files selected for processing (2)
src/backend/base/langflow/services/tracing/traceloop.pysrc/backend/tests/unit/services/tracing/test_traceloop_application_telemetry.py
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (40.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## release-1.12.0 #14357 +/- ##
==================================================
+ Coverage 54.30% 61.96% +7.65%
==================================================
Files 2300 2339 +39
Lines 228686 239148 +10462
Branches 15799 33561 +17762
==================================================
+ Hits 124188 148182 +23994
+ Misses 102670 89129 -13541
- 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:
|
2 participants
What
The Traceloop SDK takes no tracer provider. It adopts whichever provider is global and attaches its exporter to that, so its exporter sees every span on that provider, including the service's own HTTP and flow spans.
instrument_fastapi_appruns unconditionally at startup, so those HTTP spans exist in every install, whether or not an APM is configured. They were being shipped toapi.traceloop.comalongside the LLM traces the operator actually asked for.The fix wraps the SDK's own span processor factory for the duration of
Traceloop.init, so whatever it builds is filtered: spans whose instrumentation scope is in the application allowlist are dropped, everything else passes through untouched.Why the factory and not the provider
The factory is the one place every export path goes through.
Patching the provider's
add_span_processoronly covers the case where a provider already exists. It misses the more common one: with no OTLP endpoint the bootstrap installs nothing, the global provider is still a proxy, the SDK creates and registers its own concrete provider, and the proxy resolves onto it. Same leak, different route. The provider is also a shared object, so a processor another integration registered duringinitwould have been wrapped too and had this boundary applied backwards.Passing
processor=toinitis the other obvious hook and is worse. It turns off the SDK's metrics, drops theTRACELOOP_HEADERSauth the Instana integration depends on, makesdisable_batchinert, and skips config/prompt sync. Wrapping the factory leaves all of that alone, because frominit's point of view no processor was supplied.Fail-closed
traceloop-sdkis depended on across>=0.43.1,<1.0.0and the filter is installed by wrapping a function of theirs. If a release stops routing throughget_default_span_processor, the filter stops applying and there is no other symptom: spans keep flowing, and the only difference is that the service's telemetry is in them. CI will not catch it either, since CI resolves the lockfile while the version that breaks it is the one a user installs.So the integration now records whether the filter actually installed, and disables itself with an explanatory error if an init that built the SDK's pipeline did not get one. A broken LLM tracing integration is recoverable; silently shipping the operator's HTTP and flow spans to a third party is not. Happy to be argued out of the severity of that response.
Verification
Every test was checked against the failure it claims to catch, not just observed green:
flow.executespan arrives at both the APM and the vendor. With it, the APM getsflow.executeand the vendor gets onlyllm.call.TRACELOOP_HEADERSstill reaches the collector, metrics stay enabled, and 200 tracer constructions add one thread rather than 200.src/backend/tests/unit/services/tracing/.Known gap
If Traceloop initialises before the bootstrap it owns the global provider and
set_tracer_provideris one-shot, so the operator's APM receives nothing. That part is not fixed here and cannot be from our side in the same process. The leak direction is fixed in that ordering, because the filter is on Traceloop's own processor rather than on a provider. There is a test pinning both halves.Note on direction
This is a workaround for one SDK's design choice. Seven of the eight tracing vendors either accept a
tracer_provider=or build their own; Traceloop is the only one that adopts the global. The durable fix is upstream, and this should be deletable once it lands.Companion: #14359 pins that the other six vendors coexist with the bootstrap.
Summary by CodeRabbit
Bug Fixes
Tests