feat(analytics-controller): add optional invocation options for track, identify, view#8701
feat(analytics-controller): add optional invocation options for track, identify, view#8701gauthierpetetin wants to merge 4 commits into
Conversation
…, identify, view Forward AnalyticsInvocationOptions (context, callback, messageId, timestamp) from AnalyticsController to AnalyticsPlatformAdapter for trackEvent, identify, and trackView. Extend adapter method signatures with optional third argument. Export AnalyticsContext and AnalyticsInvocationCallback. Co-authored-by: Cursor <cursoragent@cursor.com>
gauthierpetetin
force-pushed
the
feat/analytics-invocation-options
branch
from
7a3f783 to
002a571
Compare
gauthierpetetin
temporarily deployed
to
default-branch
GitHub Actions
Inactive
Co-authored-by: Cursor <cursoragent@cursor.com>
| /** | ||
| * Optional context object attached to the invocation. | ||
| */ | ||
| context?: AnalyticsContext; |
There was a problem hiding this comment.
Q: there's already a context attached by Segment SDK, how does this interfere with it?
There was a problem hiding this comment.
Extension adds context to events, that Segment SDK doesn't provide (cf. this code).
Segment can still add its own SDK context. We are not replacing that.
We are allowing the adapter to pass additional context through to Segment. If there are key collisions, that is a platform-adapter concern, so Extension should avoid overriding SDK-owned fields unless intentional.
So far, the only SDK-owned fields are:
context.library.name = '@segment/analytics-node'
context.library.version = <package version>
There was a problem hiding this comment.
Will all these values be in the same context object in the final event payload?
| /** | ||
| * Optional callback when the invocation completes or fails. | ||
| */ | ||
| callback?: AnalyticsInvocationCallback; |
There was a problem hiding this comment.
Q: What are the use cases of this? The tracking should never block or interfere with UX, so it's best effort and errors are handled inside controller, right?
There was a problem hiding this comment.
See message here.
Also callback is not for UX and should not be awaited by product flows. It is only the cleanup signal: once Segment finishes processing the call, we remove that messageId from the persisted queue.
| /** | ||
| * Optional stable identifier for deduplication or tracing. | ||
| */ | ||
| messageId?: string; |
There was a problem hiding this comment.
Q; we already have a message ID in the Segment payload. How is this different?
There was a problem hiding this comment.
See message here.
We need a messageId that is defined by MetaMetricsController before sending the event to Segment, in order to persist the event queue.
| /** | ||
| * Optional event time. ISO strings, Unix timestamps in milliseconds, or Date values are typical. | ||
| */ | ||
| timestamp?: Date | number | string; |
There was a problem hiding this comment.
Q: we already have a timestamp in Segment payload. How is this different?
There was a problem hiding this comment.
We need a timestamp that is defined by MetaMetricsController before sending the event to Segment, in order to preserve the original event time when the event is replayed later, thanks to the persisted event queue.
|
@NicolasMassart the reason why we need to pass On Mobile, that’s achieved thanks to Segment SDK’s That's quite problematic by the way, because it prevents us from fully getting rid of the existing MetaMetricController: we still need it besides AnalyticsController in order to persist the event queue. |
2 participants
Explanation
Add the possibility to forward
context,callback,messageId, andtimestampto AnalyticsController for trackEvent, identify, and trackView functions, , as this is needed in Extension.References
NA
Checklist
Note
Medium Risk
Extends the public
AnalyticsPlatformAdapterand controller method signatures, which is potentially breaking for downstream adapter implementations and callers. Logic change also altersmessageIdfor split anonymous events, which could affect event deduping/tracing semantics.Overview
Adds optional
AnalyticsInvocationOptions(context, callback,messageId, timestamp) and threads it throughAnalyticsController.trackEvent,identify, andtrackViewintoAnalyticsPlatformAdapter.track/identify/view.When anonymous-event splitting is enabled and sensitive properties are present, the controller now derives a distinct
messageIdsuffix for the anonymous/combined track call to avoid SDK deduping, and the test suite + changelog are updated accordingly.Reviewed by Cursor Bugbot for commit 20a4e85. Bugbot is set up for automated code reviews on this repo. Configure here.