Conversation
There was a problem hiding this comment.
Pull request overview
Updates Tool Kit telemetry event shape to include a required root-level systemCode, aligning with the new client-metrics-server expectations.
Changes:
- Add
systemCodeto theTelemetryEventtype and require it inTelemetryRecorder.recordEvent. - Update telemetry tests and test child-process fixture to provide
systemCode. - Update CLI task execution to provide a
systemCodevalue when recording task completion metrics.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/telemetry/src/types.ts | Adds systemCode to the telemetry event contract. |
| lib/telemetry/src/index.ts | Updates recordEvent API to require systemCode and includes it in emitted events. |
| lib/telemetry/test/index.test.ts | Updates tests to pass systemCode when recording events. |
| lib/telemetry/test/metricsProcess.mjs | Updates child-process test fixture to pass systemCode. |
| core/cli/src/tasks.ts | Computes a systemCode (with fallback) and passes it into telemetry events. |
| core/cli/src/install.ts | Removes prior systemCode-scoping behavior when loading hook installations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ebe9630 to
3750bcd
Compare
| recordEvent<N extends Namespace>(namespace: N, systemCode: string, details: NamespaceSchemas[N]) { | ||
| const event: TelemetryEvent = { | ||
| namespace: `dotcom-tool-kit.${namespace}`, | ||
| systemCode, | ||
| eventTimestamp: Date.now(), | ||
| data: { ...this.attributes, ...details } |
There was a problem hiding this comment.
suggestion: I think users that call the recordEvent function shouldn't have to concern themselves with metadata explicitly, and the systemCode in particular is something that should be passed around in the scoped metadata rather than calling guessSystemCode with the same fallback every time we want to record an event. I think it makes more sense to move the fallback logic here and get the systemCode from the scoped attributes?
it will also resolve the breaking change Mx LLM is talking about which is a little bit of a concern even though no one has enabled the telemetry the function arguments are still processed and will cause a TypeError if you update the telemetry package but are still using another Tool Kit package that calls the old recordEvent function.
| recordEvent<N extends Namespace>(namespace: N, systemCode: string, details: NamespaceSchemas[N]) { | |
| const event: TelemetryEvent = { | |
| namespace: `dotcom-tool-kit.${namespace}`, | |
| systemCode, | |
| eventTimestamp: Date.now(), | |
| data: { ...this.attributes, ...details } | |
| recordEvent<N extends Namespace>(namespace: N, details: NamespaceSchemas[N]) { | |
| const data = { ...this.attributes, ...details } | |
| const event: TelemetryEvent = { | |
| namespace: `dotcom-tool-kit.${namespace}`, | |
| systemCode: data.systemCode || 'dotcom-tool-kit', | |
| eventTimestamp: Date.now(), | |
| data |
It is now required by the client-metrics-server at root level
Remove useless import Check in unit tests that the systemCode is actually in the event
3750bcd to
4f10d2a
Compare
Description
Summary
Reliability is working on a new client metrics server and Ivo was very proactive in adding the tool kit side of this project but a WIP means that some change might still happen. And friends, they happened. We are now requiring a
systemCodeat the root level of the event (so same as the namespace) which means the telemetry code needed a bit of refactoring to provide it. From my understanding, the systemCode is not a required property and could be undefined when using that little magicalguessSystemCodeso I took on me to provide a default one (dotcom-tool-kit) but I am open to suggestion.Relevant motivation
I was very motivated.
But also it is for tool kit to be up to date with our latest change without having to do anything about it (apart from reviewing this pr I guess)
A bit more context
This was Ivo PR to add the metrics client.
Checklist:
feat(circleci): add support for nightly workflows,fix: set Heroku app name for staging apps too