Skip to content

Update telemetry systemCode#1190

Open
camillecroci wants to merge 4 commits intomainfrom
code-system-client-metrics
Open

Update telemetry systemCode#1190
camillecroci wants to merge 4 commits intomainfrom
code-system-client-metrics

Conversation

@camillecroci
Copy link

@camillecroci camillecroci commented Feb 5, 2026

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 systemCode at 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 magical guessSystemCode so 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:

  • My branch has been rebased onto the latest commit on main (don't merge main into your branch) (although depending on how long this takes to be merged, I might need to rebase again)
  • My commit messages are conventional commits, for example: feat(circleci): add support for nightly workflows, fix: set Heroku app name for staging apps too

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 systemCode to the TelemetryEvent type and require it in TelemetryRecorder.recordEvent.
  • Update telemetry tests and test child-process fixture to provide systemCode.
  • Update CLI task execution to provide a systemCode value 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.

@camillecroci camillecroci force-pushed the code-system-client-metrics branch from ebe9630 to 3750bcd Compare February 16, 2026 13:57
Comment on lines +150 to 155
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 }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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

Camille Croci added 4 commits February 25, 2026 15:36
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
@camillecroci camillecroci force-pushed the code-system-client-metrics branch from 3750bcd to 4f10d2a Compare February 25, 2026 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants