Skip to content

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

Merged
merged 8 commits into from
Jun 25, 2024

Conversation

f0ssel
Copy link
Contributor

@f0ssel f0ssel commented Jun 22, 2024

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 experiment workspace-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.

@f0ssel f0ssel marked this pull request as ready for review June 22, 2024 16:53
@f0ssel f0ssel requested review from johnstcn and deansheather June 22, 2024 16:54
@f0ssel f0ssel requested a review from johnstcn June 24, 2024 16:24
Copy link
Member

@johnstcn johnstcn left a 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
Copy link
Member

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.

Copy link
Contributor Author

@f0ssel f0ssel Jun 24, 2024

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.

@f0ssel f0ssel merged commit fed668b into main Jun 25, 2024
30 checks passed
@f0ssel f0ssel deleted the f0ssel/ws-stats-experiment branch June 25, 2024 14:58
@github-actions github-actions bot locked and limited conversation to collaborators Jun 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants