Repository navigation
fix: report event batch serialization failures - #78
Open
Shubham-Padkonde wants to merge 1 commit into
Open
Shubham-Padkonde wants to merge 1 commit into
Shubham-Padkonde wants to merge 1 commit into
Conversation
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the missing error notification in #59. When event properties added after construction cannot be serialized (for example, a
datetimeadded by enrichment), the worker raises inside an unobserved background future. The batch is not sent, but neither the configured logger nor callbacks receive the failure.Catch JSON serialization
TypeError/ValueErrorat the send boundary, log the failure, and notify client/event callbacks for the unsent batch with code 400 and the serialization error. Re-raise the original exception so callers that await a flush future retain the existing error behavior. The batch is not retried and no HTTP request is made.This does not change constructor property validation or convert unsupported values automatically. On current main, invalid properties passed directly to the constructor are filtered; the regressions exercise properties modified afterward.
Validation
All 117 tests pass on Windows/Python 3.13 and Linux/Python 3.12 using
python -m unittest discover -s ./src -p 'test_*.py'. Three regression cases fail before the change because no error is logged. Coverage includes datetime/bytes payload failures, client and event callbacks, a mixed batch, no HTTP request, and preservation of the flush future's exception. No live Amplitude events were sent.Checklist
Prepared with Codex assistance for implementation, tests, and validation.
Note
Low Risk
Change is confined to the send path on serialization failure and mainly adds logging and callbacks where failures were previously invisible.
Overview
Fixes silent failures when a batch cannot be JSON-serialized (e.g. a
datetimeorbytesslipped intoevent_propertiesafter construction/enrichment).Workers.sendnow catchesTypeError/ValueErrorfrom payload building, logs an ERROR, invokesresponse_processor.callbackfor every event in the batch with status 400 and a clear message, then re-raises soflush().result()still surfaces the same exception. No HTTP POST is attempted and retries are unchanged.Adds regression tests covering mixed batches, client/event callbacks, logging, skipped
HttpClient.post, and flush future behavior.Reviewed by Cursor Bugbot for commit 7cc2513. Bugbot is set up for automated code reviews on this repo. Configure here.