-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: Make Amplitude publish fail gracefully #531
Conversation
CodSpeed Performance ReportMerging #531 will create unknown performance changesComparing Summary
|
5ffa9b4
to
968cb83
Compare
0424d86
to
6bf7a5b
Compare
error_message=str(e), | ||
), | ||
) | ||
|
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.
except Exception
is broad - every type of raised error will qualify. Can you make it more specific?
If not, it looks like you're swallowing the error (nothing is raised) - is this how you want it to work?
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.
This is the intention yes. The only code below this is the little logic in this file and the underlying amplitude library. We are adding the error details to the log and the counter, so in theory we can catch unexpected errors getting swallowed here.
The end goal of this PR is to make certain calling publish
will never error out so we can put it wherever we want with 0 risk of interrupting critical flows.
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.
I see, that makes sense. awesome!
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.
left some thoughts, other than that lgtm!
This PR adds an
_unsafe_publish
method to the Amplitude event publisher, converting the existingpublish
method to be safe by default (i.e., it will always fail gracefully).Also adds prometheus metrics to track success and failures of amplitude publish calls.