[fix] Only trigger externalAcctSignup event when a new user is created#63843
Conversation
eseliger
left a comment
There was a problem hiding this comment.
I want Robert to review this, but code change in general LGTM, without knowing what the event was supposed to track 😬
| }() | ||
| // Record our V2 event. | ||
| recorder.Record(telemetryCtx, telemetryV2UserSignUpFeatureName, action, &telemetry.EventParameters{ | ||
| Version: 2, // We've significantly refactored telemetryV2UserSignUpFeatureName occurrences |
There was a problem hiding this comment.
| Version: 2, // We've significantly refactored telemetryV2UserSignUpFeatureName occurrences | |
| Version: 3, // We've significantly refactored telemetryV2UserSignUpFeatureName occurrences |
This can help the data team drop all the "old" version of the events
| telemetry.EventMetadata{ | ||
| "serviceVariant": telemetry.Number(serviceVariant), | ||
| // Track the various outcomes of the massive signup closure above. | ||
| "newUserSaved": telemetry.Bool(newUserSaved), |
There was a problem hiding this comment.
Do we still need newUserSaved in the metadata, if we only record the event when newUserSaved?
bobheadxi
left a comment
There was a problem hiding this comment.
Thanks for digging into this! Overall LGTM, but - doesn't this mean that we no longer record telemetry when you sign up with a new external account into an existing user?
i.e. I sign in with GitHub, then sign in with GitLab into the same user, we should now only get one event based on a reading of the code, and there will be no record of the second sign-in with GitLab. Maybe that's what we want anyway?
|
@bobheadxi we can do |
|
@pjlast what you suggest sounds good to me! I also don't know how much data we want from this specifically, so I defer to your judgement as to what is useful as owner of this stuff. You could also check in with someone from the data analytics team about the specific scenarios they might be interested in, as the last time I was in this code is because they were looking at one of the events emitted here |
…ast-src-461-bug-discovered-where-customers-on-v54x-with-http-auth-are
…ew user is created (#63975) Currently events are triggered whenever a user signs in with `http-header` auth. This is because of the `GetAndSaveUser` function always triggering an event. However, before the new telemetry events, these events were only created when a new user was created. This PR brings the new telemetry code in line with the old telemetry code to stop the massive amounts of spam caused by this event. Closes SRC-461 ## Test plan Adjust expected events in unit test. ## Changelog - Fixed an issue where the `http-header` auth would cause a massive amount of event logs spam <br> Backport cd65951 from #63843 Co-authored-by: Petri-Johan Last <petri.last@sourcegraph.com>
…ed (#64005) Follow-up on https://github.com/sourcegraph/sourcegraph/pull/63843 Based on comments from [this](https://sourcegraph.slack.com/archives/C04RG0JD8L9/p1721668767261719?thread_ts=1721661216.365709&cid=C04RG0JD8L9) Slack thread, it seems like the events causing the spam are ones where a new ext acct is saved without a user being created. So if we want to fix the spam we need to only save an event if a user was created. ## Test plan Test updated. ## Changelog <br> Backport 777c7a0 from #64004 Co-authored-by: Petri-Johan Last <petri.last@sourcegraph.com>
Currently events are triggered whenever a user signs in with
http-headerauth. This is because of theGetAndSaveUserfunction always triggering an event.However, before the new telemetry events, these events were only created when a new user was created.
This PR brings the new telemetry code in line with the old telemetry code to stop the massive amounts of spam caused by this event.
Closes SRC-461
Test plan
Adjust expected events in unit test.
Changelog
http-headerauth would cause a massive amount of event logs spam