fix(analytics-core): drop events with an empty event type before upload - #1920
fix(analytics-core): drop events with an empty event type before upload#1920Mercy811 wants to merge 1 commit into
Conversation
Events with a blank event type are rejected by the event server with a 400, which also forces the rest of their upload batch to be retried. 99.6% of dropped Browser SDK events have a blank event name. Validate client-side in AmplitudeCore.process() instead: events whose event_type is empty, whitespace-only, or not a string are dropped with a warning log before reaching the timeline or any destination. Mirrors the same check added to the Kotlin SDK in amplitude/Amplitude-Kotlin#444. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a72764b815
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
bugbot run |
size-limit report 📦
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit a72764b. Configure here.
| // upload batch to be retried, so drop it before it ever reaches a destination. | ||
| const eventType: unknown = event.event_type; | ||
| if (typeof eventType !== 'string' || eventType.trim().length === 0) { | ||
| this.config.loggerProvider.warn(EMPTY_EVENT_TYPE_MESSAGE); |
There was a problem hiding this comment.
Should this be a warning or an error? I feel like this justifies an "error" because it's something customers will want surfaced quickly so they can take corrective action right away.
3 participants
Summary
SDKW-50 — 99.6% of dropped Browser SDK events have a blank event name and get 400d by the event server (Slack thread). Beyond being pointless traffic, a blank event type in a batch forces the rest of that batch to be retried.
This adds the same client-side validation the Kotlin SDK just picked up in amplitude/Amplitude-Kotlin#444: drop the event before upload, with a warning log.
Where:
AmplitudeCore.process()(packages/analytics-core/src/core-client.ts), right after the existingoptOutcheck — the direct analogue of Kotlin'sAmplitude.process(). This is the single funnel into the timeline: bothdispatch()anddispatchWithCallback()route through it, so the check coverstrack/logEvent,identify,groupIdentify,setGroup, andrevenuefor every SDK built on core (browser, node, react-native).What counts as empty:
event_typethat is empty, whitespace-only, or not a string. Kotlin'sisBlank()covers empty + whitespace; thetypeofguard is the JS-specific part —track({})ortrack(undefined)reachesprocess()withevent_type === undefined, sincecreateTrackEventpasses object input straight through.Dropped events return
buildResult(event, 0, EMPTY_EVENT_TYPE_MESSAGE), matching howoptOutandCLIENT_NOT_INITIALIZEDalready report skipped events, so callers awaitingtrack()get a result rather than a hang, and the promise message matches the logged warning.Not covered (deliberate, matches Kotlin): the destination plugin's
setup()restores unsent events from storage and callsexecute()directly, bypassingprocess(). Blank events persisted by older SDK versions will still be uploaded once — but a 400 lists them ineventsWithInvalidFields, sohandleInvalidResponsedrops them instead of retrying forever. Not worth a second gate.Testing
New tests in
packages/analytics-core/test/core-client.test.tsunderdescribe('process'), mirroring the Kotlin test's shape (dropped event never reaches the pipeline; a valid event still flows):test.eachover empty / whitespace-only / missingevent_type— assertstimeline.pushis never called, the result isEMPTY_EVENT_TYPE_MESSAGEwith code 0, and exactly one warning is logged.event_typestill reachestimeline.pushwith no warning.Verified locally:
analytics-core: 873 tests pass, 100% statements/branches/functions/lines (the package's enforced threshold).analytics-browser: 499 tests pass, 100% coverage.pnpm test: all 29 projects pass.pnpm lint(0 errors) andpnpm typecheckclean onanalytics-core.Checklist
track()promise resolves with code 0 instead of 400.🤖 Generated with Claude Code
Note
Low Risk
Narrow validation in the shared event funnel; no API or auth changes, though callers now get a local code-0 skip instead of a failed upload for blank event names.
Overview
Client-side guard in
AmplitudeCore.process()drops events whoseevent_typeis missing, not a string, or only whitespace—right after the existingoptOutcheck and beforetimeline.push. That blocks doomed uploads that 400 the event server and can force batch retries.Dropped events follow the same skip pattern as opt-out:
buildResultwith code0, messageEMPTY_EVENT_TYPE_MESSAGE, and a warning on the logger. Valid event types still go through the pipeline unchanged.Tests cover empty, whitespace-only, and undefined
event_type(nopush, correct result/warn) plus a happy path that still callspush.Reviewed by Cursor Bugbot for commit a72764b. Configure here.