-
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
Conversation
d3a0093
to
4ae40ce
Compare
4ae40ce
to
387f94b
Compare
38b1b2b
to
438c94a
Compare
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.
Some minor questions, good for merge independently imo.
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-]+/; |
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
}); | ||
// 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 comment
The 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 comment
The 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 vscodesessionid
with the common properties in data, and the segmentAnonId
is stable in each instance of the TelemetryLogger, so it won't make much difference now but in updates I'll be able to reset the anon id (during log out flow or other cases where the User Id might be new for an existing VSCode session)
Summary of Changes
getTelemetryLogger
from the other functions to make testing easier (since mocking module-level functions is still a challenge)Any additional details or context that should be provided?
domain
property instead ofemail
Pull request checklist
Please check if your PR fulfills the following (if applicable):
Tests
Other
.vsix
file?