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

Fix IsPermanent to account for wrapped errors #2455

Merged
merged 2 commits into from
Feb 13, 2021

Conversation

joe-elliott
Copy link
Contributor

Description:
Currently IsPermanent does not correctly test for wrapped errors. We noticed this when the collector was incorrectly retrying otlp grpc FailedPrecondition status:

level=error msg="Exporting failed. No more retries left. Dropping data." component=tempo component_kind=exporter component_type=otlp component_name=otlp error="max elapsed time expired failed to push trace data via OTLP exporter: Permanent error: fatal error sending to server" dropped_items=100

This occurs b/c the error is wrapped and the IsPermanent function test does not correctly detect wrapped errors.

Error wrapping:

https://github.com/open-telemetry/opentelemetry-collector/blob/main/exporter/otlpexporter/otlp.go#L79

Go Playground with examples:

https://play.golang.org/p/kVBm3SesDBu

Testing:
Added a simple test to detect a wrapped error.

@joe-elliott joe-elliott requested a review from a team February 10, 2021 15:03
@joe-elliott joe-elliott changed the title Fix IsPermanent test to account for wrapped errors Fix IsPermanent to account for wrapped errors Feb 10, 2021
@codecov
Copy link

codecov bot commented Feb 10, 2021

Codecov Report

Merging #2455 (4cb6d71) into main (5cc0707) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2455      +/-   ##
==========================================
- Coverage   91.76%   91.76%   -0.01%     
==========================================
  Files         266      266              
  Lines       15112    15111       -1     
==========================================
- Hits        13867    13866       -1     
  Misses        867      867              
  Partials      378      378              
Impacted Files Coverage Δ
consumer/consumererror/permanenterror.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5cc0707...4cb6d71. Read the comment docs.

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

I ran into this and was preparing a fix as well. The OTLP exporter potentially rewraps permanent errors, which end up getting retried, for example:

return md.MetricCount(), fmt.Errorf("failed to push metrics data via OTLP exporter: %w", err)

Can confirm that this fixes that issue.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

@bogdandrutu
Copy link
Member

@joe-elliott please fix the tests, I tried to re-run them multiple times and same error. Maybe a rebase would help.

joe-elliott and others added 2 commits February 13, 2021 00:17
Signed-off-by: Joe Elliott <number101010@gmail.com>
Co-authored-by: Matthew Wear <matthew.wear@gmail.com>
@joe-elliott
Copy link
Contributor Author

Wow, yeah. I was way further behind than I realized. Rebased and pushed.

This was referenced Mar 15, 2021
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
Bumps [github.com/knadh/koanf](https://github.com/knadh/koanf) from 1.4.4 to 1.4.5.
- [Release notes](https://github.com/knadh/koanf/releases)
- [Commits](knadh/koanf@v1.4.4...v1.4.5)

---
updated-dependencies:
- dependency-name: github.com/knadh/koanf
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024
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.

6 participants