diff --git a/cmd/frontend/auth/user.go b/cmd/frontend/auth/user.go index 0627be90a38..aca2193c4f9 100644 --- a/cmd/frontend/auth/user.go +++ b/cmd/frontend/auth/user.go @@ -260,38 +260,43 @@ func GetAndSaveUser( // We handle all V2 telemetry related to GetAndSaveUser within this defer // closure, to ensure we cover all exit paths correctly after the other mega // closure above. - defer func() { - action := telemetry.ActionSucceeded - if err != nil { // check final error - action = telemetry.ActionFailed - } + // + // We only store the event if a new user was created, or if a new external + // account was added to an existing user. + if newUserSaved || extAcctSaved { + defer func() { + action := telemetry.ActionSucceeded + if err != nil { // check final error + action = telemetry.ActionFailed + } - // Most auth providers services have an exstvc.Variant, so try and - // extract that from the account spec. For ease of use in we also - // preserve the raw value in the private metadata. - serviceVariant, _ := extsvc.VariantValueOf(acct.AccountSpec.ServiceType) - privateMetadata := map[string]any{"serviceType": acct.AccountSpec.ServiceType} + // Most auth providers services have an exstvc.Variant, so try and + // extract that from the account spec. For ease of use in we also + // preserve the raw value in the private metadata. + serviceVariant, _ := extsvc.VariantValueOf(acct.AccountSpec.ServiceType) + privateMetadata := map[string]any{"serviceType": acct.AccountSpec.ServiceType} - // Include safe err if there is one for maybe-useful diagnostics - if len(safeErrMsg) > 0 { - privateMetadata["safeErrMsg"] = safeErrMsg - } + // Include safe err if there is one for maybe-useful diagnostics + if len(safeErrMsg) > 0 { + privateMetadata["safeErrMsg"] = safeErrMsg + } - // Record our V2 event. - recorder.Record(telemetryCtx, telemetryV2UserSignUpFeatureName, action, &telemetry.EventParameters{ - Version: 2, // We've significantly refactored telemetryV2UserSignUpFeatureName occurrences - Metadata: telemetry.MergeMetadata( - telemetry.EventMetadata{ - "serviceVariant": telemetry.Number(serviceVariant), - // Track the various outcomes of the massive signup closure above. - "newUserSaved": telemetry.Bool(newUserSaved), - "extAcctSaved": telemetry.Bool(extAcctSaved), - }, - op.UserCreateEventProperties, - ), - PrivateMetadata: privateMetadata, - }) - }() + // Record our V2 event. + recorder.Record(telemetryCtx, telemetryV2UserSignUpFeatureName, action, &telemetry.EventParameters{ + Version: 2, // We've significantly refactored telemetryV2UserSignUpFeatureName occurrences + Metadata: telemetry.MergeMetadata( + telemetry.EventMetadata{ + "serviceVariant": telemetry.Number(serviceVariant), + // Track the various outcomes of the massive signup closure above. + "newUserSaved": telemetry.Bool(newUserSaved), + "extAcctSaved": telemetry.Bool(extAcctSaved), + }, + op.UserCreateEventProperties, + ), + PrivateMetadata: privateMetadata, + }) + }() + } if err != nil { // Legacy event - retain because it is still exported by the legacy diff --git a/cmd/frontend/auth/user_test.go b/cmd/frontend/auth/user_test.go index 641278f3d5d..9ab3992f37b 100644 --- a/cmd/frontend/auth/user_test.go +++ b/cmd/frontend/auth/user_test.go @@ -43,6 +43,7 @@ func TestGetAndSaveUser(t *testing.T) { expSafeErr string expErr error expNewUserCreated bool + expExtAcctSaved bool // expected side effects expSavedExtAccts map[int32][]extsvc.AccountSpec @@ -122,6 +123,7 @@ func TestGetAndSaveUser(t *testing.T) { 1: {ext("st1", "s1", "c1", "s1/u1")}, }, expNewUserCreated: false, + expExtAcctSaved: true, }, { description: "ext acct exists, username and email don't exist", @@ -137,6 +139,7 @@ func TestGetAndSaveUser(t *testing.T) { 1: {ext("st1", "s1", "c1", "s1/u1")}, }, expNewUserCreated: false, + expExtAcctSaved: true, }, { description: "ext acct exists, email belongs to another user", @@ -152,6 +155,7 @@ func TestGetAndSaveUser(t *testing.T) { 1: {ext("st1", "s1", "c1", "s1/u1")}, }, expNewUserCreated: false, + expExtAcctSaved: true, }, { description: "ext acct doesn't exist, user with username and email exists", @@ -507,9 +511,13 @@ func TestGetAndSaveUser(t *testing.T) { // All telemetry should have the expected user (or lack // of user) attached, and all code paths should generate - // at least 1 user event. + // at least 1 user event if a new user was created. gotEvents := eventsStore.CollectStoredEvents() - assert.NotEmpty(t, gotEvents) + if c.expNewUserCreated || c.expExtAcctSaved { + assert.NotEmpty(t, gotEvents) + } else { + assert.Empty(t, gotEvents) + } for _, ev := range gotEvents { switch { // We are expecting a specific user ID