Return consumererror.Permanent in the Prometheus remote write exporter #1734
Conversation
huyan0
requested review from
bogdandrutu,
dmitryax,
james-bebbington,
nilebox,
owais,
pjanotti and
tigrannajaryan
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1734 +/- ##
==========================================
+ Coverage 92.39% 92.42% +0.03%
==========================================
Files 257 257
Lines 18184 18214 +30
==========================================
+ Hits 16801 16835 +34
+ Misses 969 967 -2
+ Partials 414 412 -2
Continue to review full report at Codecov.
|
| if httpResp.StatusCode/100 != 2 { | ||
| scanner := bufio.NewScanner(io.LimitReader(httpResp.Body, 256)) | ||
| line := "" | ||
| if scanner.Scan() { | ||
| line = scanner.Text() | ||
| } | ||
| errMsg := "server returned HTTP status " + httpResp.Status + ": " + line | ||
| return errors.New(errMsg) | ||
| if httpResp.StatusCode >= 500 && httpResp.StatusCode < 600 { | ||
| return errors.New(errMsg) |
There was a problem hiding this comment.
I think you want permanent in this case not the other, can you add test?
There was a problem hiding this comment.
I thought we should retry if its a 503 or other 500 errors. This is the Prometheus implementation I referenced.
I don't know how I can make the proto buf marshal fail or the construction of a http request return error.
There was a problem hiding this comment.
Ok, if you want to retry on 5xx add a comment to the code that this is expected based on link to prom implementation.
There was a problem hiding this comment.
Cool. I have added comments from 259 - 262
|
Please also rebase. |
| } | ||
|
|
||
| if dropped != 0 { | ||
| if permanent { |
There was a problem hiding this comment.
Instead of doing this hack here, can you consider to change componenterror.CombineErrors to do this? you just need to check the type and if any error is permanent return permanent?
Probably a separate PR :)
There was a problem hiding this comment.
yeah. I was hesitant on doing that because i'm not sure if its good to introduce the idea of "conusmer" in component error. I have the code already haha. Will file a PR tomorrow then.
There was a problem hiding this comment.
Components already depend on consumer. The other way around is not expected
There was a problem hiding this comment.
This needs now to be removed, the CombineErrors handles this.
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
According to the [community charter](https://github.com/open-telemetry/community/blob/main/community-membership.md#responsibilities-and-privileges-2): > Expected to be responsive to review requests (inactivity for more than > 1 month may result in suspension until active again) Given @paivagustavo has returned to an active state, this reinstates their status as an Approver.
This PR changes the Prometheus remote write to return error of consumererror.permanent type when a invalid metric is encountered or a non-5xx HTTP status code is received from the backend.
linked issue: resolves #1733
cc: @bogdandrutu @alolita @jmacd @huyan0