-
Notifications
You must be signed in to change notification settings - Fork 486
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
Chore: Fixes error handling in log exporter #6262
base: main
Are you sure you want to change the base?
Chore: Fixes error handling in log exporter #6262
Conversation
@domasx2 I want to double check this doesnt break anything for you all, this causes it to fail on the first exception instead of trying to process everything. |
Hey, sorry, missed the notification! I'm gonna pass this to @rlankfo though for a look. Not sure how the cliend endpoint deals with this? |
Based on @domasx2 comments, @vanugrah using the multierrors might be a better solution here. Lets you build an array of errors and then return. We use this library already in quite a few places. |
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! I'd rather see these as if err := func(...); err != nil {
but either way works. 👍
though let Robbie approve before merging I haven't worked in this repo yet.
Also +1 for the multierrors suggestion based on what others are saying. Seems like this does actually have maybe some unintended consequences so I'll withdraw my approval for now so others have a chance to look.
I agree with @mattdurham that we should use |
Hey folks - apologies for the delay. Totally support the use of multierrors. Will try to update this PR over the weekend. |
This PR has not had any activity in the past 30 days, so the |
This PR has not had any activity in the past 30 days, so the |
PR Description
Which issue(s) this PR fixes
The error handling in log_exporter.go for the app_agent_receiver is currently implemented in a way that the err variable gets overridden - hence the only time it will return an error is if there is an issue processing the last event.
This PR addresses introduces error handling to return if any error is encountered during the processing of logs/measurements/exceptions/events by the logs_exporter.
Notes to the Reviewer
I'm not entirely sure what the thought process of the original author was - but my guess is they wanted to do all the processing and only return errors at the end. If that is the case, this PR should be modified to adhere to that intent.
PR Checklist