Add internal Checkout Kit telemetry foundations - #674
Conversation
Package Size
Web file breakdown
React Native file breakdown
Android file breakdown
Measured from the PR base SHA and PR head SHA. The file breakdown shows uncompressed sizes within each package artifact, so individual files do not sum to the compressed artifact total. This comment reports package artifact sizes only; it is not a final app binary-size report. |
Install this buildOpen Tophat, select your target device, then click Install. Links open on the Mac running Tophat.
Checkout Kit E2E results
|
kieran-osgood-shopify
left a comment
There was a problem hiding this comment.
These foundations are looking good - left a few small comments
I'm interested to see what metrics we integrate and whether we need to look at integrating any metrics in the react native side specifically (wondering if we can detect errors that prevent react-native contacting the native sdks?)
One thing that came up with an AI review was around the sentiment of how the keepAlive flush works on TS - sounds like it might contract the PR bodies statement, but could also be a conscious choice?
Given: Request A is active: `{keepalive: false}`
And: Metric B is queued
When: Pagehide calls `flush({keepalive: true})`
Then: no keepalive request is started at that moment
Instead: B waits for A's promise to settle
Here is the failing test it was verifying with
it('starts a keepalive export immediately when an ordinary export is active', async () => {
let resolveFirstFetch: ((value: {ok: boolean}) => void) | undefined;
const fetch = vi
.fn()
.mockImplementationOnce(
() =>
new Promise<{ok: boolean}>((resolve) => {
resolveFirstFetch = resolve;
}),
)
.mockResolvedValue({ok: true});
const telemetry = createCheckoutKitTelemetryForTesting({
sdkVersion: '1.2.3',
fetch,
now: () => BigInt(1),
});
telemetry.recordError({
category: 'http',
stage: 'load',
code: '5xx',
retryable: true,
});
const ordinaryFlush = telemetry.flush();
telemetry.recordError({
category: 'protocol',
stage: 'message',
code: 'unknown',
retryable: false,
});
const terminalFlush = telemetry.flush({keepalive: true});
expect(fetch).toHaveBeenCalledTimes(2);
expect(fetch.mock.calls[0]![1].keepalive).toBe(false);
expect(fetch.mock.calls[1]![1].keepalive).toBe(true);
await expect(terminalFlush).resolves.toBe(true);
resolveFirstFetch?.({ok: true});
await expect(ordinaryFlush).resolves.toBe(true);
});Fixing this would get us to
ordinary request A active
metric B queued
pagehide calls flush({keepalive: true})
keepalive request B starts immediately
|
|
||
| - name: Telemetry package checks | ||
| run: | | ||
| pnpm --filter @shopify/checkout-kit-telemetry run typecheck |
There was a problem hiding this comment.
These scripts seem to be part of the telemetry package not web - think we've been splitting jobs by domain, worth moving these to their own job file?
There was a problem hiding this comment.
Agreed — moved these to a dedicated Telemetry workflow with its own changed-file filter, and dropped the step from the web job. Web still runs its own suite on telemetry changes since the package is bundled into it.
| typecheck: | ||
| desc: "Type-check the TypeScript telemetry package" | ||
| run: cd telemetry/languages/typescript && pnpm typecheck | ||
| check: |
There was a problem hiding this comment.
I think we should update all of the dev.yml changes to include kotlin/swift, test / check / lint etc.
There was a problem hiding this comment.
Done — dev telemetry test now takes typescript/swift/kotlin and runs all three by default, and check aggregates typecheck + lint + tests. On lint: the Swift telemetry sources weren't in SwiftLint's scope at all, so I've added them; Kotlin was already covered by detekt inside the lib, and the TS package now has its own oxlint script.
8c0ff67 to
e36641b
Compare
|
Good catch on the keepalive path — that serialization was real. A pagehide flush would chain behind an in-flight export, and the page could be gone before it ever started. Keepalive flushes now grab the queued batch and start their own request immediately (ordinary flushes still serialize), and I've added your test pretty much as written. Updated the description bullet to match. On react-native: today RN metrics come entirely from the native SDKs (reported as |
| # The TypeScript telemetry package is a member of the web pnpm | ||
| # workspace, so dependency install and filtered runs happen there. |
There was a problem hiding this comment.
This broke my brain for a second - not introduced by your PR but that workspace file shouldn't sit in the web package
Summary
service.name=checkout-kitservice.version=<Checkout Kit version>telemetry.sdk.language=java|swift|webjstelemetry.sdk.name=checkout-kit-telemetrytelemetry.sdk.version=<Checkout Kit version>product=checkout_kit|accelerated_checkouts|customer_authplatform=android|swift|web|react-native-android|react-native-swiftis_retryattributeValidation