Skip to content
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

Merged
merged 7 commits into from
Feb 26, 2025

Conversation

spalmurray-codecov
Copy link
Contributor

@spalmurray-codecov spalmurray-codecov commented Feb 24, 2025

This PR adds an _unsafe_publish method to the Amplitude event publisher, converting the existing publish 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.

@spalmurray-codecov spalmurray-codecov marked this pull request as ready for review February 24, 2025 18:39
Copy link

codspeed-hq bot commented Feb 24, 2025

CodSpeed Performance Report

Merging #531 will create unknown performance changes

Comparing spalmurray/safe-amplitude (c8494ae) with main (0222015)

Summary

⚠️ No benchmarks were detected in both the base of the PR and the PR.\

@spalmurray-codecov spalmurray-codecov force-pushed the spalmurray/safe-amplitude branch 2 times, most recently from 5ffa9b4 to 968cb83 Compare February 25, 2025 14:57
error_message=str(e),
),
)

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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!

Copy link
Contributor

@nora-codecov nora-codecov left a 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!

@spalmurray-codecov spalmurray-codecov added this pull request to the merge queue Feb 26, 2025
Merged via the queue into main with commit 4de8f9e Feb 26, 2025
8 checks passed
@spalmurray-codecov spalmurray-codecov deleted the spalmurray/safe-amplitude branch February 26, 2025 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants