feat: new telemetry events for connections #6018

paula-stacho · 2024-07-11T13:56:35Z

Description

Existing events

Connection Imported - when new connections are imported
added

connection_ids

New events

Connection Disconnected

connection_id

Connection Created - when a new connection is saved

connection_id
color (optional)

Connection Removed

connection_id

** UPDATE ** Removed active_connection_count and inactive_connection_count upon further discussion. We might add these later if needed.

Checklist

New tests and/or benchmarks are included
Documentation is changed or added --> https://docs.google.com/spreadsheets/d/1Wi7np8CooM0b3xPHZNd-iYst_DnKEo7FBswbnSXKoys/edit?gid=953290655#gid=953290655

Motivation and Context

Bugfix
New feature
Dependency update
Misc

Open Questions

Dependents

Types of changes

Backport Needed
Patch (non-breaking change which fixes an issue)
Minor (non-breaking change which adds functionality)
Major (fix or feature that would cause existing functionality to change)

paula-stacho · 2024-07-11T14:50:32Z

packages/compass-connections/src/hooks/use-connection-repository.ts

          return prevList;
        } else {
-          return nonFavoriteConnections;
+          return newNonFavoriteConnections;


it's not visible in the diff, but there are higher scope consts with the name favoriteConnections and nonFavoriteConnections. I noticed while exploring some other options for triggering the events.

mcasimir · 2024-07-12T09:20:12Z

it also make sense to save before and not after connecting

💯

The reason for this was to keep track of the history. Can we create a follow up ticket to clean this up? I'm not sure folks removing the FF will be aware of it.

mcasimir · 2024-07-12T09:21:39Z

Did you consider splitting the shouldSave change from the new telemetry events? Seems to be quite loosely related.

packages/compass-connection-import-export/src/hooks/use-import.ts

paula-stacho · 2024-07-12T11:43:12Z

it also make sense to save before and not after connecting

💯

The reason for this was to keep track of the history. Can we create a follow up ticket to clean this up? I'm not sure folks removing the FF will be aware of it.

that's a valid concern. what needs to be cleaned up here is the legacyShouldSaveConnection, I changed the name to mark it for the cleanup. we do have a general cleanup ticket https://jira.mongodb.org/browse/COMPASS-7906, but I do wonder if we're doing good enough job of marking everything 🙈 I can add a todo with a link to that ticket.

paula-stacho · 2024-07-12T11:57:11Z

Did you consider splitting the shouldSave change from the new telemetry events? Seems to be quite loosely related.

I encountered several issues, one with the duplication and another with saving twice. the first was a problem on it's own so it got a ticket. this one was just unnecessary, it only became problem for the tracking (due to the asynchronous nature of repository updates we couldn't reliably tell that the connection already exists when it was saved second time). still, I can make it a separate ticket if that makes it easier to review

mcasimir · 2024-07-15T11:28:15Z

add a todo with a link to that ticket

that sounds great, should we update that ticket with a todo list too?

packages/compass-connections/src/stores/connections-store.tsx

himanshusinghs

Looking good 👍 I left a small note but not a blocker.

packages/compass-connections/src/connections-manager.ts

packages/compass-connections/src/stores/connections-store.tsx

paula-stacho · 2024-07-15T11:55:36Z

add a todo with a link to that ticket

that sounds great, should we update that ticket with a todo list too?

making a complete list is not going to be that simple at this point, and rather than making an incomplete list I'd leave it for when the ticket is picked up.

packages/compass-connections/src/stores/connections-store.tsx

mcasimir · 2024-07-15T12:30:29Z

packages/compass/src/app/components/home.tsx

+      connectionInfo: ConnectionInfo | undefined,
+      activeAndInactiveConnectionsCount: { active: number; inactive: number }
+    ) => {
+      trackConnectionRemovedEvent(


Similar to my previous comment. I'm curious about what prompted this "callback" / "event-driven" style here.

It seems onDisconnected, onConnectionCreated, and onConnectionRemoved exclusively call the respective track functions and require adapting the calling point to surface additional information.

What is the benefit of this approach? From my perspective, this indirection might be obscuring the code's intention, making it harder to follow. I'm open to discuss this further of course, but i don't see much of an advantage to be doing this instead of just calling trackConnection[X]Event directly.

FWIW the only reason for that is that tracking requires desktop-only dependencies so had to stay away from the code shared with web, my proposal was to only pass the extra tracking info somehow to compass-connections instead of exposing the hook, but this was not done right away and instead we had a refactor ticket (that I can't really find anymore, I guess it was not linked here and maybe closed?)

Found it: we initially had https://jira.mongodb.org/browse/COMPASS-7740 opened for this, but it's assigned to me and closed. I don't really remember why it got resolved like that, but I'm guessing we decided that it will be just resolved with some other refactoring that still needs to happen?

yeah, that's probably a good guess. Though we probably thought you were doing this refactoring @gribnoysup :)

desktop-only dependencies so had to stay away from the code shared with web

Isn't that addressed by providing a different implementation of services?

packages/compass/src/app/utils/telemetry.ts

packages/compass-connections/src/stores/connections-store.tsx

gribnoysup · 2024-07-15T14:43:46Z

packages/compass/src/app/components/home.tsx

+      connectionInfo: ConnectionInfo | undefined,
+      activeAndInactiveConnectionsCount: { active: number; inactive: number }
+    ) => {
+      trackConnectionRemovedEvent(


FWIW the only reason for that is that tracking requires desktop-only dependencies so had to stay away from the code shared with web, my proposal was to only pass the extra tracking info somehow to compass-connections instead of exposing the hook, but this was not done right away and instead we had a refactor ticket (that I can't really find anymore, I guess it was not linked here and maybe closed?)

gribnoysup · 2024-07-15T15:14:28Z

packages/compass-connections/src/stores/connections-store.tsx

+
+      if (activeConnectionId === connectionInfo.id) {
+        // Update the active connection if it's currently selected.
+        dispatch({


Why other places that save info directly through saveConnectionInfo don't need to update this?

good question. I actually only moved this function and I haven't questioned this bit. I'd say this is a legacy thing, because in multiple connections the activeConnection is only used by the connection form, and the form is closed when we save. perhaps @himanshusinghs can confirm? if I'm right we could mark this for a cleanup

packages/compass-connections/src/stores/connections-store.tsx

paula-stacho · 2024-07-16T20:24:21Z

note: separated the legacy parameter change, as I need it for the Save&Connect update #6034

gribnoysup · 2024-07-30T09:14:30Z

packages/compass-connections/src/stores/connections-store.tsx

@@ -360,7 +366,11 @@ export function useConnections({
        Pick<ConnectionInfo, 'id'>


Not related to your changes, but I just noticed that I missed changing this type to PartialConnectionInfo that I added in the refactor, do you mind adjusting as a drive-by?

Also looking closer at this, I'm pretty sure we never actually pass the partial here, so maybe this special case type is not even needed. I copied this from previous implementation without digging too deep

actually we do pass partial in oidcUpdateSecrets

Ahhh, true, missed that! Hmmm, that method is internal to this hook logic though and we do have the whole connection info object passed to it so we probably can do the merge there ourselves? Totally no strong preference though, mostly saying this because I saw your discussion above about this whole partial / full info type and maybe if we want to remove the ambiguity, we can just refactor away the use-case for this partial type

after merging your changes I updated the tracking so that it uses the updated connectionInfo, so this is now resolved 🎉

packages/compass-connections/src/stores/connections-store.tsx

packages/compass/src/app/utils/telemetry.ts

gribnoysup

Looks good, two small comments, nothing major

packages/compass-connections/src/stores/connections-store.spec.tsx

github-actions bot added the feat label Jul 11, 2024

paula-stacho commented Jul 11, 2024

View reviewed changes

paula-stacho added the no release notes Fix or feature not for release notes label Jul 11, 2024

mcasimir reviewed Jul 12, 2024

View reviewed changes

packages/compass-connection-import-export/src/hooks/use-import.ts Outdated Show resolved Hide resolved

paula-stacho force-pushed the COMPASS-7724-3 branch from c131dbf to acf4e0a Compare July 15, 2024 08:25

paula-stacho added 14 commits July 15, 2024 11:25

fix: add active and inactive connections count to New Connection

d900ed3

add Connection Disconnected event

0fc227c

wip

28fb7d1

create remove

e996bab

cleanup

4195d55

first tests

32a82b1

add telemetry tests

0efb298

Connections Imported event

c04fec5

use existing event

5b08c43

rename files and cleanup

96bf8e3

test

922615b

more test

65e212d

.

5d7e1ff

check

742ae71

paula-stacho force-pushed the COMPASS-7724-3 branch from 90d65bb to 742ae71 Compare July 15, 2024 09:25

paula-stacho marked this pull request as ready for review July 15, 2024 09:31

mcasimir reviewed Jul 15, 2024

View reviewed changes

packages/compass-connections/src/stores/connections-store.tsx Outdated Show resolved Hide resolved

mcasimir reviewed Jul 15, 2024

View reviewed changes

packages/compass-connections/src/stores/connections-store.tsx Outdated Show resolved Hide resolved

himanshusinghs reviewed Jul 15, 2024

View reviewed changes

packages/compass-connections/src/connections-manager.ts Outdated Show resolved Hide resolved

mcasimir reviewed Jul 15, 2024

View reviewed changes

packages/compass-connections/src/stores/connections-store.tsx Outdated Show resolved Hide resolved

mcasimir reviewed Jul 15, 2024

View reviewed changes

packages/compass-connections/src/stores/connections-store.tsx Outdated Show resolved Hide resolved

mcasimir reviewed Jul 15, 2024

View reviewed changes

packages/compass-connections/src/stores/connections-store.tsx Outdated Show resolved Hide resolved

mcasimir reviewed Jul 15, 2024

View reviewed changes

..

0c5e334

mcasimir reviewed Jul 15, 2024

View reviewed changes

packages/compass/src/app/utils/telemetry.ts Outdated Show resolved Hide resolved

gribnoysup reviewed Jul 15, 2024

View reviewed changes

paula-stacho added the blocked Waiting for something before merging/reviewing further label Jul 16, 2024

paula-stacho added wip and removed blocked Waiting for something before merging/reviewing further labels Jul 18, 2024

paula-stacho added 3 commits July 18, 2024 14:37

cleanup parameters

e01c6be

more cleanup

8f89092

Merge remote-tracking branch 'origin' into COMPASS-7724-3

2101f86

paula-stacho removed the wip label Jul 29, 2024

paula-stacho added 3 commits July 29, 2024 13:16

Merge branch 'main' into COMPASS-7724-3

bc2a141

merge main and rewrite store updates

0c28bc7

cleanup

d5094f5

gribnoysup reviewed Jul 30, 2024

View reviewed changes

paula-stacho added 3 commits July 30, 2024 13:12

fix

0449fc7

move telemetry events

99facac

move telemetry events

e30dc4d

gribnoysup approved these changes Jul 31, 2024

View reviewed changes

packages/compass-connections/src/stores/connections-store.spec.tsx Outdated Show resolved Hide resolved

packages/compass-connections/src/stores/connections-store.spec.tsx Outdated Show resolved Hide resolved

paula-stacho added 2 commits July 31, 2024 09:58

update tests

9ecdc79

cleanup

cc99ae0

paula-stacho merged commit ada8b30 into main Jul 31, 2024
28 checks passed

paula-stacho deleted the COMPASS-7724-3 branch July 31, 2024 11:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: new telemetry events for connections #6018

feat: new telemetry events for connections #6018

paula-stacho commented Jul 11, 2024 •

edited

Loading

paula-stacho Jul 11, 2024 •

edited

Loading

mcasimir commented Jul 12, 2024 •

edited

Loading

mcasimir commented Jul 12, 2024

paula-stacho commented Jul 12, 2024

paula-stacho commented Jul 12, 2024

mcasimir commented Jul 15, 2024

himanshusinghs left a comment •

edited

Loading

paula-stacho commented Jul 15, 2024

mcasimir Jul 15, 2024 •

edited

Loading

gribnoysup Jul 15, 2024

gribnoysup Jul 15, 2024 •

edited

Loading

mcasimir Jul 15, 2024

mcasimir Jul 15, 2024

gribnoysup Jul 15, 2024

gribnoysup Jul 15, 2024

paula-stacho Jul 15, 2024

paula-stacho commented Jul 16, 2024

gribnoysup Jul 30, 2024

gribnoysup Jul 30, 2024

paula-stacho Jul 30, 2024

gribnoysup Jul 30, 2024

paula-stacho Jul 31, 2024 •

edited

Loading

gribnoysup left a comment

		@@ -360,7 +366,11 @@ export function useConnections({
		Pick<ConnectionInfo, 'id'>

feat: new telemetry events for connections #6018

feat: new telemetry events for connections #6018

Conversation

paula-stacho commented Jul 11, 2024 • edited Loading

Description

Existing events

New events

Checklist

Motivation and Context

Open Questions

Dependents

Types of changes

paula-stacho Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

mcasimir commented Jul 12, 2024 • edited Loading

mcasimir commented Jul 12, 2024

paula-stacho commented Jul 12, 2024

paula-stacho commented Jul 12, 2024

mcasimir commented Jul 15, 2024

himanshusinghs left a comment • edited Loading

Choose a reason for hiding this comment

paula-stacho commented Jul 15, 2024

mcasimir Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gribnoysup Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paula-stacho commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paula-stacho Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

gribnoysup left a comment

Choose a reason for hiding this comment

paula-stacho commented Jul 11, 2024 •

edited

Loading

paula-stacho Jul 11, 2024 •

edited

Loading

mcasimir commented Jul 12, 2024 •

edited

Loading

himanshusinghs left a comment •

edited

Loading

mcasimir Jul 15, 2024 •

edited

Loading

gribnoysup Jul 15, 2024 •

edited

Loading

paula-stacho Jul 31, 2024 •

edited

Loading