-
Notifications
You must be signed in to change notification settings - Fork 52
IMPROVEMENT: Removed xdm.timestamp and added meta.queueTimeMillis #1416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| event.mergeData(userData); | ||
| } | ||
|
|
||
| if (content?.xdm?.timestamp) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't there always be a timestamp inserted by the context component (https://vscode.dev/github/adobe/alloy/blob/impr/noStampOnlyQueue/packages/core/src/components/Context/index.js#L46)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think so but it seems there's always an exception and a million ways clients or their users might mess with the structure 😅
|
|
||
| expect(event.meta.queueTimeMillis).toBeGreaterThanOrEqual(0); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add tests for known conditions where the event would be queued:
- Waiting for consent - (configured with defaultConsent=pending, sendEvent, sleep, setConsent)
- Running events close together - (3 sendEvents), first should be sent immediately, second and third will wait for first to return so that identity is established.
Also somewhere we should add integration tests that use timestamp strings that customers have actually gotten for the timestamp as invalid. You may be able to use vi.spyOn(Date, "new") to provide mock data. Look through the linked tickets for PLATIRs and data customers have run into.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added an integration test for the first case. For the second case see discussion in slack
Description
Removes event timestamp during event finalization and adds a
metaproperty to determine the amount of time the event was enqueued.Related Issue
Jira
Motivation and Context
Timestamps from the WebSDK are dependent on user device settings and so can be inaccurate. In order to avoid TIME DRIFT in Konductor, we should no longer send event timestamps in the SDK. Instead, Konductor will insert the timestamp when the request is received. Because send event commands can be queued (while they wait for script load, consent, identity, etc.), we need to track the delta between when the command is enqueued and when it is actually sent to determine in Konductor when the event occurred.
Screenshots (if appropriate):
N/A
Checklist: