-
Notifications
You must be signed in to change notification settings - Fork 889
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @spikecurtis and the rest of your teammates on |
3ac9064
to
204c103
Compare
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 use clock
?
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 a const
.
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.
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
to
9d29b4a
Compare
9d29b4a
to
b850ab4
Compare
Merge activity
|
Modifies the agent to use the v2 API to report its statistics, using the
statsReporter
subcomponent.