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

Add support to handle 402, 413, 414, 431 http error code as permanent errors in OTLP exporter #5674 #5685

Merged
merged 12 commits into from
Jul 25, 2022

Conversation

mcmho
Copy link
Contributor

@mcmho mcmho commented Jul 14, 2022

Description:
Collector currently does retries 402 and 413 error code. 413 is the error code for PAYLOAD TOO LARGE
and 402 is for PAYMENT REQUIRED which a user case of this would be when license token is exhausted.
These error code should be treated as permanent error in the Collector so that they won't get retried.

Link to tracking Issue:
#5674

Testing:
oltp_test.go

Documentation:
if we currently document http error code behavior, then treating 402 and 413 as permanent failure can be documented

…en-telemetry#5674

 - added StatusRequestEntityTooLarge (413) and StatusPaymentRequired (402) as failure permanent
@mcmho mcmho requested review from a team and bogdandrutu July 14, 2022 06:14
@pavankrish123
Copy link
Contributor

pavankrish123 commented Jul 14, 2022

@mcmho please add the appropriate unit test cases and CHANGELOG entry

@mcmho
Copy link
Contributor Author

mcmho commented Jul 14, 2022

@mcmho please add the appropriate unit test cases and CHANGELOG entry

okay. will do.

@mcmho
Copy link
Contributor Author

mcmho commented Jul 17, 2022

unit test is there. please help review
cc @bogdandrutu @pavankrish123

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

LGTM

@jpkrohling
Copy link
Member

You also need a changelog entry for this. I don't think this would qualify as a breaking change but as an enhancement.

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Juraci Paixão Kröhling <[email protected]>
@jpkrohling
Copy link
Member

Before approving, can you make sure this PR has all the cases that #5705 had?

@mcmho mcmho changed the title Add support to handle 402 and 413 http error code in Otlp exporter #5674 Add support to handle 402, 413, 414, 431 http error code in Otlp exporter #5674 Jul 19, 2022
@mcmho mcmho changed the title Add support to handle 402, 413, 414, 431 http error code in Otlp exporter #5674 Add support to handle 402, 413, 414, 431 http error code in OTLP exporter #5674 Jul 19, 2022
@mcmho mcmho changed the title Add support to handle 402, 413, 414, 431 http error code in OTLP exporter #5674 Add support to handle 402, 413, 414, 431 http error code as permanent errors in OTLP exporter #5674 Jul 19, 2022
… errors in OTLP exporter open-telemetry#5674

 - added 414 and 431 as permanent errors
@mcmho
Copy link
Contributor Author

mcmho commented Jul 19, 2022

Before approving, can you make sure this PR has all the cases that #5705 had?

Sure, it's done.

@mcmho
Copy link
Contributor Author

mcmho commented Jul 20, 2022

@bogdandrutu can you approve?

@codecov
Copy link

codecov bot commented Jul 20, 2022

Codecov Report

Merging #5685 (13a8223) into main (389c047) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5685      +/-   ##
==========================================
- Coverage   91.56%   91.54%   -0.03%     
==========================================
  Files         192      192              
  Lines       11411    11426      +15     
==========================================
+ Hits        10449    10460      +11     
- Misses        768      771       +3     
- Partials      194      195       +1     
Impacted Files Coverage Δ
exporter/otlphttpexporter/otlp.go 83.84% <100.00%> (+2.10%) ⬆️
pdata/internal/common.go 91.85% <0.00%> (-0.75%) ⬇️

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 389c047...13a8223. Read the comment docs.

exporter/otlphttpexporter/otlp.go Outdated Show resolved Hide resolved
exporter/otlphttpexporter/otlp.go Outdated Show resolved Hide resolved
… errors in OTLP exporter open-telemetry#5674

 - fixed according to code review
… errors in OTLP exporter open-telemetry#5674

 - fixed build-and-test  golangci-lint run
@mcmho
Copy link
Contributor Author

mcmho commented Jul 22, 2022

Looks good now? @bogdandrutu

@mcmho
Copy link
Contributor Author

mcmho commented Jul 22, 2022

I think i have done everything now. thanks @bogdandrutu @jpkrohling @TylerHelmuth for your approval!
Let's wait for the merge is ready.

@mcmho
Copy link
Contributor Author

mcmho commented Jul 23, 2022

"First-time contributors need a maintainer to approve running workflows.
6 expected and 1 successful checks"

cc @jpkrohling @bogdandrutu @TylerHelmuth

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