-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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(otlp): Write protobuf status on error #15097
Conversation
11c1813
to
a12cf8c
Compare
} | ||
|
||
// otelErrorHeaderInterceptor maps 500 errors to 503. | ||
// According to the OTLP specification, 500 errors are never retried on the client side, but 503 are. | ||
type otelErrorHeaderInterceptor struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic moved into new OTLPError
writer
@@ -16,17 +16,19 @@ import ( | |||
|
|||
"github.com/dustin/go-humanize" | |||
"github.com/go-kit/log" | |||
"github.com/gogo/protobuf/proto" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be gogo proto? i am not sure how different this is from the google version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately not. I tried but panics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
(cherry picked from commit 63a2442)
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-15097-to-k229 origin/k229
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 63a2442191751e32aaafd6227e1602dfa3a95caa When the conflicts are resolved, stage and commit the changes:
If you have the GitHub CLI installed: # Push the branch to GitHub:
git push --set-upstream origin backport-15097-to-k229
# Create the PR body template
PR_BODY=$(gh pr view 15097 --json body --template 'Backport 63a2442191751e32aaafd6227e1602dfa3a95caa from #15097{{ "\n\n---\n\n" }}{{ index . "body" }}')
# Create the PR on GitHub
echo "${PR_BODY}" | gh pr create --title 'fix(otlp): Write protobuf status on error (backport k229)' --body-file - --label 'size/L' --label 'type/bug' --label 'backport' --base k229 --milestone k229 --web Or, if you don't have the GitHub CLI installed (we recommend you install it!): # Push the branch to GitHub:
git push --set-upstream origin backport-15097-to-k229
# Create a pull request where the `base` branch is `k229` and the `compare`/`head` branch is `backport-15097-to-k229`.
# Remove the local backport branch
git switch main
git branch -D backport-15097-to-k229 |
(cherry picked from commit 63a2442)
What this PR does / why we need it:
The OTLP spec states that:
Loki currently writes the error as a string so it's lost. This PR writes the error as a protobuf Status instead.
We also ditch the otlp error interceptor and put it's logic inside the OTELError writer.
Without change Error message is lost
With change Error message is displayed (set
max_line_size: 1B
)Special notes for your reviewer:
Checklist
CONTRIBUTING.md
guide (required)feat
PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.md
deprecated-config.yaml
anddeleted-config.yaml
files respectively in thetools/deprecated-config-checker
directory. Example PR