-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Conversation
🦋 Changeset detectedLatest commit: 057f40f The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good! I appreciate the typesafety updates.
const inlineConfig = flagsToAstroInlineConfig(flags); | ||
const { userConfig } = await resolveConfig(inlineConfig, 'add'); | ||
telemetry.record(eventCliSession('add', userConfig)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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', {}))
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Questions withdrawn because I know how to Google.
Nothing blocking, the one comment I left is up to your discretion.
const inlineConfig = flagsToAstroInlineConfig(flags); | ||
const { userConfig } = await resolveConfig(inlineConfig, 'add'); | ||
telemetry.record(eventCliSession('add', userConfig)); |
There was a problem hiding this comment.
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.
Changes
ASTRO_CLI_SESSION_STARTED
telemetry eventmeasureX
helper functions that prevent raw user input collective with input checking, type errors, and centralized sanitization logic.Testing
Docs