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

Refactor telemetry usage flow #7522

Merged
merged 4 commits into from
Jun 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/astro/src/@types/astro.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1375,7 +1375,6 @@ export interface AstroSettings {
tsConfig: TsConfigJson | undefined;
tsConfigPath: string | undefined;
watchFiles: string[];
forceDisableTelemetry: boolean;
Copy link
Member Author

@bluwy bluwy Jun 29, 2023

Choose a reason for hiding this comment

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

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.

timer: AstroTimer;
}

Expand Down
8 changes: 3 additions & 5 deletions packages/astro/src/cli/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ async function runCommand(cmd: string, flags: yargs.Arguments) {

telemetry.record(event.eventCliSession(cmd));
const packages = flags._.slice(3) as string[];
return await add(packages, { cwd: root, flags, logging, telemetry });
return await add(packages, { cwd: root, flags, logging });
}
case 'docs': {
telemetry.record(event.eventCliSession(cmd));
Expand All @@ -170,7 +170,7 @@ async function runCommand(cmd: string, flags: yargs.Arguments) {
// Do not track session start, since the user may be trying to enable,
// disable, or modify telemetry settings.
const subcommand = flags._[3]?.toString();
return await telemetryHandler.update(subcommand, { flags, telemetry });
return await telemetryHandler.update(subcommand, { flags });
}
}

Expand Down Expand Up @@ -209,7 +209,6 @@ async function runCommand(cmd: string, flags: yargs.Arguments) {
configFlagPath,
flags,
logging,
telemetry,
handleConfigError(e) {
handleConfigError(e, { cmd, cwd: root, flags, logging });
info(logging, 'astro', 'Continuing with previous valid configuration\n');
Expand All @@ -224,7 +223,6 @@ async function runCommand(cmd: string, flags: yargs.Arguments) {
return await build(settings, {
flags,
logging,
telemetry,
teardownCompiler: true,
mode: flags.mode,
});
Expand Down Expand Up @@ -256,7 +254,7 @@ async function runCommand(cmd: string, flags: yargs.Arguments) {
case 'preview': {
const { default: preview } = await import('../core/preview/index.js');

const server = await preview(settings, { logging, telemetry, flags });
const server = await preview(settings, { logging, flags });
if (server) {
return await server.closed(); // keep alive until the server is closed
}
Expand Down
6 changes: 3 additions & 3 deletions packages/astro/src/cli/telemetry.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
/* eslint-disable no-console */
import type { AstroTelemetry } from '@astrojs/telemetry';
import type yargs from 'yargs-parser';
import * as msg from '../core/messages.js';
import { telemetry } from '../events/index.js';

export interface TelemetryOptions {
flags: yargs.Arguments;
telemetry: AstroTelemetry;
}

export async function update(subcommand: string, { flags, telemetry }: TelemetryOptions) {
export async function update(subcommand: string, { flags }: TelemetryOptions) {
const isValid = ['enable', 'disable', 'reset'].includes(subcommand);

if (flags.help || flags.h || !isValid) {
Expand Down
4 changes: 1 addition & 3 deletions packages/astro/src/core/add/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import type { AstroTelemetry } from '@astrojs/telemetry';
import boxen from 'boxen';
import { diffWords } from 'diff';
import { execa } from 'execa';
Expand Down Expand Up @@ -30,7 +29,6 @@ import { wrapDefaultExport } from './wrapper.js';
export interface AddOptions {
logging: LogOptions;
flags: yargs.Arguments;
telemetry: AstroTelemetry;
cwd?: string;
}

Expand Down Expand Up @@ -83,7 +81,7 @@ async function getRegistry(): Promise<string> {
return stdout || 'https://registry.npmjs.org';
}

export default async function add(names: string[], { cwd, flags, logging, telemetry }: AddOptions) {
export default async function add(names: string[], { cwd, flags, logging }: AddOptions) {
applyPolyfill();
if (flags.help || names.length === 0) {
printHelp({
Expand Down
2 changes: 0 additions & 2 deletions packages/astro/src/core/build/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import type { AstroTelemetry } from '@astrojs/telemetry';
import type { AstroConfig, AstroSettings, ManifestData, RuntimeMode } from '../../@types/astro';

import fs from 'fs';
Expand Down Expand Up @@ -26,7 +25,6 @@ import { getTimeStat } from './util.js';
export interface BuildOptions {
mode?: RuntimeMode;
logging: LogOptions;
telemetry: AstroTelemetry;
/**
* Teardown the compiler WASM instance after build. This can improve performance when
* building once, but may cause a performance hit if building multiple times in a row.
Expand Down
1 change: 0 additions & 1 deletion packages/astro/src/core/config/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@ export function createBaseSettings(config: AstroConfig): AstroSettings {
scripts: [],
clientDirectives: getDefaultClientDirectives(),
watchFiles: [],
forceDisableTelemetry: false,
timer: new AstroTimer(),
};
}
Expand Down
8 changes: 1 addition & 7 deletions packages/astro/src/core/dev/container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ export interface CreateContainerParams {
// The string passed to --config and the resolved path
configFlag?: string;
configFlagPath?: string;
disableTelemetry?: boolean;
}

export async function createContainer(params: CreateContainerParams = {}): Promise<Container> {
Expand All @@ -55,13 +54,8 @@ export async function createContainer(params: CreateContainerParams = {}): Promi
logging = defaultLogging,
settings = await createDefaultDevSettings(params.userConfig, params.root),
fs = nodeFs,
disableTelemetry,
} = params;

if (disableTelemetry) {
settings.forceDisableTelemetry = true;
}

// Initialize
applyPolyfill();
settings = await runHookConfigSetup({
Expand Down Expand Up @@ -149,7 +143,7 @@ export async function runInContainer(
params: CreateContainerParams,
callback: (container: Container) => Promise<void> | void
) {
const container = await createContainer({ ...params, disableTelemetry: true });
const container = await createContainer(params);
try {
await callback(container);
} finally {
Expand Down
5 changes: 2 additions & 3 deletions packages/astro/src/core/dev/dev.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import type { AstroTelemetry } from '@astrojs/telemetry';
import type http from 'http';
import { cyan } from 'kleur/colors';
import type { AddressInfo } from 'net';
Expand All @@ -7,6 +6,7 @@ import type * as vite from 'vite';
import type yargs from 'yargs-parser';
import type { AstroSettings } from '../../@types/astro';
import { attachContentServerListeners } from '../../content/index.js';
import { telemetry } from '../../events/index.js';
import { info, warn, type LogOptions } from '../logger/core.js';
import * as msg from '../messages.js';
import { printHelp } from '../messages.js';
Expand All @@ -18,7 +18,6 @@ export interface DevOptions {
configFlagPath: string | undefined;
flags?: yargs.Arguments;
logging: LogOptions;
telemetry: AstroTelemetry;
handleConfigError: (error: Error) => void;
isRestart?: boolean;
}
Expand Down Expand Up @@ -56,7 +55,7 @@ export default async function dev(
}

const devStart = performance.now();
await options.telemetry.record([]);
await telemetry.record([]);

// Create a container which sets up the Vite server.
const restart = await createContainerWithAutomaticRestart({
Expand Down
2 changes: 0 additions & 2 deletions packages/astro/src/core/preview/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import type { AstroTelemetry } from '@astrojs/telemetry';
import { cyan } from 'kleur/colors';
import { createRequire } from 'module';
import { pathToFileURL } from 'url';
Expand All @@ -12,7 +11,6 @@ import { getResolvedHostForHttpServer } from './util.js';

interface PreviewOptions {
logging: LogOptions;
telemetry: AstroTelemetry;
flags?: Arguments;
}

Expand Down
1 change: 0 additions & 1 deletion packages/astro/src/core/render/dev/environment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ export function createDevelopmentEnvironment(
site: settings.config.site,
ssr: isServerLikeOutput(settings.config),
streaming: true,
telemetry: Boolean(settings.forceDisableTelemetry),
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

});

return {
Expand Down
2 changes: 0 additions & 2 deletions packages/astro/src/core/render/environment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ export interface Environment {
site?: string;
ssr: boolean;
streaming: boolean;
telemetry?: boolean;
}

export type CreateEnvironmentArgs = Environment;
Expand Down Expand Up @@ -53,6 +52,5 @@ export function createBasicEnvironment(options: CreateBasicEnvironmentArgs): Env
routeCache: new RouteCache(options.logging, mode),
ssr: options.ssr ?? true,
streaming: options.streaming ?? true,
telemetry: false,
});
}
4 changes: 1 addition & 3 deletions packages/astro/src/vite-plugin-astro-server/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,7 @@ export async function handleRequest(
// Our error should already be complete, but let's try to add a bit more through some guesswork
const errorWithMetadata = collectErrorMetadata(err, config.root);

if (env.telemetry !== false) {
telemetry.record(eventError({ cmd: 'dev', err: errorWithMetadata, isFatal: false }));
}
telemetry.record(eventError({ cmd: 'dev', err: errorWithMetadata, isFatal: false }));

error(env.logging, null, msg.formatErrorMessage(errorWithMetadata));
handle500Response(moduleLoader, res, errorWithMetadata);
Expand Down
20 changes: 6 additions & 14 deletions packages/astro/test/test-utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ polyfill(globalThis, {
exclude: 'window document',
});

// Disable telemetry when running tests
process.env.ASTRO_TELEMETRY_DISABLED = true;
Comment on lines +25 to +26
Copy link
Member Author

Choose a reason for hiding this comment

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

This is where I disable the telemetry. It should affect other integration tests as they import this file too.


/**
* @typedef {import('undici').Response} Response
* @typedef {import('../src/core/dev/dev').DedvServer} DevServer
Expand Down Expand Up @@ -146,13 +149,6 @@ export async function loadFixture(inlineConfig) {
return settings;
};

/** @type {import('@astrojs/telemetry').AstroTelemetry} */
const telemetry = {
record() {
return Promise.resolve();
},
};

const resolveUrl = (url) =>
`http://${config.server.host || 'localhost'}:${config.server.port}${url.replace(/^\/?/, '/')}`;

Expand Down Expand Up @@ -184,15 +180,15 @@ export async function loadFixture(inlineConfig) {
return {
build: async (opts = {}) => {
process.env.NODE_ENV = 'production';
return build(await getSettings(), { logging, telemetry, ...opts });
return build(await getSettings(), { logging, ...opts });
},
sync: async (opts) => sync(await getSettings(), { logging, fs, ...opts }),
check: async (opts) => {
return await check(await getSettings(), { logging, ...opts });
},
startDevServer: async (opts = {}) => {
process.env.NODE_ENV = 'development';
devServer = await dev(await getSettings(), { logging, telemetry, ...opts });
devServer = await dev(await getSettings(), { logging, ...opts });
config.server.host = parseAddressToHost(devServer.address.address); // update host
config.server.port = devServer.address.port; // update port
return devServer;
Expand All @@ -214,11 +210,7 @@ export async function loadFixture(inlineConfig) {
},
preview: async (opts = {}) => {
process.env.NODE_ENV = 'production';
const previewServer = await preview(await getSettings(), {
logging,
telemetry,
...opts,
});
const previewServer = await preview(await getSettings(), { logging, ...opts });
config.server.host = parseAddressToHost(previewServer.host); // update host
config.server.port = previewServer.port; // update port
return previewServer;
Expand Down
1 change: 0 additions & 1 deletion packages/astro/test/units/routing/route-matching.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,6 @@ describe('Route matching', () => {
output: 'hybrid',
adapter: testAdapter(),
},
disableTelemetry: true,
});
settings = container.settings;

Expand Down
2 changes: 1 addition & 1 deletion packages/astro/test/units/shiki/shiki.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ describe('<Code />', () => {
let container;
let mod;
before(async () => {
container = await createContainer({ root, disableTelemetry: true });
container = await createContainer({ root });
Copy link
Member Author

Choose a reason for hiding this comment

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

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:

  1. Our unit tests currently don't trigger any telemetry record
  2. Unit tests probably shouldn't trigger any telemetry at the first place.
  3. Tests run more often in CI than locally, and CI has it disabled.

So I think it's fine 😬

Copy link
Member

Choose a reason for hiding this comment

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

How do we discover if the unit tests will start triggering telemetry events?

Copy link
Member Author

@bluwy bluwy Jun 29, 2023

Choose a reason for hiding this comment

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

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.

const loader = createViteLoader(container.viteServer);
mod = await loader.import('astro/components/Shiki.js');
});
Expand Down