Add a timeout param to all OTLP grpc / http export calls -- fixed merge conflicts #4564
+369
−315
Conversation
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
3 tasks
emdneto
reviewed
May 2, 2025
...orter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/metric_exporter/__init__.py
Outdated
Show resolved
Hide resolved
emdneto
reviewed
May 9, 2025
There was a problem hiding this comment.
Running a basic example of sending span/metric to a non existent collector through grpc:
Before (timeout not respected)
$ OTEL_EXPORTER_OTLP_TIMEOUT=5 uv run repro.py
2025-05-09 01:19:57 INFO [test] Hello world
2025-05-09 01:19:57 WARNING [opentelemetry.exporter.otlp.proto.grpc.exporter] Transient error StatusCode.UNAVAILABLE encountered while exporting metrics to localhost:4317, retrying in 1s.
2025-05-09 01:19:58 WARNING [opentelemetry.exporter.otlp.proto.grpc.exporter] Transient error StatusCode.UNAVAILABLE encountered while exporting metrics to localhost:4317, retrying in 2s.
2025-05-09 01:20:00 WARNING [opentelemetry.exporter.otlp.proto.grpc.exporter] Transient error StatusCode.UNAVAILABLE encountered while exporting metrics to localhost:4317, retrying in 4s.
^[[B2025-05-09 01:20:02 WARNING [opentelemetry.exporter.otlp.proto.grpc.exporter] Transient error StatusCode.UNAVAILABLE encountered while exporting traces to localhost:4317, retrying in 1s.
2025-05-09 01:20:03 WARNING [opentelemetry.exporter.otlp.proto.grpc.exporter] Transient error StatusCode.UNAVAILABLE encountered while exporting traces to localhost:4317, retrying in 2s.
2025-05-09 01:20:04 WARNING [opentelemetry.exporter.otlp.proto.grpc.exporter] Transient error StatusCode.UNAVAILABLE encountered while exporting metrics to localhost:4317, retrying in 8s.
2025-05-09 01:20:05 WARNING [opentelemetry.exporter.otlp.proto.grpc.exporter] Transient error StatusCode.UNAVAILABLE encountered while exporting traces to localhost:4317, retrying in 4s.
2025-05-09 01:20:09 WARNING [opentelemetry.exporter.otlp.proto.grpc.exporter] Transient error StatusCode.UNAVAILABLE encountered while exporting traces to localhost:4317, retrying in 8s.
2025-05-09 01:20:12 WARNING [opentelemetry.exporter.otlp.proto.grpc.exporter] Transient error StatusCode.UNAVAILABLE encountered while exporting metrics to localhost:4317, retrying in 16s.
Now (timeout is respected)
$ OTEL_EXPORTER_OTLP_TIMEOUT=5 uv run repro.py
2025-05-09 01:22:43 INFO [test] Hello world
2025-05-09 01:22:48 ERROR [opentelemetry.exporter.otlp.proto.grpc.exporter] Failed to export metrics to localhost:4317, error code: StatusCode.DEADLINE_EXCEEDED
2025-05-09 01:22:53 ERROR [opentelemetry.exporter.otlp.proto.grpc.exporter] Failed to export traces to localhost:4317, error code: StatusCode.DEADLINE_EXCEEDED
When exporting metrics/traces in the same program I noticed that:
Is this expected?
$ OTEL_EXPORTER_OTLP_TRACES_TIMEOUT=5 uv run repro.py
2025-05-09 15:37:54 INFO [test] Hello world
2025-05-09 15:38:04 ERROR [opentelemetry.exporter.otlp.proto.grpc.exporter] Failed to export traces to localhost:4317, error code: StatusCode.DEADLINE_EXCEEDED
2025-05-09 15:38:04 ERROR [opentelemetry.exporter.otlp.proto.grpc.exporter] Failed to export metrics to localhost:4317, error code: StatusCode.DEADLINE_EXCEEDED
emdneto
reviewed
May 9, 2025
...lemetry-exporter-opencensus/src/opentelemetry/exporter/opencensus/trace_exporter/__init__.py
Outdated
Show resolved
Hide resolved
| { | ||
| "name": [dict()], | ||
| "retryPolicy": { | ||
| "maxAttempts": 5, |
There was a problem hiding this comment.
@DylanRussell could you please add a comment based on what we discussed in slack?
attempt 1 at t0, attempt 2 at t1, attempt 3 at t3, attempt 4 at t7, and attempt 5 at t15
There was a problem hiding this comment.
Ack. I added a long comment here explaining everything.. LEt me know if something is unclear still..
...pentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/exporter.py
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Update
exporton OTLP / HTTP exporters to include a timeout param. Maketimeoutencompass retries, rather than being applied per-request.Update the grpc exporter to us the
retryconfig, which letsgrpchandle retries / timeout automatically based on our config, so we can delete a lot of code.There is no equivalent for HTTP (there is a
urlilib3.RetryConfig, but it's retries are not inclusive oftimeout- instead each retry gets passed the same timeout), so I manually updated the HTTP exporters.I also cleaned up the HTTP exporter code a tiny bit.
#4183 -- similar to this PR and what's discussed in #4043, but I implemented it in as minimal a way as I could..
I updated the
LogExporterandSpanExporterinterfaces to includetimeout_millis(MetricExporteralready has it), this isn't a breaking change because@abstractmethodjust checks that classes implementin have a method with a particular name, it doesn't check method parameters at all.. It does cause a pylint error, but I think that's a good thing.In future PRs I plan to update
shutdownandforceFlushto pass their timeouts along to export.. We could potentially pass these env var timeouts along to all otherexportcalls too..Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Lots of unit tests.
Does This PR Require a Contrib Repo Change?
Checklist: