- Notifications
You must be signed in to change notification settings - Fork1k
chore: add DRPC server implementation for network telemetry#13675
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
620c148 tofb9d5aaCompareethanndickson commentedJun 27, 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@deansheather and the rest of your teammates on |
Graphite messed up all the commits in my branch and now they're co-authored by Ethan... the actual commits were only written by me. When I do the merge I'll remove the co-authored by line. |
937bf83 to479df1fCompareUh 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.
| func (b*NetworkTelemetryBatcher)Close()error { | ||
| close(b.closed) | ||
| <-b.done | ||
| returnnil | ||
| } |
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.
This should probably accept a context so it can't block forever, or move the waiting to a separateWaitUntilEmpty() or similar
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'm not going to make it accept a context, but I've added a 5s timeout and an error.
| ifb.ticker!=nil { | ||
| b.ticker.Reset(b.frequency) | ||
| } | ||
| gob.batchFn(events) |
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.
This feels a bit racey. We should probably have a separate channel to signal to the other goroutine that it should tick.
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.
This isn't racey because we hold a lock. If a tick happens while we're handling messages, the pending events list will be emptied and the periodic batcher goroutine will do nothing because there are no events. If somehow we went above the batch size anyways and overflow, it'll just send two telemetry events in short succession which is acceptable. I've added a comment that the ticker reset is best effort
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
Uh oh!
There was an error while loading.Please reload this page.
tailnetproto.DRPCTailnetServerPostTelemetrymethodtelemetrypackage and converter functionsTODO: