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

telemetry: Improve config info object format #8600

Merged
merged 1 commit into from
Sep 20, 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
6 changes: 6 additions & 0 deletions .changeset/large-shoes-hammer.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@astrojs/telemetry': patch
'astro': patch
---

Improve config info telemetry
13 changes: 10 additions & 3 deletions packages/astro/src/cli/add/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,12 @@ import ora from 'ora';
import preferredPM from 'preferred-pm';
import prompts from 'prompts';
import type yargs from 'yargs-parser';
import { loadTSConfig, resolveConfigPath, resolveRoot } from '../../core/config/index.js';
import {
loadTSConfig,
resolveConfig,
resolveConfigPath,
resolveRoot,
} from '../../core/config/index.js';
import {
defaultTSConfig,
presets,
Expand All @@ -23,7 +28,7 @@ import { appendForwardSlash } from '../../core/path.js';
import { apply as applyPolyfill } from '../../core/polyfill.js';
import { parseNpmName } from '../../core/util.js';
import { eventCliSession, telemetry } from '../../events/index.js';
import { createLoggerFromFlags } from '../flags.js';
import { createLoggerFromFlags, flagsToAstroInlineConfig } from '../flags.js';
import { generate, parse, t, visit } from './babel.js';
import { ensureImport } from './imports.js';
import { wrapDefaultExport } from './wrapper.js';
Expand Down Expand Up @@ -87,7 +92,9 @@ async function getRegistry(): Promise<string> {
}

export async function add(names: string[], { flags }: AddOptions) {
telemetry.record(eventCliSession('add'));
const inlineConfig = flagsToAstroInlineConfig(flags);
const { userConfig } = await resolveConfig(inlineConfig, 'add');
telemetry.record(eventCliSession('add', userConfig));
Comment on lines +95 to +97
Copy link
Member

Choose a reason for hiding this comment

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

Is the current config actually important to record here?

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a problem if this info is statistically useful, but it seems like we'll get final data when users run dev or the build so would err on the side of not recording this.

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 was the one command that wasn't reading config, which meant the entire eventCliSession command had to handle a potentially undefined config object.

This is still useful data to better understand how astro add is used, similar to any other command.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can pass an empty user config object instead so we don't have to load the entire thing? e.g. telemetry.record(eventCliSession('add', {}))

Copy link
Member Author

Choose a reason for hiding this comment

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

Is the reason that we don't track it here because it's slow?

I see approvals so I'll merge but happy to revisit this post-PR if there's still interest in removing!

applyPolyfill();
if (flags.help || names.length === 0) {
printHelp({
Expand Down
24 changes: 3 additions & 21 deletions packages/astro/src/core/config/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,6 @@ export const AstroConfigSchema = z.object({
.optional()
.default(ASTRO_CONFIG_DEFAULTS.build.excludeMiddleware),
})
.optional()
natemoo-re marked this conversation as resolved.
Show resolved Hide resolved
.default({}),
server: z.preprocess(
// preprocess
Expand All @@ -158,7 +157,6 @@ export const AstroConfigSchema = z.object({
port: z.number().optional().default(ASTRO_CONFIG_DEFAULTS.server.port),
headers: z.custom<OutgoingHttpHeaders>().optional(),
})
.optional()
natemoo-re marked this conversation as resolved.
Show resolved Hide resolved
.default({})
),
redirects: z
Expand Down Expand Up @@ -274,27 +272,11 @@ export const AstroConfigSchema = z.object({
.optional()
.default(ASTRO_CONFIG_DEFAULTS.experimental.optimizeHoistedScript),
})
.passthrough()
.refine(
(d) => {
const validKeys = Object.keys(ASTRO_CONFIG_DEFAULTS.experimental);
const invalidKeys = Object.keys(d).filter((key) => !validKeys.includes(key));
if (invalidKeys.length > 0) return false;
return true;
},
(d) => {
const validKeys = Object.keys(ASTRO_CONFIG_DEFAULTS.experimental);
const invalidKeys = Object.keys(d).filter((key) => !validKeys.includes(key));
return {
message: `Invalid experimental key: \`${invalidKeys.join(
', '
)}\`. \nMake sure the spelling is correct, and that your Astro version supports this experiment.\nSee https://docs.astro.build/en/reference/configuration-reference/#experimental-flags for more information.`,
};
}
.strict(
`Invalid or outdated experimental feature.\nCheck for incorrect spelling or outdated Astro version.\nSee https://docs.astro.build/en/reference/configuration-reference/#experimental-flags for a list of all current experiments.`
)
.optional()
.default({}),
legacy: z.object({}).optional().default({}),
legacy: z.object({}).default({}),
natemoo-re marked this conversation as resolved.
Show resolved Hide resolved
});

export type AstroConfigType = z.infer<typeof AstroConfigSchema>;
Expand Down
194 changes: 108 additions & 86 deletions packages/astro/src/events/session.ts
Original file line number Diff line number Diff line change
@@ -1,25 +1,8 @@
import type { AstroUserConfig } from '../@types/astro.js';
import type { AstroIntegration, AstroUserConfig } from '../@types/astro.js';
import { AstroConfigSchema } from '../core/config/schema.js';

const EVENT_SESSION = 'ASTRO_CLI_SESSION_STARTED';

interface ConfigInfo {
markdownPlugins: string[];
adapter: string | null;
integrations: string[];
trailingSlash: undefined | 'always' | 'never' | 'ignore';
build:
| undefined
| {
format: undefined | 'file' | 'directory';
};
markdown:
| undefined
| {
drafts: undefined | boolean;
syntaxHighlight: undefined | 'shiki' | 'prism' | false;
};
}

interface EventPayload {
cliCommand: string;
config?: ConfigInfo;
Expand All @@ -28,87 +11,126 @@ interface EventPayload {
optionalIntegrations?: number;
}

const multiLevelKeys = new Set([
'build',
'markdown',
'markdown.shikiConfig',
'server',
'vite',
'vite.resolve',
'vite.css',
'vite.json',
'vite.server',
'vite.server.fs',
'vite.build',
'vite.preview',
'vite.optimizeDeps',
'vite.ssr',
'vite.worker',
]);
function configKeys(obj: Record<string, any> | undefined, parentKey: string): string[] {
if (!obj) {
return [];
type ConfigInfoValue = string | boolean | string[] | undefined;
type ConfigInfoRecord = Record<string, ConfigInfoValue>;
type ConfigInfoBase = {
[alias in keyof AstroUserConfig]: ConfigInfoValue | ConfigInfoRecord;
};
export interface ConfigInfo extends ConfigInfoBase {
build: ConfigInfoRecord;
image: ConfigInfoRecord;
markdown: ConfigInfoRecord;
experimental: ConfigInfoRecord;
legacy: ConfigInfoRecord;
vite: ConfigInfoRecord | undefined;
}

function measureIsDefined(val: unknown) {
// if val is undefined, measure undefined as a value
if (val === undefined) {
return undefined;
}
// otherwise, convert the value to a boolean
return Boolean(val);
}

type StringLiteral<T> = T extends string ? (string extends T ? never : T) : never;

return Object.entries(obj)
.map(([key, value]) => {
if (typeof value === 'object' && !Array.isArray(value)) {
const localKey = parentKey ? parentKey + '.' + key : key;
if (multiLevelKeys.has(localKey)) {
let keys = configKeys(value, localKey).map((subkey) => key + '.' + subkey);
keys.unshift(key);
return keys;
}
}
/**
* Measure supports string literal values. Passing a generic `string` type
* results in an error, to make sure generic user input is never measured directly.
*/
function measureStringLiteral<T extends string>(
val: StringLiteral<T> | boolean | undefined
): string | boolean | undefined {
return val;
}

return key;
})
.flat(1);
function measureIntegration(val: AstroIntegration | false | null | undefined): string | undefined {
if (!val || !val.name) {
return undefined;
}
return val.name;
}

function sanitizeConfigInfo(obj: object | undefined, validKeys: string[]): ConfigInfoRecord {
if (!obj || validKeys.length === 0) {
return {};
}
return validKeys.reduce(
(result, key) => {
result[key] = measureIsDefined((obj as Record<string, unknown>)[key]);
return result;
},
{} as Record<string, boolean | undefined>
);
}

/**
* This function creates an anonymous ConfigInfo object from the user's config.
* All values are sanitized to preserve anonymity. Simple "exist" boolean checks
* are used by default, with a few additional sanitized values added manually.
* Helper functions should always be used to ensure correct sanitization.
*/
function createAnonymousConfigInfo(userConfig: AstroUserConfig) {
// Sanitize and measure the generic config object
// NOTE(fks): Using _def is the correct, documented way to get the `shape`
// from a Zod object that includes a wrapping default(), optional(), etc.
// Even though `_def` appears private, it is type-checked for us so that
// any changes between versions will be detected.
const configInfo: ConfigInfo = {
...sanitizeConfigInfo(userConfig, Object.keys(AstroConfigSchema.shape)),
build: sanitizeConfigInfo(
userConfig.build,
Object.keys(AstroConfigSchema.shape.build._def.innerType.shape)
),
image: sanitizeConfigInfo(
userConfig.image,
Object.keys(AstroConfigSchema.shape.image._def.innerType.shape)
),
markdown: sanitizeConfigInfo(
userConfig.markdown,
Object.keys(AstroConfigSchema.shape.markdown._def.innerType.shape)
),
experimental: sanitizeConfigInfo(
userConfig.experimental,
Object.keys(AstroConfigSchema.shape.experimental._def.innerType.shape)
),
legacy: sanitizeConfigInfo(
userConfig.legacy,
Object.keys(AstroConfigSchema.shape.legacy._def.innerType.shape)
),
vite: userConfig.vite
? sanitizeConfigInfo(userConfig.vite, Object.keys(userConfig.vite))
: undefined,
};
// Measure string literal/enum configuration values
configInfo.build.format = measureStringLiteral(userConfig.build?.format);
configInfo.markdown.syntaxHighlight = measureStringLiteral(userConfig.markdown?.syntaxHighlight);
configInfo.output = measureStringLiteral(userConfig.output);
configInfo.scopedStyleStrategy = measureStringLiteral(userConfig.scopedStyleStrategy);
configInfo.trailingSlash = measureStringLiteral(userConfig.trailingSlash);
// Measure integration & adapter usage
configInfo.adapter = measureIntegration(userConfig.adapter);
configInfo.integrations = userConfig.integrations
?.flat(100)
.map(measureIntegration)
.filter(Boolean) as string[];
// Return the sanitized ConfigInfo object
return configInfo;
}

export function eventCliSession(
cliCommand: string,
userConfig?: AstroUserConfig,
userConfig: AstroUserConfig,
flags?: Record<string, any>
): { eventName: string; payload: EventPayload }[] {
// Filter out falsy integrations
const configValues = userConfig
? {
markdownPlugins: [
...(userConfig?.markdown?.remarkPlugins?.map((p) =>
typeof p === 'string' ? p : typeof p
) ?? []),
...(userConfig?.markdown?.rehypePlugins?.map((p) =>
typeof p === 'string' ? p : typeof p
) ?? []),
] as string[],
adapter: userConfig?.adapter?.name ?? null,
integrations: (userConfig?.integrations ?? [])
.filter(Boolean)
.flat()
.map((i: any) => i?.name),
trailingSlash: userConfig?.trailingSlash,
build: userConfig?.build
? {
format: userConfig?.build?.format,
}
: undefined,
markdown: userConfig?.markdown
? {
drafts: userConfig.markdown?.drafts,
syntaxHighlight: userConfig.markdown?.syntaxHighlight,
}
: undefined,
}
: undefined;

// Filter out yargs default `_` flag which is the cli command
const cliFlags = flags ? Object.keys(flags).filter((name) => name != '_') : undefined;

const payload: EventPayload = {
cliCommand,
configKeys: userConfig ? configKeys(userConfig, '') : undefined,
config: configValues,
config: createAnonymousConfigInfo(userConfig),
flags: cliFlags,
};
return [{ eventName: EVENT_SESSION, payload }];
Expand Down
Loading
Loading