NEW @W-17915999@ Implemented engine-level telemetry framework. #265
Conversation
jfeingold35
force-pushed
the
d/W-17915999
branch
from
fc10831 to
9a9ba7f
Compare
jfeingold35
force-pushed
the
d/W-17915999
branch
from
9a9ba7f to
5841976
Compare
| */ | ||
| export type TelemetryEvent = { | ||
| type: EventType.TelemetryEvent, | ||
| key: string, |
There was a problem hiding this comment.
A string-type key and a completely loose data object seemed to me to strike the balance between structure and flexibility that we want for telemetry.
| toAbsolutePath, | ||
| UniqueIdGenerator | ||
| } from "./utils"; | ||
| import {EngineProgressAggregator, SimpleUniqueIdGenerator, toAbsolutePath, UniqueIdGenerator} from "./utils"; |
There was a problem hiding this comment.
Not sure why my IDE saw fit to change this. I can change it back if we care.
jfeingold35
changed the title
jfeingold35
force-pushed
the
d/W-17915999
branch
from
286f3f4 to
ff99198
Compare
| // istanbul ignore next | ||
| private emitTelemetryEvent(eventName: string, data: TelemetryData): void { | ||
| this.emitEvent({ | ||
| type: EventType.TelemetryEvent, | ||
| timestamp: this.clock.now(), | ||
| eventName, | ||
| uuid: this.uniqueIdGenerator.getUniversallyUniqueId(), | ||
| data | ||
| }); | ||
| } |
There was a problem hiding this comment.
Probably didn't need to add this until we actually had a use for it. But if you want to update the CLI to prepare for the future scenario of possibly updating core with a core TelemetryEvent, that's fine to do now I suppose. Up to you.
| @@ -395,6 +400,17 @@ export class CodeAnalyzer { | |||
| }) | |||
| } | |||
|
|
|||
| // istanbul ignore next | |||
There was a problem hiding this comment.
If we do add this in even though it isn't being used (which probably means we shouldn't have this added to our code yet), can we at least add in a comment saying that this is unused currently, but when used to remove that istanbul ignore next.
| import path from "node:path"; | ||
| import crypto from "node:crypto"; |
There was a problem hiding this comment.
Should both of these instead be import * as path and import * as crypto instead of directly importing a namespace?
There was a problem hiding this comment.
I'll do that for safety's sake, though I'm not positive it actually matters here.
This PR does the following: