8000 Make exporter `timeout` encompass retries/backoffs, add jitter to backoffs, cleanup code a bit by DylanRussell · Pull Request #4564 · open-telemetry/opentelemetry-python · GitHub
[go: up one dir, main page]
Skip to content

Make exporter timeout encompass retries/backoffs, add jitter to backoffs, cleanup code a bit #4564

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

Make exporter timeout encompass retries/backoffs, add jitter to backoffs, cleanup code a bit #4564

wants to merge 28 commits into from
+435 −513

Conversation

DylanRussell
Copy link
Contributor
@DylanRussell DylanRussell commented Apr 30, 2025 •
edited
Loading

Description

Make timeout encompass retries and backoffs, rather than being applied per HTTP request or gRPC RPC.

Added a +/- 20% jitter to each backoff (both gRPC/HTTP).

Cleanup up the exporter code some. I got rid of a pointless 32 second sleep we would do after our last retry attempt before failing.

fixes: #3309, #4043, #2663

#4183 -- similar to this PR and what's discussed in #4043, but I implemented it in as minimal a way as I could..

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • [ x] New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Lots of unit tests.

Does This PR Require a Contrib Repo Change?

  • Yes. - Link to PR:
  • [x ] No.

Checklist:

  • [ x] Followed the style guidelines of this project
  • [ x] Changelogs have been updated
  • [x ] Unit tests have been added
  • [x ] Documentation has been updated

@DylanRussell DylanRussell requested a review from a team as a code owner April 30, 2025 18:43
@DylanRussell DylanRussell mentioned this pull request Apr 30, 2025
Closed
3 tasks
emdneto
emdneto reviewed May 9, 2025
View reviewed changes
Copy link
Member
@emdneto emdneto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

aabmass
aabmass reviewed May 14, 2025
View reviewed changes
Copy link
Member
@aabmass aabmass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can add the description which issues that are fixed by this PR? https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue

...pentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/exporter.py Outdated
"methodConfig": [
{
"name": [dict()],
"retryPolicy": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this correctly implement the OTLP spec? https://github.com/open-telemetry/opentelemetry-proto/blob/main/docs/specification.md#failures

I'm wondering about RetryInfo and partial errors in particular

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't realize RetryInfo was part of OTLP spec. gRPC retry policy does not make use of RetryInfo, and it isn't mentioned in https://github.com/grpc/proposal/blob/master/A6-client-retries.md or https://grpc.io/docs/guides/retry/#retry-configuration. My reading of the partial success part is that the status code is still OK in that case, which this does handle correctly (by not retrying).

So you think we should remove this then ? I feel it's nice to let the gRPC library handle this stuff instead of hand rolling our own code.. If gRPC changes something around retries I'm assuming they'd update their own retry handling code and we wouldn't have to worry about it.

The gFC (https://github.com/grpc/proposal/blob/master/A6-client-retries.md) also mentions how the retryPolicy can be used for hedging and respect server pushback, but neither of those appear to be implemented yet

@aabmass
Copy link
Member
aabmass commented May 14, 2025

I updated the LogExporter and SpanExporter interfaces to include timeout_millis (MetricExporter already has it), this isn't a breaking change because @abstractmethod just 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.

I didn't see this in the code, if you updated can you just update the description as well?

@DylanRussell
Copy link
Contributor Author

Updated the description

@DylanRussell DylanRussell mentioned this pull request May 16, 2025
Open
@DylanRussell DylanRussell changed the title Add a timeout param to all OTLP grpc / http export calls -- fixed merge conflicts Switch gRPC exporters to use official gRPC retry config. Make timeout encompass retries/backoffs May 22, 2025
@DylanRussell
Copy link
Contributor Author
DylanRussell commented May 29, 2025 •
edited
Loading

Nevermind, not going to use gRPC retry config for now..

@DylanRussell DylanRussell changed the title Switch gRPC exporters to use official gRPC retry config. Make timeout encompass retries/backoffs Make exporter timeout encompass retries/backoffs, add jitter to backoffs, cleanup code a bit Jun 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aabmass aabmass aabmass left review comments

@emdneto emdneto emdneto approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Exporters shutdown takes longer then a minute when failing to send metrics/traces
3 participants
@DylanRussell @aabmass @emdneto
0