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

Add keystone telemetry inform command and update telemetry policy #9292

Merged
merged 19 commits into from
Aug 20, 2024

Conversation

dcousens
Copy link
Member

@dcousens dcousens commented Aug 15, 2024

This pull request adds a keystone telemetry inform command to print the consent notice - this is useful for testing:

> pnpm keystone telemetry inform
Keystone Telemetry
Keystone collects anonymous data when you run "keystone dev"

You can use "keystone telemetry --help" to update your preferences at any time

No telemetry data has been sent as part of this notice
Telemetry data will be sent the next time you run "keystone dev"

For more information, including how to opt-out see https://keystonejs.com/telemetry (updated 2024-08-20)

Additionally, as part of #9290 this pull request completes the new migration to a version 3 of the telemetry command.

Lastly, while reviewing #9290 and working on this pull request, I noticed that the database project event field was missing from our policy and JSON examples, but present in the code.

As in #9290, I will err on the side of missed consent for this and happily drop that information from the telemetry data being analysed as part of #9290.

Copy link

codesandbox-ci bot commented Aug 15, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 3dff2b2:

Sandbox Source
@keystone-6/sandbox Configuration

@@ -63,6 +63,7 @@ Keystone collects telemetry information in the form of two different types of da
We refer to these two different reports, as “device telemetry” and “project telemetry” respectively.

These reports are forwarded to [https://telemetry.keystonejs.com/](https://telemetry.keystonejs.com/), and are reported separately to minimize any correlation between them insofar as the timing and grouping of that data, that an otherwise combined report may have. We are collecting these two reports for different reasons, and thus have no need to associate them.
We differentiate and record the type and version of reports from the URL used by Keystone.
Copy link
Member Author

@dcousens dcousens Aug 15, 2024

Choose a reason for hiding this comment

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

Moving forward, we'll use the /3 version in the endpoint to differentiate report schema versions in the same way we differentiate report types using /project and /device.

This makes that abundantly transparent.

console.log() // gap to help visiblity
console.log(`For more information, including how to opt-out see ${grey`https://keystonejs.com/telemetry`} (updated ${b`2024-08-15`})`)
Copy link
Member Author

@dcousens dcousens Aug 15, 2024

Choose a reason for hiding this comment

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

As everyone will be re-informed as of #9290, this date helps to show that the information has been updated (however minor that change is)

@dcousens dcousens changed the title Add keystone telemetry inform command Add keystone telemetry inform command and update telemetry policy Aug 15, 2024
@dcousens dcousens marked this pull request as ready for review August 19, 2024 23:16
"@keystone-6/document-renderer": "1.1.2",
"@keystone-6/fields-document": "5.0.2"
},
"database": "postgresql",
Copy link
Member Author

Choose a reason for hiding this comment

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

@DavidMulder0 as in #9290, with this missed in our telemetry policy - I will set this field to NULL for any remaining telemetry data that is to be aggregated.

UPDATE ProjectEvent SET database = NULL;

id?: import('./node_modules/.myprisma/client').Prisma.PostCreateInput['id']
title?: import('./node_modules/.myprisma/client').Prisma.PostCreateInput['title']
content?: import('./node_modules/.myprisma/client').Prisma.PostCreateInput['content']
publishDate?: import('./node_modules/.myprisma/client').Prisma.PostCreateInput['publishDate']
Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated, but this is problematic in different situations, only included in this pull request to reduce conflicts


// don't send telemetry before we inform the user, allowing opt-out
if (!telemetry.informedAt) return inform()
const telemetryDefaulted = getDefault(telemetry)
if (!telemetryDefaulted.informedAt) return inform(telemetryDefaulted, userConfig)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest adding an explicit log here to remind that telemetry is active:

    console.log('🛎️  Telemetry is enabled')

image

Copy link
Member Author

@dcousens dcousens Aug 20, 2024

Choose a reason for hiding this comment

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

This is pretty neat, what about showing it is disabled too?

    console.log('🛎️  Telemetry is enabled')
    // or
    console.log('✅  Telemetry is enabled')

and

    console.log('❎  Telemetry is disabled')

Copy link
Member Author

@dcousens dcousens Aug 20, 2024

Choose a reason for hiding this comment

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

At some stage I want to switch to declarative wording here rather than procedural, but we won't do that in this pull request

Example is Database unchanged versus Connecting to the database

Copy link
Member Author

@dcousens dcousens Aug 20, 2024

Choose a reason for hiding this comment

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

@kennedybaird can we do this in a different pull request?
I want to merge and release this and #9290 as soon as possible

The suggested improvement might require a new set of wiring and functions, depending on how we do this

Copy link
Member Author

@dcousens dcousens Aug 20, 2024

Choose a reason for hiding this comment

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

And, like in #8118 (see #8118 (comment)), the phrasing of this might resurface whether we want to try and manage our dependencies telemetry configuration too

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@kennedybaird
Copy link
Contributor

Added a couple of thoughts @dcousens - nice to see you jumping on this!

The only other comment is the "Package Unit Tests" failing, not sure exactly what's happening there.

@dcousens
Copy link
Member Author

Fixed the Package Unit Tests in cd18302, jest isn't happy with dynamic imports, the same problem with #8820

@dcousens dcousens merged commit 19a734c into main Aug 20, 2024
42 of 43 checks passed
@dcousens dcousens deleted the telemetry-2 branch August 20, 2024 01:15
@dcousens dcousens mentioned this pull request Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants