- Notifications
You must be signed in to change notification settings - Fork1k
feat: add statsReporter for reporting stats on agent v2 API#11920
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
This stack of pull requests is managed by Graphite.Learn more about stacking. Join@spikecurtis and the rest of your teammates on |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| req := testutil.RequireRecvCtx(ctx, t, fDest.reqs) | ||
| require.NotNil(t, req) | ||
| require.Nil(t, req.Stats) | ||
| interval := time.Second * 34 |
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.
Is there a special meaning to this magic value?
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 just wanted something that's not likely to be some internal constant and proves that we actually use the interval in the response.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
11f3d11 to8c43c94CompareThere 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.
Had one more question but looks good, thanks for added commentary 👍🏻
| return xerrors.Errorf("initial update: %w", err) | ||
| } | ||
| s.lastInterval = resp.ReportInterval.AsDuration() | ||
| s.source.SetConnStatsCallback(s.lastInterval, maxConns, s.callback) |
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.
Would it make sense to also unset the callback on exit? Mostly a safety precaution in casecallback ever has code that may block without a consumer.
Uh oh!
There was an error while loading.Please reload this page.
Adds a new statsReporter subcomponent of the agent, which in a later PR will be used to report stats over the v2 API.
Refactors the logic a bit so that we can handle starting and stopping stats reporting if the agent API connection drops and reconnects.