-
-
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
update telemetry to support more anonymized project id #3713
Conversation
🦋 Changeset detectedLatest commit: 58cf55d The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
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 |
const packages = flags._.slice(3) as string[]; | ||
telemetry.record( | ||
event.eventCliSession({ | ||
astroVersion: process.env.PACKAGE_VERSION ?? '', |
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 isn't needed, since astroVersion
is passed to the AstroTelemetry
constructor above.
viteVersion: getViteVersion(), | ||
nodeVersion: process.version.replace(/^v?/, ''), | ||
configKeys: userConfig ? configKeys(userConfig, '') : undefined, | ||
// Config Values | ||
config: configValues, | ||
flags: cliFlags, | ||
// Optional integrations | ||
optionalIntegrations: userConfig?.integrations?.length - integrations?.length, |
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 added as a "nice-to-have" in a PR that was fixing a different bug, but I don't think this is actually valuable to us so I removed it.
@@ -108,7 +107,7 @@ export function eventCliSession( | |||
: undefined, | |||
markdown: userConfig?.markdown | |||
? { | |||
mode: userConfig?.markdown?.mode, |
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.
mode is no longer supported but the types were out of date, so this wasn't caught. Removed this and added drafts
instead.
export interface AstroTelemetryOptions { | ||
version: string; | ||
} | ||
import { getProjectInfo, ProjectInfo } from './project-info.js'; |
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.
Note: All changes in this file are from refactoring and cleanup and small bug fixing. Lots of little things tackled, but the meat of the change (the new project ID) lives in project-info.js
.
return fetch(ASTRO_TELEMETRY_ENDPOINT, { | ||
method: 'POST', | ||
body: JSON.stringify(body), | ||
headers: { 'content-type': 'application/json' }, | ||
}) | ||
.catch(noop) |
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.
moved this post handling logic into the call telemetry/src/index.ts
Need a changeset |
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.
Overall I'm EXTREMELY happy with this update! Just had a few comments but nothing blocking.
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.
LGTM!
* update telemetry to support more anonymized project id * Create strange-laws-kick.md * respond to nate feedback
Changes
projectId
to be more anonymous: it's now based on a commit hash instead of your git remote. This means that even if the hash is somehow reversed, it is no longer possible to identify the project.project-info.ts
file.Refactoring
Testing
Docs