Skip to content
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

OTLP exporter: Let final retry error include last retryable error message #3514

Merged
merged 20 commits into from
Jan 4, 2023

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Dec 5, 2022

This improves the error messages printed when an OTel-Go SDK experiences retryable errors that eventually time out. Significantly, this includes text from the final retryable error to help the user understand the why the timeout has been reached.

I briefly updates the OTel collector example main() to avoid blocking in dial, to demonstrate the problem. After this change:

2022/12/05 11:14:10 context deadline exceeded: rpc error: code = Unavailable desc = connection error: desc = "transport: Error while dialing dial tcp 127.0.0.1:4318: connect: connection refused"
2022/12/05 11:14:20 context deadline exceeded: rpc error: code = Unavailable desc = connection error: desc = "transport: Error while dialing dial tcp 127.0.0.1:4318: connect: connection refused"

Before this change:

2022/12/05 11:14:10 context deadline exceeded
2022/12/05 11:14:20 context deadline exceeded

Part of #3513.

@jmacd jmacd changed the title OTLP exporter: Let final retry error return the first retryable error OTLP exporter: Let final retry error include last retryable error message Dec 5, 2022
Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

lgtm, but probably needs a changelog

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
exporters/otlp/internal/retry/retry.go Outdated Show resolved Hide resolved
exporters/otlp/internal/retry/retry_test.go Outdated Show resolved Hide resolved
exporters/otlp/internal/retry/retry_test.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
exporters/otlp/internal/retry/retry.go Outdated Show resolved Hide resolved
exporters/otlp/internal/retry/retry_test.go Outdated Show resolved Hide resolved
@jmacd
Copy link
Contributor Author

jmacd commented Jan 4, 2023

Thanks for the help @MadVikingGod.

@codecov
Copy link

codecov bot commented Jan 4, 2023

Codecov Report

Merging #3514 (5d5859a) into main (4607516) will not change coverage.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #3514   +/-   ##
=====================================
  Coverage   77.9%   77.9%           
=====================================
  Files        163     163           
  Lines      11850   11850           
=====================================
  Hits        9235    9235           
  Misses      2417    2417           
  Partials     198     198           
Impacted Files Coverage Δ
exporters/otlp/internal/retry/retry.go 98.2% <100.0%> (ø)
sdk/trace/batch_span_processor.go 80.2% <0.0%> (-1.8%) ⬇️
exporters/jaeger/jaeger.go 91.1% <0.0%> (+0.8%) ⬆️
exporters/otlp/otlptrace/otlptracehttp/client.go 77.5% <0.0%> (+1.0%) ⬆️

@MrAlias MrAlias added this to the Release v1.12.0 milestone Jan 4, 2023
@MrAlias MrAlias merged commit efd8a7d into open-telemetry:main Jan 4, 2023
@MrAlias MrAlias mentioned this pull request Jan 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants