-
Notifications
You must be signed in to change notification settings - Fork 9.8k
feat: database spans, and pin that sampling stays env-driven #14420
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
Open
ogabrielluiz
wants to merge
2
commits into
release-1.12.0
Choose a base branch
from
feat/dependency-spans
base: release-1.12.0
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
121 changes: 121 additions & 0 deletions
121
src/backend/tests/unit/services/telemetry/test_dependency_spans.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,121 @@ | ||
| """Dependency spans and sampling. | ||
|
|
||
| Database spans are allowlisted for export; the LLM vendor transports deliberately are not. | ||
| Sampling needs no code of ours, so the check here is that it stays that way. | ||
|
|
||
| Each case runs in a subprocess because the tracer provider is process-global. | ||
| """ | ||
|
|
||
| import gzip | ||
| import http.server | ||
| import os | ||
| import subprocess | ||
| import sys | ||
| import threading | ||
|
|
||
| import pytest | ||
| from lfx.observability import APPLICATION_INSTRUMENTATION_SCOPES | ||
|
|
||
|
|
||
| class _Collector(http.server.BaseHTTPRequestHandler): | ||
| def do_POST(self) -> None: | ||
| body = self.rfile.read(int(self.headers.get("Content-Length", 0))) | ||
| if self.headers.get("Content-Encoding") == "gzip": | ||
| body = gzip.decompress(body) | ||
| self.server.requests.append((self.path, body)) | ||
| self.send_response(200) | ||
| self.send_header("Content-Length", "0") | ||
| self.end_headers() | ||
|
|
||
| def log_message(self, *args) -> None: | ||
| pass | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def collector(): | ||
| server = http.server.HTTPServer(("127.0.0.1", 0), _Collector) | ||
| server.requests = [] | ||
| thread = threading.Thread(target=server.serve_forever, daemon=True) | ||
| thread.start() | ||
| yield server | ||
| server.shutdown() | ||
| server.server_close() | ||
| thread.join(timeout=5) | ||
|
|
||
|
|
||
| def test_outbound_http_scopes_stay_out_of_the_allowlist(): | ||
| """Redacting URLs does not make the LLM transports safe to export; that boundary stands.""" | ||
| for scope in ( | ||
| "opentelemetry.instrumentation.httpx", | ||
| "opentelemetry.instrumentation.requests", | ||
| "opentelemetry.instrumentation.urllib3", | ||
| ): | ||
| assert scope not in APPLICATION_INSTRUMENTATION_SCOPES | ||
|
|
||
|
|
||
| def test_database_spans_are_allowlisted(): | ||
| """Verified separately to carry bound-parameter placeholders, never row values.""" | ||
| assert "opentelemetry.instrumentation.sqlalchemy" in APPLICATION_INSTRUMENTATION_SCOPES | ||
|
|
||
|
|
||
| # The ticket's sampler criterion, run against the real bootstrap rather than a hand-built | ||
| # provider: the same request at ratio 0.0 must export nothing and at 1.0 must export the lot. | ||
| SAMPLER_PROBE = """ | ||
| import json | ||
| from opentelemetry import trace | ||
| from lfx.observability import APPLICATION_TRACER_NAME, bootstrap_application_telemetry | ||
|
|
||
| telemetry = bootstrap_application_telemetry() | ||
| tracer = trace.get_tracer(APPLICATION_TRACER_NAME) | ||
| for _ in range(20): | ||
| with tracer.start_as_current_span("flow.execute"): | ||
| pass | ||
| telemetry.shutdown() | ||
| print("PROBE_RESULT done") | ||
| """ | ||
|
|
||
|
|
||
| def _run_sampler_probe(endpoint: str, ratio: str) -> None: | ||
| env = {k: v for k, v in os.environ.items() if not k.startswith("OTEL_")} | ||
| env["OTEL_EXPORTER_OTLP_ENDPOINT"] = endpoint | ||
| env["OTEL_EXPORTER_OTLP_PROTOCOL"] = "http/protobuf" | ||
| env["OTEL_TRACES_SAMPLER"] = "traceidratio" | ||
| env["OTEL_TRACES_SAMPLER_ARG"] = ratio | ||
| completed = subprocess.run( # noqa: S603 | ||
| [sys.executable, "-c", SAMPLER_PROBE], | ||
| env=env, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=300, | ||
| check=False, | ||
| ) | ||
| assert completed.returncode == 0, completed.stderr | ||
|
|
||
|
|
||
| def _exported_span_count(requests_seen) -> int: | ||
| from opentelemetry.proto.collector.trace.v1.trace_service_pb2 import ExportTraceServiceRequest | ||
|
|
||
| total = 0 | ||
| for path, body in requests_seen: | ||
| if path != "/v1/traces": | ||
| continue | ||
| request = ExportTraceServiceRequest() | ||
| request.ParseFromString(body) | ||
| for rs in request.resource_spans: | ||
| for ss in rs.scope_spans: | ||
| total += len(ss.spans) | ||
| return total | ||
|
|
||
|
|
||
| def test_sampling_off_exports_nothing(collector): | ||
| port = collector.server_address[1] | ||
| _run_sampler_probe(f"http://127.0.0.1:{port}", "0.0") | ||
|
|
||
| assert _exported_span_count(collector.requests) == 0 | ||
|
|
||
|
|
||
| def test_sampling_on_exports_every_span(collector): | ||
| port = collector.server_address[1] | ||
| _run_sampler_probe(f"http://127.0.0.1:{port}", "1.0") | ||
|
|
||
| assert _exported_span_count(collector.requests) == 20 | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 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 onImportError, 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
Source: Coding guidelines