Python: Fix OTLP HTTP base-endpoint losing /v1/{signal} auto-append - #5913
Conversation
|
@TaoChenOSU , @moonbox3 , @eavanvalkenburg , @chetantoshniwal - Kindly review when you get a chance. |
There was a problem hiding this comment.
Pull request overview
Fixes OTLP/HTTP exporter endpoint handling in Python observability so that OTEL_EXPORTER_OTLP_ENDPOINT (base URL) correctly results in signal-specific URLs (/v1/traces, /v1/metrics, /v1/logs) when configuring exporters programmatically, matching the OpenTelemetry spec and avoiding telemetry being sent to the wrong path.
Changes:
- Compute HTTP signal endpoints by auto-appending
/v1/{signal}when falling back toOTEL_EXPORTER_OTLP_ENDPOINT; keep signal-specific endpoint env vars verbatim. - Preserve existing gRPC behavior (base endpoint used as-is).
- Add unit tests validating HTTP base endpoint appends, trailing-slash handling, verbatim signal-specific endpoints, and unchanged gRPC behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| python/packages/core/agent_framework/observability.py | Implements correct OTLP/HTTP base-endpoint → signal endpoint derivation while leaving gRPC behavior unchanged. |
| python/packages/core/tests/core/test_observability.py | Adds focused tests to verify endpoint computation logic across HTTP and gRPC scenarios. |
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
moonbox3
left a comment
moonbox3
left a comment
There was a problem hiding this comment.
Non-blocking, two gaps I noticed while reading that this PR doesn't introduce:
-
OTEL_EXPORTER_OTLP_PROTOCOL=http/jsonis a valid spec value but_create_otlp_exportersonly matches("http/protobuf", "http")(observability.py:418), so http/json is silently ignored. The new auto-append check inherits the same set, which is consistent, just worth knowing. -
Per-signal protocol env vars (
OTEL_EXPORTER_OTLP_TRACES_PROTOCOL, etc.) aren't read in_get_exporters_from_env. A user pinning base togrpcand traces tohttp/protobufwouldn't get auto-append on traces. Pre-existing.
Happy to leave both for a follow-up if you'd rather keep this PR scoped to the base-endpoint fix. Fine to merge as-is from my side.
|
@droideronline please have a look at the failing CI/CD checks. Thanks. |
e3aeabb to
3bae52d
Compare
Per the OTel spec, OTEL_EXPORTER_OTLP_ENDPOINT is a *base* URL for HTTP — the SDK auto-appends /v1/traces, /v1/metrics, /v1/logs when it reads the env var directly. Signal-specific endpoint env vars are *full* URLs used verbatim. _get_exporters_from_env read the base endpoint and forwarded it as the constructor ``endpoint=`` argument, which the SDK always treats as a full signal URL. As a result, with OTEL_EXPORTER_OTLP_ENDPOINT=http://localhost:4318 and HTTP protocol, the exporter sent to http://localhost:4318 instead of http://localhost:4318/v1/traces (and likewise for metrics/logs). Replicate the spec's auto-append here when falling back to the base endpoint under HTTP. gRPC behavior is unchanged.
Pre-declare traces_endpoint, metrics_endpoint, logs_endpoint as str | None before the if/else block. Mypy inferred str from the if-branch f-string assignments and then rejected the str | None expressions in the else-branch as incompatible.
3bae52d to
ace1059
Compare
|
@moonbox3 - Thanks!, |
Summary
Fixes #5912
Per the OpenTelemetry specification,
OTEL_EXPORTER_OTLP_ENDPOINTis a base URL for HTTP transport — the SDK automatically appends/v1/traces,/v1/metrics, and/v1/logswhen it reads the env var directly. Signal-specific env vars (e.g.OTEL_EXPORTER_OTLP_TRACES_ENDPOINT) are full URLs used verbatim._get_exporters_from_envinobservability.pywas reading the base endpoint and passing it directly as theendpoint=constructor argument to the exporters. The OTel SDK always treats a programmatically-providedendpoint=as a full signal URL (no auto-append). This silently dropped the/v1/{signal}path suffix, causing all HTTP telemetry to be sent to the wrong URL (e.g.http://localhost:4318instead ofhttp://localhost:4318/v1/traces).Changes
python/packages/core/agent_framework/observability.pyhttp/protobuforhttp) and falling back toOTEL_EXPORTER_OTLP_ENDPOINT, auto-append/v1/traces,/v1/metrics,/v1/logsto replicate the OTel spec behaviour.python/packages/core/tests/core/test_observability.pyBefore / After
OTEL_EXPORTER_OTLP_ENDPOINT=http://localhost:4318,PROTOCOL=http/protobufhttp://localhost:4318❌http://localhost:4318/v1/traces✅OTEL_EXPORTER_OTLP_TRACES_ENDPOINT=http://localhost:4318/v1/traces,PROTOCOL=http/protobufhttp://localhost:4318/v1/traces✅http://localhost:4318/v1/traces✅OTEL_EXPORTER_OTLP_ENDPOINT=http://localhost:4317,PROTOCOL=grpchttp://localhost:4317✅http://localhost:4317✅Testing
Unit tests added in
test_observability.pycovering all three scenarios above. No integration-level changes required.