Skip to content
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

more precise telemetry types #1765

Merged
merged 1 commit into from
Oct 17, 2024
Merged

more precise telemetry types #1765

merged 1 commit into from
Oct 17, 2024

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Oct 17, 2024

This removes the [key: string]: unknown in TelemetryData that was not being explicit about the contents of telemetry events. Furthermore, this adopts an explicit enumeration of event + step as a union type, such that any additional fields of telemetry data are tied to specific events. And this adopts a page loader so that we don’t have to remember to copy-paste the code into the documentation if and when the types change.

This raised a few questions:

  • Should we have a build + error event to report build failures?
  • Should the deploy + buldManifest event have a step, or be folded into the deploy + finish step?
  • Should the signal event be associated with the current command (one of {build, deploy, preview, login})?

@mbostock mbostock requested a review from visnup October 17, 2024 16:17
@mbostock mbostock enabled auto-merge (squash) October 17, 2024 16:18
@@ -29,22 +29,22 @@ describe("telemetry", () => {

it("sends data", async () => {
Telemetry._instance = new Telemetry(noopEffects);
Telemetry.record({event: "build", step: "start", test: true});
Copy link
Member

Choose a reason for hiding this comment

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

This reminded me why I annotated these as test: true: in case these events ran in CI and made it into our production events, I wanted an easy way to filter them out. Unsure if that's a good enough reason to keep it this way or not or simply try to firewall off any chance of them showing up in our actual data.

I did just double check and we have no recorded events with test: true in them.

@mbostock mbostock merged commit e2a3de6 into main Oct 17, 2024
4 checks passed
@mbostock mbostock deleted the mbostock/strict-telemetry-data branch October 17, 2024 21:55
@visnup
Copy link
Member

visnup commented Oct 17, 2024

The signal events can come at any time, outside of a known workflow since they're from system callbacks. So, in order to track it correctly, we'd need to set some global state somewhere for that callback to be able to reference.

@visnup
Copy link
Member

visnup commented Oct 17, 2024

I would expect errors to be the top-level event type. Oh, and we do always have session ids to correlate what was happening before something like an error or signal occur, within the same session.

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.

2 participants