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 3 commits into
base: development
Choose a base branch
from
Open
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
8 changes: 8 additions & 0 deletions .github/workflows/review-requested-slack-notification.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
name: PR review requested Slack notification
on:
pull_request_target:
types: [review_requested]
jobs:
requested:
uses: NomicFoundation/github-actions-workflows/.github/workflows/review-requested-slack-notification.yml@main
secrets: inherit
8 changes: 8 additions & 0 deletions .github/workflows/review-submitted-slack-notification.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
name: PR reviewed Slack notification
on:
pull_request_review:
types: [submitted]
jobs:
reviewed:
uses: NomicFoundation/github-actions-workflows/.github/workflows/review-submitted-slack-notification.yml@main
secrets: inherit
16 changes: 6 additions & 10 deletions client/src/popups/showAnalyticsAllowPopup.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,12 @@
import {
workspace,
window,
ExtensionContext,
ConfigurationTarget,
ExtensionMode,
} from "vscode";
import { workspace, window, ConfigurationTarget, ExtensionMode } from "vscode";
import { ExtensionState } from "../types";

const PREVIOUSLY_SHOWN_TELEMETRY_LABEL = "previouslyShownTelemetryMessage";

export async function showAnalyticsAllowPopup({
context,
}: {
context: ExtensionContext;
}): Promise<void> {
client,
}: ExtensionState): Promise<void> {
if (context.extensionMode === ExtensionMode.Test) {
// Dialog messages are prohibited in tests:
// https://github.com/microsoft/vscode/blob/36fefc828e4c496a7bbb64c63f3eb3052a650f8f/src/vs/workbench/services/dialogs/common/dialogService.ts#L56
Expand Down Expand Up @@ -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.

28 changes: 15 additions & 13 deletions docs/publish-extension.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,24 +31,26 @@ release.
```

9. Push the release branch and open a pull request using the new changelog entry as the PR description
10. Generate a release candidate vsix file with `npm run package`, the vsix file should appear in the `./client` folder with the new version number

> WARNING: ensure .env file is populated with GA and Sentry secrets before packaging (see `./env.example`)
10. Ensure .env file is populated with GA and Sentry secrets before packaging (see `./env.example`)

11. Manually run smoke tests on the new features across:
11. Generate a release candidate vsix file with `npm run package`, the vsix file should appear in the `./client` folder with the new version number

12. Manually run smoke tests on the new features across:

- mac os x
- windows
- vscode running against docker

12. On a successful check, `rebase merge` the release branch into main
13. Switch to main branch and pull the latest changes
14. Git tag the version, `g tag -a v0.x.x -m "v0.x.x"` and push the tag `git push --follow-tags`
15. Publish the language server npm package, `cd ./server && npm publish`
16. Publish the coc extension, `cd ./coc && npm publish --non-interactive`
17. Upload the vsix file to the microsoft marketplace: `npx vsce publish -p $VSCE_TOKEN --packagePath client/hardhat-solidity-0.X.X.vsix`
18. Upload the vsix file to openvsx, `npx ovsx publish client/hardhat-solidity-0.X.X.vsix -p $OVSX_TOKEN`
19. Create a release on github off of the pushed tag
13. Ensure that metrics are reported correctly in both Google Analytics and Sentry for the new version.
14. On a successful check, `rebase merge` the release branch into main
15. Switch to main branch and pull the latest changes
16. Git tag the version, `git tag -a v0.x.x -m "v0.x.x"` and push the tag `git push --follow-tags`
17. Publish the language server npm package, `cd ./server && npm publish`
18. Publish the coc extension, `cd ./coc && npm publish --non-interactive`
19. Upload the vsix file to the microsoft marketplace: `npx vsce publish -p $VSCE_TOKEN --packagePath client/hardhat-solidity-0.X.X.vsix`
20. Upload the vsix file to openvsx, `npx ovsx publish client/hardhat-solidity-0.X.X.vsix -p $OVSX_TOKEN`
21. Create a release on github off of the pushed tag

- use the added changelog section as the body of the release
- append the Nomic is Hiring section to the end of the release not:
Expand All @@ -61,8 +63,8 @@ release.

- upload the vsix file as an asset

18. Rebase `development` onto `main`, and force push back to github
19. Update the discord announcements channel
22. Rebase `development` onto `main`, and force push back to github
23. Update the discord announcements channel

- link to the release entry on github (i.e. `https://github.com/NomicFoundation/hardhat-vscode/releases/tag/v0.x.x`)
- give a few sentences of description of why users should be excited about this release
59 changes: 57 additions & 2 deletions server/src/analytics/GoogleAnalytics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { ServerState } from "../types";
import { Analytics, AnalyticsPayload } from "./types";

const GA_URL = "https://www.google-analytics.com/mp/collect";
const TELEMETRY_USER_ID = "hh_vscode_telemetry_consent";

export class GoogleAnalytics implements Analytics {
private readonly measurementID: string;
Expand Down Expand Up @@ -47,7 +48,7 @@ export class GoogleAnalytics implements Analytics {
return;
}

const payload = this._buildPayloadFrom(taskName, this.machineId);
const payload = this._buildTaskPayload(taskName, this.machineId);

await this._sendHit(payload);
} catch {
Expand All @@ -56,7 +57,29 @@ export class GoogleAnalytics implements Analytics {
}
}

private _buildPayloadFrom(
// Meant for the initial response to the telemetry consent popup
public async sendTelemetryResponse(userConsent: boolean): Promise<void> {
try {
const payload = this._buildTelemetryResponsePayload(userConsent);

await this._sendHit(payload);
} catch {
return;
}
}

// Meant for subsequent changes to the telemetry setting
public async sendTelemetryChange(userConsent: boolean): Promise<void> {
try {
const payload = this._buildTelemetryChangePayload(userConsent);

await this._sendHit(payload);
} catch {
return;
}
}

private _buildTaskPayload(
taskName: string,
machineId: string
): AnalyticsPayload {
Expand All @@ -80,6 +103,38 @@ export class GoogleAnalytics implements Analytics {
};
}

private _buildTelemetryResponsePayload(userConsent: boolean) {
return {
client_id: TELEMETRY_USER_ID,
user_id: TELEMETRY_USER_ID,
user_properties: {},
events: [
{
name: "TelemetryConsentResponse",
params: {
userConsent: userConsent ? "yes" : "no",
},
},
],
};
}

private _buildTelemetryChangePayload(userConsent: boolean) {
return {
client_id: TELEMETRY_USER_ID,
user_id: TELEMETRY_USER_ID,
user_properties: {},
events: [
{
name: "TelemetryConsentChange",
params: {
userConsent: userConsent ? "yes" : "no",
},
},
],
};
}

private _sendHit(payload: AnalyticsPayload) {
return got.post(GA_URL, {
headers: {
Expand Down
29 changes: 17 additions & 12 deletions server/src/analytics/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,23 +6,26 @@ export interface AnalyticsPayload {
client_id: string;
user_id: string;

user_properties: {
extensionVersion: {
value?: string;
};
languageClient: {
value?: string;
};
operatingSystem: {
value?: string;
};
};
user_properties:
| {
extensionVersion: {
value?: string;
};
languageClient: {
value?: string;
};
operatingSystem: {
value?: string;
};
}
| {};

events: Array<{
name: string;
params: {
engagement_time_msec: string;
engagement_time_msec?: string;
session_id?: string;
userConsent?: string;
};
}>;
}
Expand All @@ -36,4 +39,6 @@ export interface Analytics {
): void;

sendPageView(taskName: string): Promise<void>;
sendTelemetryResponse(userConsent: boolean): Promise<void>;
sendTelemetryChange(userConsent: boolean): Promise<void>;
}
7 changes: 6 additions & 1 deletion server/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,14 +109,15 @@ function attachCustomHooks(serverState: ServerState) {

connection.onNotification(
"custom/didChangeTelemetryEnabled",
({ enabled }: { enabled: boolean }) => {
async ({ enabled }: { enabled: boolean }) => {
if (enabled) {
logger.info(`Telemetry enabled`);
} else {
logger.info(`Telemetry disabled`);
}

serverState.telemetryEnabled = enabled;
await serverState.telemetry.analytics.sendTelemetryChange(enabled);
}
);

Expand All @@ -126,4 +127,8 @@ function attachCustomHooks(serverState: ServerState) {
serverState.extensionConfig = extensionConfig;
}
);

connection.onNotification("custom/telemetryConsent", async (payload) => {
await serverState.telemetry.analytics.sendTelemetryResponse(payload);
});
}
2 changes: 1 addition & 1 deletion server/src/telemetry/SentryServerTelemetry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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

public analytics: Analytics;
private actionTaken: boolean;
private heartbeatInterval: NodeJS.Timeout | null;
private heartbeatPeriod: number;
Expand Down
3 changes: 3 additions & 0 deletions server/src/telemetry/types.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
import { SpanStatusType } from "@sentry/tracing";
import type { Primitive, Transaction } from "@sentry/types";
import { ServerState } from "../types";
import { Analytics } from "../analytics/types";

export interface TrackingResult<T> {
status: SpanStatusType;
result: T | null;
}

export interface Telemetry {
analytics: Analytics;

init(
machineId: string | undefined,
extensionName: string | undefined,
Expand Down
2 changes: 2 additions & 0 deletions server/test/helpers/setupMockAnalytics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,7 @@ export function setupMockAnalytics(): Analytics {
return {
init: sinon.spy(),
sendPageView: sinon.spy(),
sendTelemetryResponse: sinon.spy(),
sendTelemetryChange: sinon.spy(),
};
}
2 changes: 2 additions & 0 deletions server/test/helpers/setupMockTelemetry.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import { Transaction } from "@sentry/types";
import * as sinon from "sinon";
import { Telemetry } from "../../src/telemetry/types";
import { setupMockAnalytics } from "./setupMockAnalytics";

export function setupMockTelemetry(): Telemetry {
return {
init: sinon.spy(),
captureException: sinon.spy(),
analytics: setupMockAnalytics(),
trackTiming: async (_taskName: string, action) => {
try {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Expand Down
Loading