- Notifications
You must be signed in to change notification settings - Fork928
feat: change agent to use v2 API for reporting stats#12024
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
spikecurtis commentedFeb 6, 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.
This stack of pull requests is managed by Graphite.Learn more about stacking. Join@spikecurtis and the rest of your teammates on |
3ac9064 to204c103Compare| if req.Stats != nil { | ||
| f.statsCh <- req.Stats | ||
| } | ||
| return &agentproto.UpdateStatsResponse{ReportInterval: durationpb.New(statsInterval)}, nil |
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.
Does it mean that tests are time dependent on realstatsInterval? If so, it is possible to refactor them to useclock?
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.
yes, they are, and yes, it's possible. But, I think that's outside the scope of this PR. I've just copied the hardcoded value from before into aconst.
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.
Oh, I assumed that it would be a low-hanging fruit to refactor, but I'm good with leaving for further.
| ok,s.ConnectionCount,s.RxBytes,s.TxBytes,s.SessionCountVSCode,s.ConnectionMedianLatencyMS) | ||
| ok,s.ConnectionCount,s.RxBytes,s.TxBytes,s.SessionCountVscode,s.ConnectionMedianLatencyMs) |
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.
VSCode vs Vscode
PTY vs Pty
are these renamed on purpose?
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.
The SDK type fields were written by humans, the proto type fields are machine translated from lowercase_with_underscores so it doesn't get capitalization correct in all cases. Unfortunate from a style perspective, but I haven't chased down how to fix it yet.
Uh oh!
There was an error while loading.Please reload this page.
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
204c103 to9d29b4aCompare9d29b4a tob850ab4CompareMerge activity
|
Uh oh!
There was an error while loading.Please reload this page.
Modifies the agent to use the v2 API to report its statistics, using the
statsReportersubcomponent.