-
Notifications
You must be signed in to change notification settings - Fork 234
fix: send atlas uid as userId to compass telemetry COMPASS-7610 #5441
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
Conversation
e018193
to
3f4fc60
Compare
3f4fc60
to
7d0a431
Compare
7d0a431
to
59b58b0
Compare
1a2689b
to
ba72113
Compare
bbeb7e9
to
6f8d064
Compare
); | ||
}); | ||
|
||
it('tracks an event for identify call', function () { |
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'm not very familiar with this work so maybe I'm confused, but aren't we losing a test for the non-atlas-login case if you remove this?
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.
identify will no longer be called until we know the userId, this was requested by Ale (see the parent ticket https://jira.mongodb.org/browse/COMPASS-7519)
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.
Won't we lose telemetry for all non-atlas users then? I clearly don't know how that stuff works 😆
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.
@paula-stacho you are right the initial ask was to only identify
in that case, but we cannot do that just yet, we would lose some of the attributes that we send via identify currently (although we should send them as regular attributes with events). We may need a follow up ticket to do that.
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.
ah I see! yes, we'd loose the "traits". there are no "traits" in segment track calls, and adding them to the properties would probably require changes in the processing.
I can bring back the "no userId" identify and it's e2e test, and create a follow up ticket. who owns the data in segment and post-processing? is it some other team?
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.
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.
Looks good in as far as I understand what this code should be doing :)
https://jira.mongodb.org/browse/COMPASS-7610
Description
Main changes:
telemetryAtlasUserId
is stored in the preferencesuserId
for telemetry calls (track
andidentify
)identify
call is madeidentify
call is only send if theuserId = telemetryAtlasUserId
is knownOn Logging out:
When the user logs out, we don't clean up the
telemetryAtlasUserId
. this means subsequent identify calls might still be made and they will include the last knowntelemetryAtlasUserId
. I discussed this with @gribnoysup and he explained such cleanup would be superfluous since the two ids (anonymous and auid) are anyway coupled in the records by then.On tests:
I didn't find any existing telemetry unit or integration tests, and they're not simple/straightforward to add as it needs the CompassApp to init, so I skipped it for now.
Coming from jest, I had some trouble mocking the
getTrackingUserInfo
for thesignIn
test. In the end I found a way, but I must say I'm surprised that it's necessary to runsandbox.restore();
after the test suite. Without this, thegetTrackingUserInfo
was still affected even though it is now in a separate test suite. Is this normal or am I using Sinon in a wrong way?As the e2e scenario for the first identify is now a combination of atlas login and telemetry, I had to make a choice on which suite should have it. The chosen way seemed leaner.
The subsequent run is more contained, so it's left in the logging suite. We just need to assume that
telemetryAtlasUserId
is already in the preferences (simulating that the user has signed in some previous run).Checklist
Motivation and Context
Types of changes