feat: add event enrichment client package - #7029
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7029 +/- ##
==========================================
+ Coverage 40.97% 40.98% +0.01%
==========================================
Files 121 121
Lines 8273 8273
Branches 3598 3598
==========================================
+ Hits 3390 3391 +1
+ Misses 4883 4882 -1 🚀 New features to boost your workflow:
|
|
Please make sure to generate and commit workflows using |
7bd20c5 to
4954624
Compare
4954624 to
b05a0ad
Compare
cadivus
left a comment
There was a problem hiding this comment.
Please add our LICENSE file. Also, a README is missing.
|
Please have a look at the pana package score (e.g. here: https://github.com/aws-amplify/amplify-flutter/actions/runs/28815090606/job/85452353135?pr=7029). It's a ogood indicator for misses. |
…ient - record() starts a new session when the current one is stopped, so a stopped session carrying a stop_timestamp is no longer reused for new events - close() clears the session after stopping it so no stale session stays readable - Flutter create() asserts appMetadata.appId matches appId when both are supplied - clarify autoSessionTracking, event_version, and arrival_timestamp docs and use analytics-envelope wording in comments
The log_cw_metric action derives a package's category by substring-matching its path against an allowlist. Add event_enrichment so the enrichment packages resolve a valid category instead of failing the metrics step.
- make the pure-Dart core web compatible by moving dart:io Platform access behind a conditional import (native io resolver + web stub); rename the default provider to PlatformDeviceMetadataProvider - surface record() failures through Result.error via a new EventEnrichmentRecordException instead of throwing - degrade gracefully in the Flutter create() when device metadata or client ID resolution fails - add LICENSE and README to both packages - align SDK/Flutter constraints and shared_preferences with the other packages
90d1c28 to
f357a12
Compare
Done. Generated workflows are committed and match current |
Went through the pana report and addressed what it flagged (license, README, platform/web support). |
Added both, to each package. |
…aught EventSink.send was synchronous, so any async work inside an implementation escaped the try/catch in EventEnrichmentClient.record and surfaced as an unhandled async error instead of an error Result. send now returns Future<void> and record awaits it inside the existing try/catch, matching how the sibling record-cache Sender types its transport calls. record therefore returns Future<Result<EnrichedEvent>>. Failures are still logged and converted to EventEnrichmentRecordException, so a throwing or rejecting sink never crashes the caller. Doc comments now state that contract explicitly.
…on the Flutter wrapper - clientIdProvider is now injectable on EventEnrichmentClientFlutter.create, mirroring deviceMetadataProvider. Adds a ClientIdProvider interface next to its default implementation, the same shape device_metadata_provider.dart uses, so callers have a type to substitute. - The wrapper now delegates the manual session controls (startSession, stopSession, handleAppPaused, handleAppResumed), and documents that with autoSessionTracking disabled the first record() lazily starts a session, so recording works without calling them. - Moves the SharedPreferences client-id key into a named constant and spells out why it must stay in lockstep with the Connect client and the native packages. - Propagates the async record signature through the wrapper.
|
Do we need - name: Event Enrichment
summary: amplify_event_enrichment
propagate: none
packages:
- amplify_event_enrichment
- amplify_event_enrichment_dart
in https://github.com/aws-amplify/amplify-flutter/blob/main/pubspec.yaml#L69? |
| sdk: ^3.11.0 | ||
|
|
||
| dependencies: | ||
| amplify_foundation_dart: ">=2.11.0 <2.12.0" |
There was a problem hiding this comment.
Bumped to >=2.12.1 <2.13.0.
| amplify_foundation_dart: ">=2.11.0 <2.12.0" | ||
| aws_common: ">=0.7.15 <0.8.0" | ||
| meta: ^1.16.0 | ||
| uuid: ^4.5.0 |
There was a problem hiding this comment.
| dependencies: | ||
| amplify_event_enrichment_dart: ">=0.1.0 <0.2.0" | ||
| amplify_foundation_dart: ">=2.11.0 <2.12.0" | ||
| flutter: | ||
| sdk: flutter | ||
| shared_preferences: ^2.0.15 | ||
| uuid: ^4.5.0 |
There was a problem hiding this comment.
Please update the amplify libraries to the latest and align the external libs with https://github.com/aws-amplify/amplify-flutter/blob/main/pubspec.yaml#L16
There was a problem hiding this comment.
Updated amplify_foundation_dart to >=2.12.1 <2.13.0 and amplify_lints to >=3.1.6 <3.2.0 in both packages, and uuid to ^4.5.1. Left shared_preferences at ^2.0.15 per your earlier comment. The new codegen deps (build_runner, json_serializable, json_annotation) match the root list too.
| /// still return an already-completed future (an `async` method body with no | ||
| /// `await` is enough). | ||
| /// {@endtemplate} | ||
| abstract interface class EventSink { |
There was a problem hiding this comment.
Should we use this naming? It will collide with dart:async (https://api.dart.dev/dart-async/EventSink-class.html)
There was a problem hiding this comment.
Renamed to Sender. It's the same abstraction amplify_record_cache_dart already exposes for kinesis and firehose, down to the sender constructor param, so we're not inventing a third name for it.
| /// Use [toJson] to produce the structured analytics envelope as a | ||
| /// JSON-compatible map. | ||
| /// {@endtemplate} |
There was a problem hiding this comment.
Shouldn't we act as described here: https://github.com/aws-amplify/amplify-flutter/blob/main/AGENTS.md#8-serialization
Instead, we implement everything from scratch here.
There was a problem hiding this comment.
Converted. The envelope isn't a field for field projection of EnrichedEvent, it regroups platform name/version, renames manufacturer to make, wraps the locale code, drops empty sections, and doesn't emit eventId at all. So annotating EnrichedEvent itself wouldn't produce the right shape. The envelope is modelled as its own set of private json_serializable classes and toJson() delegates to them.
I didn't use zAmplifySerializable since it lives in amplify_core, which this package doesn't depend on, and it doesn't set the snake_case renaming the envelope needs so we'd be overriding it everywhere anyway. Added a zEventEnrichmentSerializable in the package instead, same as how amplify_foundation_dart declares its own.
Output is unchanged, the existing envelope tests pass as written.
| @@ -0,0 +1,3 @@ | |||
| ## 0.1.0 | |||
|
|
|||
| - Initial release ([#7029](https://github.com/aws-amplify/amplify-flutter/pull/7029)) | |||
There was a problem hiding this comment.
I think the leading space is wrong here. All of our other CHANGELOGs don't have it
| @@ -0,0 +1,3 @@ | |||
| ## 0.1.0 | |||
|
|
|||
| - Initial release ([#7029](https://github.com/aws-amplify/amplify-flutter/pull/7029)) | |||
There was a problem hiding this comment.
Same as with the other CHANGELOG
| void handleAppResumed() { | ||
| switch (_state) { | ||
| case SessionState.paused: | ||
| _cancelTimer(); | ||
| _state = SessionState.active; | ||
| case SessionState.stopped: | ||
| startSession(); | ||
| case SessionState.active: | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Din't we have a bug here if the customer themselves stopped the session? We would continue that too.
There was a problem hiding this comment.
Yeah, that's a real bug, thank you. The manager tracks whether a stop was explicit now: resume after an explicit stop stays stopped, resume after a timeout still starts fresh, and record() still lazily starts a session after any stop. Tests cover all three, including through FlutterLifecycleObserver. Android has the same behavior in its SessionManager, following up there separately.
| void close() { | ||
| _closed = true; | ||
| // Stop the session to record its end, then drop it so no stale session is | ||
| // readable after close. | ||
| _sessionManager | ||
| ..stopSession() | ||
| ..clearSession(); | ||
| _logger.info('Client closed'); | ||
| } |
There was a problem hiding this comment.
Do we do something with the recorded data? Or will it be thrown away?
There was a problem hiding this comment.
Recording events is ok. record() hands the event to the Sender and awaits it, and a failure comes back as an error Result.
Session end data was being thrown away though. close() computed the stop timestamp and duration and then dropped them one line later, and a public stopSession() did the same. Fixed in this PR: session boundaries now emit _session.start and _session.stop events (same names legacy Analytics used, exposed as public constants) through full enrichment, with the stop carrying the ended session's id, timestamps and duration. Once per session on every path, with tests asserting exact counts.
One API consequence is that stopSession() and close() return Future<void> now, completing once the event reaches the sender. Failures are logged, never thrown. Android and Swift need the same change to stay on one envelope contract, following up on both.
|
The package scores are not good. See: Especially the first is bad. You can download the full reports from the artifacts and have a closer look. |
Bump `amplify_foundation_dart` to the current `>=2.12.1 <2.13.0` and `amplify_lints` to `>=3.1.6 <3.2.0`, matching what the kinesis packages and `amplify_core` already use, and move `uuid` to `^4.5.1` to match the root `pubspec.yaml` global version list. `shared_preferences` stays at `^2.0.15`, which is the constraint `amplify_push_notifications` uses.
The bullet on the initial release line was indented by one space, which no other CHANGELOG in the repo does.
`EventSink` collides with `dart:async`'s `EventSink`, so any file
importing both this package and `dart:async` unprefixed gets an ambiguous
import. `Sender` is the name the kinesis and firehose clients already use
for the same abstraction in `amplify_record_cache_dart`, and it matches
the `sender` constructor parameter and `_sender` field naming there too.
- `lib/src/event_sink.dart` -> `lib/src/sender.dart`
- `EventSink` -> `Sender`
- `EventEnrichmentClient({..., EventSink? sink})` -> `Sender? sender`
- `EventEnrichmentClientFlutter.create({..., EventSink? sink})` ->
`Sender? sender`
Breaking for anyone on the unreleased branch only; the package has not
shipped yet.
…rializable AGENTS.md section 8 prescribes `json_serializable` + `json_annotation` with shared options constants and generated `.g.dart` files, and the envelope was hand-rolling its map instead. The envelope is not a field-for-field projection of `EnrichedEvent` (it regroups platform name/version, renames `manufacturer` to `make`, wraps the locale code in an object, drops empty sections, and omits `eventId` entirely), so it is modelled as its own set of private generated classes that `EnrichedEvent.toJson()` builds and delegates to. Options live in `zEventEnrichmentSerializable` (snake_case field rename, `includeIfNull: false`, `explicitToJson: true`, no factory since the envelope is write-only), following the same pattern as `amplify_core`'s `zAmplifySerializable`. That constant itself is not reusable here: it lives in `amplify_core`, which this package does not and should not depend on, and it carries Cognito attribute converters and no field renaming. Output is unchanged. Verified byte-for-byte against the previous implementation across eight envelope shapes (fully populated, fully empty, platform name only, platform version only, locale/model without platform, attributes only, metrics only, partial app metadata), comparing both deep map equality and `jsonEncode` output so key order is pinned too. The existing envelope tests pass unmodified.
`handleAppResumed()` restarted tracking from any stopped state, and a stopped state reached by an explicit `stopSession()` was indistinguishable from one reached by the session timeout expiring. So an app that called `stopSession()` got a brand new session on the next foreground, undoing the stop it asked for. The manual controls exposed on the Flutter wrapper made this reachable without touching the session manager directly. `SessionManager` now tracks whether the stop was explicit. `stopSession()` and `clearSession()` set the flag, `startSession()` clears it, and the timeout path and `startSession()`'s implicit stop go through a private `_stop()` that leaves it alone, so: - resume after an explicit stop leaves tracking stopped - resume after a timeout stop starts a fresh session, unchanged - recording an event still lazily starts a session after any stop, unchanged, and re-enables lifecycle handling along with it Documented on `SessionManager`, the Dart client, and the Flutter wrapper's `stopSession` / `handleAppResumed`. amplify-android has the same behaviour in its `SessionManager`; that gets a follow-up PR.
…itted Three inaccuracies about where session data goes: - `close()`'s comment said it stopped the session "to record its end", but the stop timestamp and duration it computed were dropped by the `clearSession()` call on the next line. Nothing consumed them, so the `stopSession()` call is gone and the comment now explains what `clearSession()` actually does (cancels the pause timer, marks the stop explicit, drops the session). No observable behaviour change: the session was nulled immediately either way. - `stopSession` on both the client and the Flutter wrapper computes end-of-session data that no transport ever sees. Documented, along with why emitting a session-end event is a follow-up rather than a fix here: it changes the envelope contract shared with the other platforms. - `SessionManager.session`'s docstring claimed it was "null if stopped". It is not: a stopped session stays readable with its stop metadata, and only `clearSession()` nulls it.
Nothing in the wrapper package or its tests imports mocktail; the tests use hand-written fakes.
Session stop timestamp and duration were computed and then thrown away: no transport ever saw them, so session length could not be measured downstream. A session end is now reported through an injected onSessionEnded callback on SessionManager, which the client wires to an emission of a _session.stop event through the configured Sender. That is the event type legacy Amplify Analytics used for the same signal, so consumers keyed on Pinpoint's convention keep working; it is a documented public constant. The emitted event's session section carries the ended session, and the event is otherwise enriched exactly like one from record() — including the global attributes and metrics, which Pinpoint also stamped on _session.stop. Every end path reports exactly once: an explicit stopSession(), the session timeout expiring, close(), and the implicit stop when startSession() displaces a running session (Pinpoint reported a stop on displacement too). Ending an already-stopped session is a no-op, which is what keeps close()-after-stop and close()-after-timeout from emitting a second event. Nothing is emitted when no session is running or none ever started. stopSession() and close() now return Future<void> on both the client and the Flutter wrapper, completing once the event has been handed to the sender, and startSession() does the same for a displaced session. Sender failures are logged and never thrown. The timeout path fires from the timer with no caller to await it, so its failures surface only in the logs. SessionManager still knows nothing about senders or events, and the explicit-stop flag behaviour is unchanged: only the state transitions moved behind _endCurrent()/_startFresh() so state changes stay synchronous while the end notification is awaitable.
Completes legacy naming parity: a session boundary now emits _session.start as well as _session.stop, the same two event types amplify_analytics_pinpoint_dart used, both as documented public constants. SessionManager gained an injected onSessionStarted callback alongside onSessionEnded, and the client wires it to the emission. A start is reported from every start path: the eager start at construction when autoSessionTracking is on, an explicit startSession(), record()'s lazy start, the restart when a resume follows a session timeout, and the new session in a displacement. Resuming a paused session inside the timeout window is the same session, so nothing is reported for it. Each _startFresh() produces a distinct session, so a start is reported at most once per session. Ordering matches Pinpoint, whose startSession() stopped the running session before starting the replacement: a displacement reports the end first and only then the start, and the start is not begun until the end's emission has completed. Both state transitions are still applied synchronously before anything is awaited, so session/state describe the new session the moment startSession() is called. _endCurrent() now returns null rather than a completed future when there was nothing to end, which lets startSession() report a start with nothing to displace synchronously instead of after a microtask. Without that, a caller that constructs the client and records an event straight away would have had its event reach the sender ahead of the session's own start. record()'s lazy start is awaited, so the start event precedes the recorded event. The eager start at construction and the timeout-driven restart have no caller to await them and are fired with the same catch-and-log guard as the timeout's end report, documented as surfacing only in the logs.
Dug into both reports. The dart package's three deductions: the foundation constraint (fixed in the latest push, it's The wrapper's 30 looks alarming but almost all of it is one cascading cause: pana resolves from pub.dev and So post-merge and post-publish these should sit at or near full score. The real gaps in the reports were the dart example and the description length, both in now. |
Yes, thanks. Added, with |
Summary
Event Enrichment client for analytics — the on-device companion that stamps device/app/session/identity context onto events and emits a Pinpoint-compatible JSON envelope, so events sent via the Kinesis/Firehose clients keep the analytics context Pinpoint used to provide automatically.
PR Stack (3-way split)
feat/event-enrichment-clientfeat/event-enrichment-client-examplefeat/event-enrichment-client-testsStructure
Dart/Flutter split, mirroring the Kinesis packages:
amplify_event_enrichment_dart— pure-Dart core (client,EnrichedEvent,SessionManager, global fields, metadata,EventSinkinterface). No Flutter dependency.amplify_event_enrichment— Flutter wrapper: lifecycle observer,SharedPreferencesClientIdProvider, injectableDeviceMetadataProvider, re-exports the core.Built on v3 Amplify Foundation (
amplify_foundation_dart,Result<T>, sealed exceptions,AmplifyLogging).Key decisions
shared_preferencesundercom.amplifyframework.device_id, read-or-create — a cross-package contract shared with the Connect client (whichever inits first generates the UUID; the other reads it). Maps to native SharedPreferences/NSUserDefaults so a device gets one id everywhere.device_info_plusis not a core dependency; device metadata comes from an injectableDeviceMetadataProviderdefaulting todart:io Platform. TheEventSinkships as an interface only (no default sinks) — keeps enrichment transport-agnostic.Verification
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.