Repository navigation
feat: deduplicate events, add logEvent, and allow turning off feature usage events - #181
Conversation
andywhite37
left a comment
andywhite37
left a comment
There was a problem hiding this comment.
Reusing a mutable properties map can change previously recorded custom events. All 337 existing tests pass serially and Dart analysis is clean; a targeted test reproduces the failure. Tests used updated dependency resolutions; live ingestion and native lifecycle delivery weren’t tested. The #180 findings are inherited here. No manual testing suggested for this directly reproduced failure.
| // An absent body is an empty object, not null: the ingestor reads | ||
| // `properties_json` as a record, and a null there is a different shape | ||
| // from "this event carried no fields". | ||
| properties: properties ?? const {}, |
There was a problem hiding this comment.
Perceived severity: Medium · Recommended before merge
Queued events retain the caller’s properties map. Reusing it appears to overwrite earlier event data before delivery.
Evidence: Recording purchase totals 10, then 20, using the same map produced outgoing totals [20, 20] instead of [10, 20].
Suggested fix: Snapshot custom-event properties when enqueueing, and test consecutive events sharing a mutable map.
There was a problem hiding this comment.
Fixed by rebasing onto the #180 fixes, which encode each event to JSON at enqueue time. Custom event properties are therefore frozen together with the attributes, nested values included. While resolving the conflict, the queue started holding (json, dedupeKey) pairs so that a failed batch can still return its dedupe keys. The dedupe check runs before encoding, and if encoding fails the key is released. Added a test that reuses one mutable properties map for two logEvent calls (totals 10 then 20, plus a nested list) and asserts [10, 20]. On the previous head it fails with [20, 20]. 341/341 tests pass and dart analyze is clean.
Three gaps in the tracking plugin compared with the JS SDK, reported in Deduplication. Every evalFeature/isOn call queued a Feature Evaluated event, so a flag read on every screen produced the same event over and over — bandwidth spent telling the ingestor nothing it did not already have. The key is the event name plus the event's own fields, deliberately not the attributes: a feature evaluation carries no unit identity of its own, so including them would make every attribute change look like a new event. Keys are sorted recursively before encoding, because two encodings of the same evaluation differing only in key order would never match; anything that fails to encode yields no key and is left undeduplicated rather than dropped. Dart's Map is insertion-ordered, so the cache is a plain Map — removing and re-inserting moves a key to the newest position and `keys.first` is the oldest to evict. The intrusive linked list the Swift SDK needs for that ordering is unnecessary here. A batch that fails to send gives its keys back. They are recorded when the events are accepted, before anything is known about whether they will arrive, so without the release the evaluations a dropped connection carried would stay suppressed — and because re-evaluating one refreshes its place in the cache, they would never age out either. logEvent. Apps can now record events of their own through the same envelope as the SDK's own events, carrying the current attributes, so the ingestor needs no special case for them. These are never deduplicated: the app chose when to send each one, so a repeat is a second occurrence rather than a redundant re-evaluation. They are dropped once the registry is closed, like everything else. enableFeatureUsageEvents. Feature evaluations are by far the larger volume; an app that only needs experiment data can drop them while keeping exposures and custom events. The new plugin callback defaults to a no-op so existing custom plugins keep compiling.
The shared experiment helper in the plugin suite now returns a fresh key per call. Those tests exercise batching and flushing, and with dedupe in place a repeated key would be suppressed rather than batched — they would have been testing the cache by accident. Covers suppression of an identical evaluation, a changed value still being sent, attributes staying out of the key, nested key order not mattering, a cache size of zero disabling dedupe, eviction of the oldest key, and a failed batch releasing its keys so the event is retried. For custom events: never deduplicated, carried in the same envelope with identity promotion, an absent body sent as an empty object, dropped after close, and reaching plugins from the SDK with the current attributes. The usage switch is covered in both directions — feature evaluations dropped, exposures and custom events untouched.
Each needs a reason, not just a name: why the dedupe key excludes attributes, that a failed batch gives its keys back, that custom events are exempt from dedupe, and that feature evaluations are the volume an app can drop while keeping its exposures.
9a6bad2 to
bf4fb87
Compare
andywhite37
added
effort:medium
andywhite37
left a comment
andywhite37
left a comment
There was a problem hiding this comment.
The earlier properties-map finding is fixed and covered by a new test; all 341 tests pass serially and Dart analysis is clean. One optional docs fix. Tests used updated dependency resolutions; live ingestion and native lifecycle delivery weren't tested.
| evaluations are sent. The key is the event name plus its own fields — | ||
| attributes are deliberately excluded, since a feature evaluation carries no | ||
| unit identity of its own and every attribute change would otherwise look like a | ||
| new event. A batch that fails to send gives its keys back, so nothing is lost |
There was a problem hiding this comment.
"Nothing is lost to a dropped connection" overstates the behavior: a failed batch is logged and dropped, and only its dedupe keys are released for later evaluations.
Suggested fix: Say that a failed batch's events are not retried but later identical evaluations are sent again.
Perceived severity: Low · Perceived priority: Optional
andywhite37
changed the title
Adds the three features missing from
GrowthBookTrackingPlugin, reported in #179. Mirrors growthbook-swift#195, minus the Objective-C bridge, which has no Dart equivalent.Deduplication
Every
evalFeature/isOncall queued a Feature Evaluated event, so a flag read on every screen produced the same event over and over — bandwidth spent telling the ingestor nothing it did not already have. The plugin now keeps anLRU of the last 1,000 events (
dedupeCacheSize,0disables it) and drops the repeats, as the JS and Kotlin SDKs do.The key is the event name plus the event's own fields, deliberately not the attributes: a feature evaluation carries no unit identity of its own, so including them would make every attribute change look like a new event. Keys are sorted recursively before encoding, because two encodings of the same evaluation differing only in key order would never match. Anything that fails to encode yields no key and is left undeduplicated rather than dropped.
Dart's
Mapis insertion-ordered, so the cache is a plainMap— removing and re-inserting moves a key to the newest position, andkeys.firstis the oldest to evict. The intrusive linked list the Swift SDK needs for that ordering is unnecessary here.A batch that fails to send gives its keys back. They are recorded when the events are accepted, before anything is known about whether they will arrive, so without the release the evaluations a dropped connection carried would stay
suppressed — and because re-evaluating one refreshes its place in the cache, they would never age out either.
logEventApps can record events of their own through the same envelope as the SDK's own events, carrying the current attributes, so the ingestor needs no special case for them:
These are never deduplicated — the app chose when to send each one, so a repeat is a second occurrence rather than a redundant re-evaluation — and they are dropped once the SDK is disposed, like every other event.
enableFeatureUsageEvents
Feature evaluations are by far the larger volume. An app that only needs experiment data can drop them while keeping exposures and custom events:
GrowthBookTrackingPluginConfig(enableFeatureUsageEvents: false)
Compatibility
The new logEvent callback on GrowthBookPlugin defaults to a no-op, so existing custom plugins keep compiling unchanged.
Worth noting for anyone reading raw event counts: deduplication means far fewer Feature Evaluated events arrive after this release for the same app behaviour.
That is the point, but a dashboard counting raw events will show a drop at the upgrade.
Compatibility
The new logEvent callback on GrowthBookPlugin defaults to a no-op, so
existing custom plugins keep compiling unchanged.
Worth noting for anyone reading raw event counts: deduplication means far fewer
Feature Evaluated events arrive after this release for the same app behaviour.
That is the point, but a dashboard counting raw events will show a drop at the
upgrade.
Testing
dart analyze and dart format clean. The plugin suite is 55 tests, green
three runs in a row.
Dedupe: an identical evaluation suppressed, a changed value still sent,
attributes staying out of the key, nested key order not mattering, a cache size
of zero disabling it, eviction of the oldest key, and a failed batch releasing
its keys so the event is retried.
Custom events: never deduplicated, carried in the same envelope with identity
promotion, an absent body sent as an empty object, dropped after close, and
reaching plugins from the SDK with the current attributes. The usage switch is
covered in both directions.
The shared experiment helper in the suite now returns a fresh key per call —
those tests exercise batching and flushing, and with dedupe in place a repeated
key would be suppressed rather than batched, so they would have been testing
the cache by accident.
Closes #179