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

chore: report telemetry consent #540

Open
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

antico5
Copy link
Collaborator

@antico5 antico5 commented Jan 29, 2024

This PR introduces two events sent to Google Analytics, without any client information, for when the user first responds to the "enable telemetry" popup, and any subsequent change to the setting via UI.

@codecov-commenter
Copy link

codecov-commenter commented Jan 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.56%. Comparing base (e5372f3) to head (c1bc8be).

Additional details and impacted files
@@               Coverage Diff               @@
##           development     #540      +/-   ##
===============================================
+ Coverage        54.52%   54.56%   +0.03%     
===============================================
  Files              226      226              
  Lines             5199     5203       +4     
  Branches           800      800              
===============================================
+ Hits              2835     2839       +4     
  Misses            2127     2127              
  Partials           237      237              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@Xanewok Xanewok left a comment

Choose a reason for hiding this comment

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

We're specifically anonymizing the consent response/change and that's why we're using a hardcoded user ID, right? That's just to gauge how many people are responding Y/N?

If the data reported is anonymous anyway, wouldn't we want to already know who changed the consent? We can sort of already understand who agreed (by inspecting the machine ID); this just prevents us from seeing when some machine ID stopped consenting (which is fine but wanted to bring it up).

Otherwise, looks good to me!

@antico5
Copy link
Collaborator Author

antico5 commented May 2, 2024

We're specifically anonymizing the consent response/change and that's why we're using a hardcoded user ID, right? That's just to gauge how many people are responding Y/N?

If the data reported is anonymous anyway, wouldn't we want to already know who changed the consent? We can sort of already understand who agreed (by inspecting the machine ID); this just prevents us from seeing when some machine ID stopped consenting (which is fine but wanted to bring it up).

Otherwise, looks good to me!

My understanding is that we want to anonymize this particular response because if the user answers no, we don't want to store their machine ID. Also for analytics purposes we only care about the number of users and not about what a specific user responded.

@antico5 antico5 force-pushed the telemetry_consent_reporting branch from b183cc0 to 4b5d843 Compare May 2, 2024 11:52
Copy link
Contributor

@OmarTawfik OmarTawfik left a comment

Choose a reason for hiding this comment

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

Left one question.
But I think @kanej would be able to review this better.
Thanks!

@@ -44,4 +38,6 @@ export async function showAnalyticsAllowPopup({
await config.update("telemetry", isAccepted, ConfigurationTarget.Global);

await context.globalState.update(PREVIOUSLY_SHOWN_TELEMETRY_LABEL, true);

client?.sendNotification("custom/telemetryConsent", isAccepted);
Copy link
Contributor

Choose a reason for hiding this comment

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

question: I wonder if this change will record the consent status for people who already gave (or rejected) telemetry before? would we collect this data for older users?

Copy link
Member

Choose a reason for hiding this comment

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

My reading of the code is that people who have previously made a decision on telemetry will not sendNotification to Google Analytics. It is only new people going through the complete process who will send this notification.

@kanej kanej force-pushed the telemetry_consent_reporting branch from 4b5d843 to efb3280 Compare June 11, 2024 10:51
@@ -13,7 +13,7 @@ const SENTRY_CLOSE_TIMEOUT = 2000;
export class SentryServerTelemetry implements Telemetry {
private dsn: string;
private serverState: ServerState | null;
private analytics: Analytics;
Copy link
Member

Choose a reason for hiding this comment

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

I get the move here, but I suspect we should either:

  1. Add analytics as a property of the serverState and call it directly (SentryTelemetry used to be the only place analytics was used, that is no longer the case). Though this likely means reworking init and heartbeat.
  2. Do this analytics changes on the client

@antico5 antico5 force-pushed the telemetry_consent_reporting branch from efb3280 to 2b16f4b Compare February 25, 2025 13:53
@antico5
Copy link
Collaborator Author

antico5 commented Feb 25, 2025

@kanej just rebased this. I noticed there is still a pending comment on moving the analytics object to serverState instead of having it under Telemetry. Is that still relevant?

@kanej
Copy link
Member

kanej commented Feb 26, 2025

@kanej just rebased this. I noticed there is still a pending comment on moving the analytics object to serverState instead of having it under Telemetry. Is that still relevant?

I think so, happy to review, but I think we move analytics into the server state rather than making it a public property.

Lets bring this PR up to date, so we can include it when we refresh the privacy policy.

@antico5 antico5 force-pushed the telemetry_consent_reporting branch from 3754b7d to c1bc8be Compare February 26, 2025 18:39
@antico5
Copy link
Collaborator Author

antico5 commented Feb 26, 2025

@kanej done! ready for review

@antico5 antico5 requested a review from kanej February 28, 2025 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

5 participants