-
Notifications
You must be signed in to change notification settings - Fork 881
chore: switch ssh session stats based on experiment #13637
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
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.
LGTM 👍
@@ -1076,5 +1081,5 @@ func getUsageAppName(usageApp string) codersdk.UsageAppName { | |||
return codersdk.UsageAppName(usageApp) | |||
} | |||
|
|||
return "" | |||
return codersdk.UsageAppNameSSH |
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.
Letting the zero value mean 'disabled' feels more idiomatic to me, but 'disabled' is way more explicit and better from a user perspective.
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 considered having the default value on the flag be ssh
and disable reporting if it's manually set to empty string but I wasn't sure if that was explicit enough behavior. I'm thinking in the extensions where we do CODER_SSH_USAGE_APP_NAME="" coder ssh ...
I'm worried there may be cases where the empty string get's passed to the CLI as "unset" instead.
This changes
coderd
to only allow a single endpoint to write ssh session data to the DB at a time. Both the CLI and the agent will always report stats, but the agent endpoint will silently drop session stats while the experimentworkspace-usage
is enabled. This prevents us from double-counting stats during the migration period.We will need to give end users sufficient time to update their CLI and extensions before moving this behavior to GA otherwise outdated clients will have their data dropped until they upgrade.