-
Notifications
You must be signed in to change notification settings - Fork 568
OTLPIntegration #4877
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
base: master
Are you sure you want to change the base?
OTLPIntegration #4877
Conversation
da7b9fe to
028a444
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4877 +/- ##
==========================================
+ Coverage 83.92% 83.99% +0.07%
==========================================
Files 179 180 +1
Lines 17948 18001 +53
Branches 3193 3199 +6
==========================================
+ Hits 15062 15120 +58
+ Misses 1913 1908 -5
Partials 973 973
|
726b769 to
f5923b2
Compare
f5923b2 to
525bd76
Compare
beb8ce0 to
6d60191
Compare
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.
Looks good! A few questions just so I understand what's going on better as well.
| if dsn: | ||
| auth = Dsn(dsn).to_auth(f"sentry.python/{VERSION}") | ||
| endpoint = auth.get_api_url(EndpointType.OTLP_TRACES) | ||
| headers = {"X-Sentry-Auth": auth.to_header()} |
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.
Why do we set two headers in the transport and only one here? See
sentry-python/sentry_sdk/transport.py
Lines 325 to 330 in 659bd84
| headers.update( | |
| { | |
| "User-Agent": str(self._auth.client), | |
| "X-Sentry-Auth": str(self._auth.to_header()), | |
| } | |
| ) |
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.
good question! user agent is already set by otlp, which I think is fine to distinguish it from a normal (sentry-traces-only) user
https://github.com/open-telemetry/opentelemetry-python/blob/f3b9e6ec96b7cc3717d5c8b529da7a8923810973/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/__init__.py#L77-L80
| return (trace.format_trace_id(ctx.trace_id), trace.format_span_id(ctx.span_id)) | ||
|
|
||
|
|
||
| def setup_otlp_exporter(dsn=None): |
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.
When dsn is None, is there any point in setting up the provider and processor?
Not sure if I am missing something because I am not as familiar with OTel. I would have thought if dsn is None we could exit early, maybe with a warning message.
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.
they can still manually set env variables as follows:
export OTEL_EXPORTER_OTLP_TRACES_ENDPOINT="https://o0.ingest.sentry.io/api/0/integration/otlp/v1/traces"
export OTEL_EXPORTER_OTLP_TRACES_HEADERS="x-sentry-auth=sentry sentry_key=examplePublicKey"
which will be picked up by otel internally when initializing the exporter. Leaving it open as an option just in case someone has a complicated ops setup.
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.
we'll iterate on the DX here once we get some feedback/complaints/confusion
| assert propagator is original_propagator | ||
|
|
||
|
|
||
| def test_otel_propagation_context(sentry_init): |
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.
When running this test I see the message below. I'm trying to find where this is coming from.
Failed to export span batch code: 404, reason: You should be really using o*.ingest.sentry.io domains.
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.
mocked
5927eb4 to
75c4163
Compare
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.
🚀
| {py3.7,py3.9,py3.12,py3.13,py3.14,py3.14t}-opentelemetry | ||
|
|
||
| # OpenTelemetry with OTLP | ||
| {py3.7,py3.9,py3.12,py3.13,py3.14,py3.14t}-otlp |
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.
Last thing that'd be good before merging.
Since pip install "opentelemetry-distro[otlp]" also pulls in grpcio, and grpcio does not distribute free-threading wheels, these new tests take like 17 mins to run in CI on free-threading 3.14.
https://github.com/getsentry/sentry-python/actions/runs/19139690916/job/54701021577?pr=4877
So I suggest we do not run the new test suite on 3.14t.
Also, as an fyi: for running populate_tox.py, I add a uv command for running it with the new pre-requisites to the README in https://github.com/getsentry/sentry-python/pull/5076/files
That said, in this case you can just edit tox.jinja and tox.ini manually, since it's just removing a single version.


trace_id/span_idfrom otelIssues