Refactor telemetry usage flow #7522
Conversation
|
bluwy
changed the title
github-actions
bot
added
the
pkg: astro
| @@ -1375,7 +1375,6 @@ export interface AstroSettings { | |||
| tsConfig: TsConfigJson | undefined; | |||
| tsConfigPath: string | undefined; | |||
| watchFiles: string[]; | |||
| forceDisableTelemetry: boolean; | |||
There was a problem hiding this comment.
For the code review, you may notice disableTelemetry, forceDisableTelemtry, etc gets removed. I found these options only exists for tests to pass a value.
I removed it to simplify the code and rely on process.env.ASTRO_TELEMETRY_DISABLED = true instead.
| @@ -32,7 +32,6 @@ export function createDevelopmentEnvironment( | |||
| site: settings.config.site, | |||
| ssr: isServerLikeOutput(settings.config), | |||
| streaming: true, | |||
| telemetry: Boolean(settings.forceDisableTelemetry), | |||
There was a problem hiding this comment.
Note: There's a bug here before. This should be telemetry: !settings.forceDisableTelemetry. Luckily we aren't recording any telemetry in the tests now that affects this.
| // Disable telemetry when running tests | ||
| process.env.ASTRO_TELEMETRY_DISABLED = true; |
There was a problem hiding this comment.
This is where I disable the telemetry. It should affect other integration tests as they import this file too.
| container = await createContainer({ root, disableTelemetry: true }); | ||
| container = await createContainer({ root }); |
There was a problem hiding this comment.
For unit tests, I didn't disable telemetry because the only way is to set it through the npm script command, which is a bit ugly.
But:
- Our unit tests currently don't trigger any telemetry record
- Unit tests probably shouldn't trigger any telemetry at the first place.
- Tests run more often in CI than locally, and CI has it disabled.
So I think it's fine 😬
There was a problem hiding this comment.
How do we discover if the unit tests will start triggering telemetry events?
There was a problem hiding this comment.
I don't know 😄 Currently I do a global search for .record and .notify, and there's only a few places used and I know they are not unit-testable.
I wouldn't mind adding a flag to the npm script to disable it completely if you prefer too.
Changes
We currently have code that passes it through several functions, because tests can then pass a telemetry stub to disable telemetry. I think it's simpler to set
process.env.ASTRO_TELEMETRY_DISABLED = trueinstead to disable this so we don't leak the test implementation details.Testing
Tested manually.
Docs
n/a.