-
Notifications
You must be signed in to change notification settings - Fork 4
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 Segment identify calls to remove email, add domain #405
Changes from all commits
1793013
ce8247e
387f94b
438c94a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,112 @@ | ||
import { sendTelemetryIdentifyEvent } from "./telemetry"; | ||
import * as vscode from "vscode"; | ||
import * as telemetry from "./telemetryLogger"; | ||
import Sinon from "sinon"; | ||
|
||
describe("sendTelemetryIdentifyEvent", () => { | ||
let mockTelemetryLogger: any; | ||
let logUsageStub: Sinon.SinonStub; | ||
let sdbx: sinon.SinonSandbox; | ||
|
||
beforeEach(() => { | ||
sdbx = Sinon.createSandbox(); | ||
mockTelemetryLogger = { | ||
logUsage: sdbx.stub(), | ||
}; | ||
sdbx.stub(telemetry, "getTelemetryLogger").returns(mockTelemetryLogger); | ||
logUsageStub = mockTelemetryLogger.logUsage; | ||
}); | ||
|
||
afterEach(() => { | ||
sdbx.restore(); | ||
}); | ||
|
||
it("should send to logUsage with correct user info and domain", () => { | ||
const userInfo = { | ||
id: "user123", | ||
username: "user@mycooldomain.com", | ||
social_connection: "github", | ||
}; | ||
const session = undefined; | ||
|
||
sendTelemetryIdentifyEvent({ | ||
eventName: "testEvent", | ||
userInfo, | ||
session, | ||
}); | ||
|
||
Sinon.assert.called(logUsageStub); | ||
Sinon.assert.calledWith(logUsageStub, "testEvent", { | ||
identify: true, | ||
user: { | ||
id: "user123", | ||
domain: "mycooldomain.com", | ||
social_connection: "github", | ||
}, | ||
}); | ||
}); | ||
it("should send to logUsage with correct session info and domain", () => { | ||
const userInfo = undefined; | ||
const session = { | ||
account: { | ||
id: "session123", | ||
label: "session@example.com", | ||
}, | ||
} as vscode.AuthenticationSession; | ||
|
||
sendTelemetryIdentifyEvent({ | ||
eventName: "testEvent", | ||
userInfo, | ||
session, | ||
}); | ||
|
||
Sinon.assert.called(logUsageStub); | ||
Sinon.assert.calledWith(logUsageStub, "testEvent", { | ||
identify: true, | ||
user: { | ||
id: "session123", | ||
domain: "example.com", | ||
social_connection: undefined, | ||
}, | ||
}); | ||
}); | ||
it("should not send to logUsage if user id is not available", () => { | ||
const userInfo = { | ||
id: undefined, | ||
username: "user@example.com", | ||
social_connection: "github", | ||
}; | ||
const session = undefined; | ||
|
||
sendTelemetryIdentifyEvent({ | ||
eventName: "testEvent", | ||
userInfo, | ||
session, | ||
}); | ||
|
||
Sinon.assert.notCalled(logUsageStub); | ||
}); | ||
it("should send event, no domain if username doesn't match email regex", () => { | ||
const userInfo = { | ||
id: "user1", | ||
username: "glitchintheusername", | ||
social_connection: "github", | ||
}; | ||
const session = undefined; | ||
|
||
sendTelemetryIdentifyEvent({ | ||
eventName: "testEvent", | ||
userInfo, | ||
session, | ||
}); | ||
|
||
Sinon.assert.calledWith(logUsageStub, "testEvent", { | ||
identify: true, | ||
user: { | ||
id: "user1", | ||
domain: undefined, | ||
social_connection: "github", | ||
}, | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
import * as vscode from "vscode"; | ||
import { UserInfo } from "../clients/sidecar/models/UserInfo"; | ||
import { getTelemetryLogger } from "./telemetryLogger"; | ||
import * as Sentry from "@sentry/node"; | ||
|
||
/** Given authenticated session and/or userInfo, clean the data & send an Identify event to Segment via TelemetryLogger | ||
* @param eventName - The event name to be sent to Segment as a follow up per docs: "follow the Identify call with a Track event that records what caused the user to be identified" | ||
* @param userInfo - The UserInfo object from the Authentiation event | ||
* @param session - The vscode.AuthenticationSession object from an existing session | ||
* ``` | ||
* sendTelemetryIdentifyEvent({eventName: "Event That Triggered Identify", userInfo: { id: "123", ...} });" | ||
* ``` | ||
*/ | ||
export function sendTelemetryIdentifyEvent({ | ||
eventName, | ||
userInfo, | ||
session, | ||
}: { | ||
eventName: string; | ||
userInfo: UserInfo | undefined; | ||
session: vscode.AuthenticationSession | undefined; | ||
}) { | ||
const id = userInfo?.id || session?.account.id; | ||
const username = userInfo?.username || session?.account.label; | ||
const social_connection = userInfo?.social_connection; | ||
let domain: string | undefined; | ||
if (username) { | ||
// email is redacted by VSCode TelemetryLogger, but we extract domain for Confluent analytics use | ||
const emailRegex = /@[a-zA-Z0-9-]+\.[a-zA-Z0-9-]+/; | ||
const match = username.match(emailRegex); | ||
if (match) { | ||
domain = username.split("@")[1]; | ||
} | ||
} | ||
if (id) { | ||
getTelemetryLogger().logUsage(eventName, { | ||
identify: true, | ||
user: { id, domain, social_connection }, | ||
}); | ||
} | ||
} | ||
|
||
/** Helper function to make sure the user has Telemetry ON before sending Sentry error events */ | ||
export function checkTelemetrySettings(event: Sentry.Event) { | ||
const telemetryLevel = vscode.workspace.getConfiguration()?.get("telemetry.telemetryLevel"); | ||
if (!vscode.env.isTelemetryEnabled || telemetryLevel === "off") { | ||
// Returning `null` will drop the event | ||
return null; | ||
} | ||
return event; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,8 @@ | ||
import { Analytics } from "@segment/analytics-node"; | ||
import * as Sentry from "@sentry/node"; | ||
import { randomUUID } from "crypto"; | ||
import { version as currentSidecarVersion } from "ide-sidecar"; | ||
import * as vscode from "vscode"; | ||
import { Logger } from "./logging"; | ||
import { Logger } from "../logging"; | ||
// TEMP keep this import here to make sure the production bundle doesn't split chunks | ||
import "opentelemetry-instrumentation-fetch-node"; | ||
|
||
|
@@ -30,12 +29,7 @@ let warnedAboutSegmentKey = false; | |
* Use Proper Case, Noun + Past Tense Verb to represent the user's action (e.g. "Order Completed", "File Downloaded", "User Registered") | ||
* Optionally, add any relevant data as the second parameter | ||
* | ||
* For IDENTIFY calls - add the "identify" key to the data along with a user object (with at least an id) to send an identify type call. | ||
* ``` | ||
* getTelemetryLogger().logUsage("Event That Triggered Identify", { identify: true, user: { id: "123", ...} });" | ||
* ``` | ||
* It will send an Identify call followed by a Track event per this Segment recommendation: | ||
* "Whenever possible, follow the Identify call with a Track event that records what caused the user to be identified." | ||
* For IDENTIFY calls - use sendTelemetryIdentifyEvent from telemetry.ts instead | ||
*/ | ||
export function getTelemetryLogger(): vscode.TelemetryLogger { | ||
// If there is already an instance of the Segment Telemetry Logger, return it | ||
|
@@ -65,25 +59,28 @@ export function getTelemetryLogger(): vscode.TelemetryLogger { | |
} | ||
analytics = new Analytics({ writeKey, disable: false }); | ||
} | ||
// We extract the vscode session ID from the event data, but this random id will be sent if it is undefined (unlikely but not guranteed by the type def) | ||
|
||
segmentAnonId = randomUUID(); | ||
|
||
telemetryLogger = vscode.env.createTelemetryLogger({ | ||
sendEventData: (eventName, data) => { | ||
const cleanEventName = eventName.replace(/^confluentinc\.vscode-confluent\//, ""); // Remove the prefix that vscode adds to event names | ||
// Remove the prefix that vscode adds to event names | ||
const cleanEventName = eventName.replace(/^confluentinc\.vscode-confluent\//, ""); | ||
// Extract & save the user id if was sent | ||
if (data?.user?.id) userId = data.user.id; | ||
if (data?.identify && userId) { | ||
if (data?.identify && data?.user) { | ||
analytics?.identify({ | ||
userId, | ||
anonymousId: data?.["common.vscodesessionid"] || segmentAnonId, | ||
traits: { email: data?.user?.username, social_connection: data?.user?.social_connection }, | ||
anonymousId: segmentAnonId, | ||
traits: { ...data.user }, | ||
}); | ||
// We don't want to send the user traits or identify prop in the following Track call | ||
delete data.identify; | ||
delete data.user; | ||
} | ||
analytics?.track({ | ||
userId, | ||
anonymousId: data?.["common.vscodesessionid"] || segmentAnonId, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just setting us up to handle logout in near future... We're already sending |
||
anonymousId: segmentAnonId, | ||
event: cleanEventName, | ||
properties: { currentSidecarVersion, ...data }, // VSCode Common properties in data includes the extension version | ||
}); | ||
|
@@ -98,12 +95,3 @@ export function getTelemetryLogger(): vscode.TelemetryLogger { | |
|
||
return telemetryLogger; | ||
} | ||
|
||
export function checkTelemetrySettings(event: Sentry.Event) { | ||
const telemetryLevel = vscode.workspace.getConfiguration()?.get("telemetry.telemetryLevel"); | ||
if (!vscode.env.isTelemetryEnabled || telemetryLevel === "off") { | ||
// Returning `null` will drop the event | ||
return null; | ||
} | ||
return event; | ||
} |
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'd of maybe just done if
@
present, then split and grab[1]
since we don't care about other properties and expect it to always be an email entry anyway.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.
does sound simpler! I wasn't sure if we were 100% certain this will be email every time. Happy to update if it makes sense to do so in follow ups