- Notifications
You must be signed in to change notification settings - Fork1k
chore: zero out session stats from agent with experiment enabled#13579
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| // if the experiment is enabled we zero out certain session stats | ||
| // as we migrate to the client reporting these stats instead. |
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.
Can this get implemented serverside instead? It seems like a lot of changes in the agent for something that could be zeroed out in the stats API on the server.
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.
+1, this would keep the change in 'one place' and remove the need for plumbing this through AgentAPI
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.
Not really and here's why - If customers update coder but do not rebuild the workspace, we will have an old agent client and old cli version in the workspace. If the experiment is enabled and we zero out the value server side, we will lose data for that workspace because the old cli will not be reporting the new data via the usage endpoint. So the graphs will go to zero and slowly come back up over time as each workspace is restarted.
So we need the endpoint to still save this data for old workspaces, but also allow new workspaces to report it in the new way. Or we accept downtime on the stats on upgrade until all workspaces are updated. Thoughts?
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.
If the experiment is enabled and we zero out the value server side, we will lose data for that workspace because the old cli will not be reporting the new data via the usage endpoint.
Won't the same thing happen even if the feature was GA? Old agent/CLI versions need to be supported on new servers for multiple months.
This will also affect stats collection for older CLIs being used outside of workspaces, which arguably is where more people use the CLI anyways. I don't think anyone is using the CLI in a workspace to connect to a workspace.
So we need the endpoint to still save this data for old workspaces, but also allow new workspaces to report it in the new way. Or we accept downtime on the stats on upgrade until all workspaces are updated.
I think either way there will be a "downtime" of stats with or without the agent changes when an older CLI is being used on the local machine.
I think this PR should change into just the CLI stats upload portion and have both sides report stats for now. Then in a few months you can remove stats reporting from the API by deprecating the field and ignoring it serverside.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| // if the experiment is enabled we zero out certain session stats | ||
| // as we migrate to the client reporting these stats instead. |
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.
+1, this would keep the change in 'one place' and remove the need for plumbing this through AgentAPI
f0ssel commentedJun 22, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Closing due to release rollout issues in favor of#13635 |
Uh oh!
There was an error while loading.Please reload this page.
This moves the responsibility for reporting ssh session stats from the agent to the cli ssh command.
agentapi- Adds newGetExperimentsendpoint so agents can query active experiments. AgentAPI Version bumped to2.2.agent- Now queries experiments before starting stats report loop. Ifworkspace_usageexperiment is enabled we will zero out SSH session stats before sending it to coderd.cli- Connecting to a new SSH session will now regularly push to workspace usage endpoint, which will write ssh session stats if the experiment is enabled.