-
Notifications
You must be signed in to change notification settings - Fork 81
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 additional transaction labels with error details to requests. #3124
Conversation
59d93c9
to
865623a
Compare
ea02fff
to
a3c7031
Compare
trans.Context.SetLabel("error.type", "ErrElastic") | ||
trans.Context.SetLabel("error.details.status", esErr.Status) | ||
trans.Context.SetLabel("error.details.type", esErr.Type) | ||
trans.Context.SetLabel("error.details.reason", esErr.Reason) | ||
trans.Context.SetLabel("error.details.cause.type", esErr.Cause.Type) | ||
trans.Context.SetLabel("error.details.cause.reason", esErr.Cause.Reason) |
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.
@joshdover is this closer to what you had in mind (in the issue description)?
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.
Yes this makes sense to me. One thing I can't tell from just reading this is if any of these fields contain anything instance-specific content that will create noise and make it harder to group related errors, such as specific disk usage stats, node names, etc. This would probably be tricky to normalize though at this point and I think we can do this at query time later if it's an issue.
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.
ok, we'll merge as is for now and we can revist/clean up the labels in the future
trans.Context.SetLabel("error.type", "ErrElastic") | ||
trans.Context.SetLabel("error.details.status", esErr.Status) | ||
trans.Context.SetLabel("error.details.type", esErr.Type) | ||
trans.Context.SetLabel("error.details.reason", esErr.Reason) | ||
trans.Context.SetLabel("error.details.cause.type", esErr.Cause.Type) | ||
trans.Context.SetLabel("error.details.cause.reason", esErr.Cause.Reason) |
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.
Yes this makes sense to me. One thing I can't tell from just reading this is if any of these fields contain anything instance-specific content that will create noise and make it harder to group related errors, such as specific disk usage stats, node names, etc. This would probably be tricky to normalize though at this point and I think we can do this at query time later if it's an issue.
SonarQube Quality Gate |
What is the problem this PR solves?
It's difficult to determine if a fleet-server error is caused by a downstream elasticsearch error, or it indicates an actual issue (fleet-server misconfiguration etc.)
How does this PR solve the problem?
Add transaction labels with more error details when a request would send an APM error.
Checklist
I have made corresponding changes to the documentationI have made corresponding change to the default configuration files./changelog/fragments
using the changelog toolRelated issues