Add logging of user tier in the telemetry #1132

pkukielka · 2024-03-19T16:29:23Z

Fixes:
#550
sourcegraph/cody#2467

Changes

Added CodyAccount::getActiveAccountTier method with result caching.
Added tier logging in GraphQlLogger
Refactored account change notification logic to be concentrated in one place (CodyAccount::setActiveAccount)
Added periodic and on focus change querying of the account state (see CodyAuthenticationManager::init)

Test plan

Manual verification if proper entries are in the GraphQL

mkondratek

The issue: sourcegraph/cody#2467

comments, comments everywhere 🗯️

pkukielka · 2024-03-21T15:28:13Z

src/main/java/com/sourcegraph/cody/MyAccountTabPanel.kt

    val tier =
-        if (getIsCurrentUserPro == null) CodyBundle.getString("my-account-tab.loading-label")
-        else if (getIsCurrentUserPro) CodyBundle.getString("my-account-tab.cody-pro-label")
+        if (isPro == null) CodyBundle.getString("my-account-tab.loading-label")


mkondratek · 2024-03-19T21:33:13Z

src/main/kotlin/com/sourcegraph/cody/config/CodyAccount.kt

+      if (isDotcomAccount()) isProUser(project).thenApply { it.not() }
+      else CompletableFuture.completedFuture(false)
+
+  fun isProUser(project: Project): CompletableFuture<Boolean> {


maybe deriveIsCurrentUserPro? to be more precise and reflect the fact that it's not an instant quick "get" but a call to agent

oh, I just realised - we are caching the pro state now... 🤔

When a user upgrades to pro they are informed that they need to restart the IDE to see the changes. What when a user downgrades to free during the usage?

What when a user downgrades to free during the usage?\

Good question :)
#550
I will address that and some other improvements in a follow-up PR.

mkondratek · 2024-03-19T21:50:55Z

src/main/java/com/sourcegraph/cody/MyAccountTabPanel.kt

        else CodyBundle.getString("my-account-tab.cody-free-label")
    row { label("<html>Current tier: <b>$tier</b><html/>") }
    row {
-      if (getIsCurrentUserPro != null && !getIsCurrentUserPro) {
+      if (isPro != null && !isPro) {


nit: isn't that simply isPro == false?

mkondratek · 2024-03-19T21:51:00Z

src/main/java/com/sourcegraph/cody/MyAccountTabPanel.kt

        val upgradeButton =
            button("Upgrade") { BrowserUtil.browse(ConfigUtil.DOTCOM_URL + "cody/subscription") }
        upgradeButton.component.putClientProperty(DarculaButtonUI.DEFAULT_STYLE_KEY, true)
      }
      button("Check Usage") { BrowserUtil.browse(ConfigUtil.DOTCOM_URL + "cody/manage") }
    }
-    if (getIsCurrentUserPro != null && !getIsCurrentUserPro) {
+    if (isPro != null && !isPro) {


nit: isn't that simply isPro == false?

mkondratek · 2024-03-19T21:55:24Z

src/main/java/com/sourcegraph/cody/MyAccountTabUtils.kt

-}
-
-@RequiresBackgroundThread
-private fun ensureUserIdMatchInAgent(jetbrainsUserId: String, server: CodyAgentServer) {


so this is redundant? nice 🚀

tested - indeed it seems to work

mkondratek · 2024-03-19T22:06:07Z

src/main/kotlin/com/sourcegraph/cody/config/CodyAccount.kt

-    return AccountType.ENTERPRISE
+  @Volatile private var isProUser: Boolean? = null
+
+  fun getAccountType(project: Project): String {


I don't like that change. The difference b/w enterprise and free/pro is different than the difference b/w free and pro. We can call the first category "account type" and the second a "tier". And my feeling is that we are adjusting the model to the telemetry requirements (which feel bad).

If we really need to consider enterprise/free/pro as a same category maybe let's introduce an enum for them. Is AccountType still needed or should we deprecate it?

Also, if we do the agent call here the method name should be altered to reflect the hidden process - "derive" instead of "get"?

I can move this code the the logger as it's not user anywhere else so far.

mkondratek · 2024-03-19T22:18:28Z

src/main/kotlin/com/sourcegraph/cody/chat/ui/LlmDropdown.kt

-        !didSendFirstMessage && !(activeAccountType.isEnterpriseAccount() || model.size <= 1)
+    val isEnterpriseAccount =
+        CodyAuthenticationManager.instance.getActiveAccount(project)?.isEnterpriseAccount() ?: false
+    isEnabled = !didSendFirstMessage && !isEnterpriseAccount && model.size > 1


hah - it looks much better now 😅

mkondratek · 2024-03-19T22:20:13Z

src/main/java/com/sourcegraph/telemetry/GraphQlLogger.java

-      var event = createEvent(ConfigUtil.getServerPath(project), "CodyInstalled", new JsonObject());
+      var event =
+          createEvent(
+              project, ConfigUtil.getServerPath(project), "CodyInstalled", new JsonObject());


btw, can we have an enum class for the event name? 🙏 it would be easier to navigate b/w then and see which are already in the code in a one place

mkondratek · 2024-03-19T22:21:59Z

src/main/java/com/sourcegraph/telemetry/GraphQlLogger.java

@@ -65,9 +75,15 @@ private static Event createEvent(

  @NotNull
  private static JsonObject addGlobalEventParameters(


ah, now I see why we need to cache the tier... 🕵️

mkondratek · 2024-03-19T22:56:58Z

src/main/java/com/sourcegraph/telemetry/GraphQlLogger.java

I just noted new additional entries in the logs:

2024-03-19 23:48:44,484 [ 20502] WARN - #c.s.c.a.CodyAgentClient - Cody by Sourcegraph: █ GraphQLTelemetryExporter: telemetry: failed to evaluate server version: Error: accessing Sourcegraph GraphQL API: Error: HTTP status code 401: Private mode requires authentication. (https://sg02.sourcegraphcloud.com/.api/graphql?SiteProductVersion) 2024-03-19 23:48:44,867 [ 20885] WARN - #c.s.c.a.CodyAgentClient - Cody by Sourcegraph: █ GraphQLTelemetryExporter: Error exporting telemetry events: Error: accessing Sourcegraph GraphQL API: Error: HTTP status code 401: Private mode requires authentication.

Steps:

runIde

switch accounts from the settings

I tested it against v5.4.5 - no GraphQLTelemetryExporter entries there in the same scenario

I think it's a false alarm - sorry for confusion.

The issue (#454 (comment)) seems to be there already. The two mentioned lines of logs seem to appear when switching to sg02 instance account (in particular).

mkondratek · 2024-03-26T08:36:20Z

src/main/kotlin/com/sourcegraph/cody/config/CodyAuthenticationManager.kt

-@Service
-class CodyAuthenticationManager internal constructor() {
+@Service(Service.Level.PROJECT)
+class CodyAuthenticationManager(val project: Project) : Disposable {


good point about revamping it into a project-level service 👍

mkondratek · 2024-03-26T08:37:28Z

src/main/kotlin/com/sourcegraph/cody/config/notification/AccountSettingChangeContext.kt

-    val accessTokenChanged: Boolean = false
-)
+    val accessTokenChanged: Boolean = false,
+    val accountTierChanged: Boolean = false


but it's unused 🤔

It was added mostly for clarity so e.g. in debugger when you will notice AccountSettingChangeContext it will be clear why it was emitted. With just two other properties set to false it could look like a bug.

mkondratek · 2024-03-26T08:40:41Z

src/main/kotlin/com/sourcegraph/cody/chat/ui/LlmDropdown.kt

 class LlmDropdown(
    private val project: Project,
    private val onSetSelectedItem: (ChatModelsResponse.ChatModelProvider) -> Unit,
    private val chatModelProviderFromState: ChatModelsResponse.ChatModelProvider?,
 ) : ComboBox<ChatModelsResponse.ChatModelProvider>(MutableCollectionComboBoxModel()) {

  private var didSendFirstMessage: Boolean = false
+  var isCurrentUserFree = true


iirc that is the place you mentioned - the scenario is that a user has an open chat with no message in and changes their account tier - that change will not be reflected in the dropdown. is that right?

can you please put a comment on this field? this place looks a bit extraordinary compared to the rest of the codebase after your changes

I think we should just fix it, I kind of forgot about it afterwards.

mkondratek · 2024-03-26T08:48:26Z

src/main/kotlin/com/sourcegraph/cody/auth/ui/AccountsPanelFactoryExtensions.kt

-    val activeAccount = accountsModel.activeAccount
-    activeAccountHolder.account = activeAccount


iiuc this logic (setting the active account in the holder) is executed anyway by CodyAuthenticationManager.getInstance(project).setActiveAccount(activeAccount)

mkondratek · 2024-03-26T11:33:07Z

src/main/kotlin/com/sourcegraph/cody/config/notification/AccountSettingChangeListener.kt

-                agent.server.configurationDidChange(ConfigUtil.getAgentConfiguration(project))
-              }
-              CodyAgentService.getInstance(project).restartAgent(project)
+            if (ConfigUtil.isCodyEnabled() && context.accountChanged()) {
              CodyToolWindowContent.executeOnInstanceIfNotDisposed(project) {
                removeAllChatSessions()


hmm, if I change the token it makes accountChanged == true but that is not necessarily mean the account changed - I mean - I can change my own token to another token of the same account

Maybe we should adjust the name. accountConfigurationChanged?

…dy model

mkondratek requested changes Mar 19, 2024

View reviewed changes

mkondratek reviewed Mar 19, 2024

View reviewed changes

mkondratek approved these changes Mar 21, 2024

View reviewed changes

pkukielka force-pushed the pkukielka/add-user-tier-logging-telemetry branch 2 times, most recently from ea6d66a to 0fe55e6 Compare March 25, 2024 14:06

mkondratek reviewed Mar 26, 2024

View reviewed changes

pkukielka added 4 commits March 27, 2024 15:46

Refactor account settings for free/pro users

24f8236

Add logging user tier to telemetry

1a2193f

Fixes for review

c1561ed

Improve account management model

45f6e84

pkukielka force-pushed the pkukielka/add-user-tier-logging-telemetry branch from 27ded74 to 9602860 Compare March 27, 2024 16:31

Change tier logging from string to number representation to follow Co…

5a01768

…dy model

pkukielka force-pushed the pkukielka/add-user-tier-logging-telemetry branch from 9602860 to 5a01768 Compare March 27, 2024 16:50

pkukielka merged commit 57070c5 into main Mar 27, 2024
5 checks passed

pkukielka deleted the pkukielka/add-user-tier-logging-telemetry branch March 27, 2024 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logging of user tier in the telemetry #1132

Add logging of user tier in the telemetry #1132

pkukielka commented Mar 19, 2024 •

edited

Loading

mkondratek left a comment

This comment was marked as resolved.

pkukielka Mar 21, 2024

mkondratek Mar 19, 2024

mkondratek Mar 19, 2024

pkukielka Mar 20, 2024 •

edited

Loading

mkondratek Mar 19, 2024

pkukielka Mar 21, 2024

mkondratek Mar 19, 2024

pkukielka Mar 26, 2024

mkondratek Mar 19, 2024

mkondratek Mar 19, 2024

mkondratek Mar 19, 2024

pkukielka Mar 20, 2024

mkondratek Mar 19, 2024

mkondratek Mar 19, 2024

mkondratek Mar 19, 2024

mkondratek Mar 19, 2024

mkondratek Mar 19, 2024

mkondratek Mar 26, 2024

mkondratek Mar 26, 2024

mkondratek Mar 26, 2024

pkukielka Mar 27, 2024

mkondratek Mar 26, 2024

pkukielka Mar 26, 2024

mkondratek Mar 26, 2024

pkukielka Mar 26, 2024

mkondratek Mar 26, 2024

pkukielka Mar 26, 2024

		@@ -65,9 +75,15 @@ private static Event createEvent(

		@NotNull
		private static JsonObject addGlobalEventParameters(

		val activeAccount = accountsModel.activeAccount
		activeAccountHolder.account = activeAccount

Add logging of user tier in the telemetry #1132

Add logging of user tier in the telemetry #1132

Conversation

pkukielka commented Mar 19, 2024 • edited Loading

Changes

Test plan

mkondratek left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkukielka Mar 20, 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

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

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

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

pkukielka commented Mar 19, 2024 •

edited

Loading

pkukielka Mar 20, 2024 •

edited

Loading