feat: database spans, and pin that sampling stays env-driven - #14420
feat: database spans, and pin that sampling stays env-driven#14420ogabrielluiz wants to merge 2 commits into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughChangesThe change adds SQLAlchemy OpenTelemetry instrumentation, excludes outbound HTTP instrumentation, redacts credentials from exported URL attributes, wires instrumentation into application startup, and adds subprocess telemetry tests. Dependency telemetry
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CreateApp
participant FastAPIInstrumentation
participant DependencyInstrumentation
participant SQLAlchemy
CreateApp->>FastAPIInstrumentation: instrument FastAPI application
CreateApp->>DependencyInstrumentation: instrument dependencies
DependencyInstrumentation->>SQLAlchemy: configure instrumentation
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (7 passed)
✨ Finishing Touches 💡 1📝 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! 🎉
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release-1.12.0 #14420 +/- ##
==================================================
+ Coverage 61.51% 62.88% +1.37%
==================================================
Files 2408 2376 -32
Lines 240471 240606 +135
Branches 36217 37390 +1173
==================================================
+ Hits 147914 151309 +3395
+ Misses 90617 87357 -3260
Partials 1940 1940
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/backend/tests/unit/services/telemetry/test_dependency_span_redaction.py (1)
105-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRun both probes through
uv run.
src/backend/tests/unit/services/telemetry/test_dependency_span_redaction.py#L105-L106: invoke the file probe throughuv run python.src/backend/tests/unit/services/telemetry/test_dependency_span_redaction.py#L175-L176: invoke the inline sampler probe throughuv run python.As per coding guidelines, “Backend code must use
uv runwhen running Python commands to ensure correct environment setup.”🤖 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/telemetry/test_dependency_span_redaction.py` around lines 105 - 106, Update both subprocess probes in test_dependency_span_redaction.py: the file probe at lines 105-106 and the inline sampler probe at lines 175-176 must invoke Python through “uv run python” instead of calling sys.executable directly. Apply the same command-prefix change at both sites.Source: Coding guidelines
🤖 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.
Inline comments:
In `@src/backend/tests/unit/services/telemetry/test_dependency_span_redaction.py`:
- Around line 147-149: Replace the allowlist-only check in
test_database_spans_are_allowlisted with a subprocess-based probe that calls
instrument_dependencies(), executes a parameterized SQLite query using a unique
sentinel, and inspects the exported SQLAlchemy span. Assert instrumentation
succeeds and the span contains SQL parameter placeholders without exposing the
sentinel value.
- Around line 118-124: Extend the telemetry redaction tests around
SERVER_SPAN_PROBE with an allowed-span probe containing query parameters and URL
userinfo, then assert the exported url.full and http.url values retain only the
scheme, host, port, and path, with credentials and query data removed. Keep the
existing serve API key regression intact and verify the new pytest coverage
exercises both URL attributes.
---
Nitpick comments:
In `@src/backend/tests/unit/services/telemetry/test_dependency_span_redaction.py`:
- Around line 105-106: Update both subprocess probes in
test_dependency_span_redaction.py: the file probe at lines 105-106 and the
inline sampler probe at lines 175-176 must invoke Python through “uv run python”
instead of calling sys.executable directly. Apply the same command-prefix change
at both sites.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 381dd170-ff07-42e5-84e5-f755665dc6bb
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.secrets.baselinesrc/backend/base/langflow/main.pysrc/backend/base/pyproject.tomlsrc/backend/tests/unit/services/telemetry/test_dependency_span_redaction.pysrc/lfx/pyproject.tomlsrc/lfx/src/lfx/cli/serve_app.pysrc/lfx/src/lfx/observability.py
| def test_the_serve_api_key_never_reaches_the_apm(): | ||
| """Regression for a live leak: the key is accepted as a query param and the span kept it.""" | ||
| result = run_probe(SERVER_SPAN_PROBE) | ||
|
|
||
| assert result["spans"], "expected a server span carrying url.path" | ||
| blob = json.dumps(result["spans"]) | ||
| assert SECRET not in blob, f"serve API key reached the APM: {blob}" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Cover URL userinfo redaction.
This probe tests only url.query. It does not prove that url.full and http.url remove user:password@host.
Add an allowed-span probe with query and userinfo attributes. Assert that the exported attributes retain the scheme, host, port, and path only.
As per coding guidelines, “For new backend implementations or bug fixes, ensure corresponding pytest test files are included ... and verify the tests actually cover the new or changed behavior.”
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 122-122: use jsonify instead of json.dumps for JSON output
Context: json.dumps(result["spans"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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/telemetry/test_dependency_span_redaction.py`
around lines 118 - 124, Extend the telemetry redaction tests around
SERVER_SPAN_PROBE with an allowed-span probe containing query parameters and URL
userinfo, then assert the exported url.full and http.url values retain only the
scheme, host, port, and path, with credentials and query data removed. Keep the
existing serve API key regression intact and verify the new pytest coverage
exercises both URL attributes.
Source: Coding guidelines
| def test_database_spans_are_allowlisted(): | ||
| """Verified separately to carry bound-parameter placeholders, never row values.""" | ||
| assert "opentelemetry.instrumentation.sqlalchemy" in APPLICATION_INSTRUMENTATION_SCOPES |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Exercise SQLAlchemy instrumentation with a real query.
This assertion checks only a string in the allowlist. It passes if _instrument_sqlalchemy() returns on ImportError, catches an instrumentation error, or exports bound values instead of SQL placeholders.
Add a subprocess probe that invokes instrument_dependencies(), runs a parameterized SQLite query with a sentinel value, and asserts that an exported SQLAlchemy span omits that sentinel.
As per coding guidelines, “For new backend implementations or bug fixes, ensure corresponding pytest test files are included ... and verify the tests actually cover the new or changed behavior.”
🤖 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/telemetry/test_dependency_span_redaction.py`
around lines 147 - 149, Replace the allowlist-only check in
test_database_spans_are_allowlisted with a subprocess-based probe that calls
instrument_dependencies(), executes a parameterized SQLite query using a unique
sentinel, and inspects the exported SQLAlchemy span. Assert instrumentation
succeeds and the span contains SQL parameter placeholders without exposing the
sentinel value.
Source: Coding guidelines
ogabrielluiz
changed the title
6c32b6a to
edff026
Compare
github-actions
Bot
added
enhancement
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
ec94200 to
c028037
Compare
edff026 to
efe096b
Compare
github-actions
Bot
added
enhancement
|
Self-review after running this against a real APM (grafana/otel-lgtm, local): the DB spans in this PR do not deliver what the description claims, and I would not merge it as is. One Cause: langflow builds an Fix is to call it once the database service has built its engine, passing that engine, rather than from the app factory. I will push that rather than leave the claim standing. Worth noting for whoever reviews: the unit tests passed throughout, because they assert the scope is allowlisted and that the instrumentor is wired, not that a query span ever reaches an exporter. The gap only showed up against a real backend. |
github-actions
Bot
added
enhancement
|
✅ Migration Validation Passed All migrations follow the Expand-Contract pattern correctly. |
|
Heads up @erichare @Cristhianzl @Adam-Aghili, this changed materially after I asked you to look, so please review from the current head rather than what you were pinged on. What moved: Why: the DB spans did not work. Verified against a live OTLP backend, one API request produced 13 sqlalchemy connect spans and zero query spans, no db.statement anywhere. Langflow builds an AsyncEngine and the instrumentor patches the sync engine underneath it, so instrumenting globally from the app factory (where no engine exists yet) attached to pool events only. Same request after the change: 28 query spans with db.statement, bound parameters still placeholders, and the flow's input text absent from the trace. The part worth a reviewer's attention: my unit tests passed before and after. They assert the scope is allowlisted and the instrumentor is wired, not that a query span ever reaches an exporter, so nothing in CI would have caught this. If you have a view on how to test that without standing up a collector in CI, I would take it. The two CI reds here are a docker job whose build step was cancelled mid-build and a Playwright shard; both look like flakes and I have re-run them. I will not call them clean until they come back green. |
github-actions
Bot
added
enhancement
github-actions
Bot
added
enhancement
SQLAlchemy is instrumented with an explicit tracer_provider, so a slow request can be attributed to the queries it made. Allowlisted only after probing what those spans carry: db.statement keeps bound parameters as placeholders, so chat message text stays in the database. The outbound HTTP scopes (httpx, requests, urllib3) are deliberately still absent. They are the transports the LLM vendor SDKs ride on, and those SDKs instrument them globally against whichever provider is global, so admitting them would put one span per outbound provider call into the operator's APM. Outbound provider health has a leak-safe metrics path instead. Left as its own decision rather than a side effect of this ticket. Sampling needed no change: the provider takes no explicit sampler, so OTEL_TRACES_SAMPLER already applies. Pinned end to end against a loopback collector, 0 spans at ratio 0.0 and all 20 at 1.0, so that adding a sampler later cannot silently take env control away.
The DB spans this PR added did not deliver what it claimed. Verified against a live backend: one API request produced 13 sqlalchemy connect spans and zero query spans, with no db.statement anywhere. Connection-pool noise dressed up as database visibility. Langflow builds an AsyncEngine, and the instrumentor patches the sync engine underneath it. instrument_dependencies() was called from the app factory with no engine, because the engine does not exist that early, so SQLAlchemy instrumented globally and attached to pool events only. Instrument where the engine is built instead, passing it. Renamed to instrument_database, and the engine argument is required rather than optional: the engine-less call is exactly the broken configuration, and a global instrument() would also win the race and make a later engine-specific call a silent no-op. Same request after the change: 28 query spans carrying db.statement, bound parameters still placeholders, and the flow's input text absent from the trace. The unit tests passed throughout. They assert the scope is allowlisted and the instrumentor is wired, not that a query span reaches an exporter, so this was only visible against a real backend.
31d5a2e to
842d6ea
Compare
github-actions
Bot
added
enhancement
2 participants
Stacked on #14422 and merges after it. The URL redaction that was originally in this PR moved there, so it can land at security speed; the diff below is against that branch.
Database spans
SQLAlchemy is instrumented with an explicit
tracer_provider, so a slow request can be attributed to the queries it made.Allowlisted only after checking what those spans actually carry.
db.statementkeeps bound parameters as placeholders:so chat message text stays in the database. Verified by probe rather than assumed, because assuming is how the outbound-HTTP hole got opened the first time.
What this deliberately does not do
The ticket also asks for httpx spans. This does not add them, and does not allowlist
httpx,requestsorurllib3.Those are the transports the OpenAI and Anthropic SDKs ride on, and the vendor SDKs instrument them globally against whichever provider is global — ours. Admitting them puts one span per outbound provider call into the operator's APM. Four existing tests encode that decision explicitly, and outbound provider health already has a leak-safe metrics path built for exactly this reason. Reversing it is a security-posture call that belongs with the people who made it, not a side effect of this ticket.
Redacting URLs would remove the credential from those spans, but not the underlying question of whether LLM vendor traffic belongs in the operator's APM at all. That question is still open.
All four guard tests still pass unchanged.
Sampling
No change was needed. The provider takes no explicit sampler, so
OTEL_TRACES_SAMPLERalready applies. Pinned with an end-to-end determinism check against a loopback collector: ratio0.0exports 0 spans,1.0exports all 20. The test exists so that adding an explicit sampler later cannot silently take env control away.Verification
statusfrom the sampler probetest_application_span_filter.pythat guard the boundary.