- Notifications
You must be signed in to change notification settings - Fork928
feat: switch agent to use v2 API for sending logs#12068
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
Conversation
spikecurtis commentedFeb 8, 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 |
4a32bba to14d0583Compare1c29364 to6f02c02Compare14d0583 to2014110Compare6f02c02 to7d7df61Compare2014110 to446cb12Compare7d7df61 to3106dacCompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
5f74643 tof25da2cCompare3106dac to72a8e58Compare40bd260 to08764ffCompare72a8e58 to67539d6Compare08764ff to4ce9f1dCompare67539d6 tod92399dCompare4ce9f1d to7ecf10eCompared92399d to6b20b22Compare7ecf10e to2f05031Compare6b20b22 to249076eCompare2f05031 to5cf1bb4Compare249076e to103c6e3Compare5cf1bb4 tof9ca6b2Compare103c6e3 to8e4eab1Comparef9ca6b2 tod9b360cCompare8e4eab1 to425a947Compare| // shutdown scripts. | ||
| connMan.start("send logs", gracefulShutdownBehaviorRemain, | ||
| func(ctx context.Context, conn drpc.Conn) error { | ||
| err := a.logSender.SendLoop(ctx, proto.NewDRPCAgentClient(conn)) |
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 it good practice to use multiple clients over the same conn, or should we define the client in the parent scope?
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 client contains no state other than the conn itself. It just maps RPC methods toInvoke orNewStream calls on the conn.
So, I think it's totally reasonable to make a new one per routine. That allows me to keep theconnMan generic with adrpc.Conn. There are actually 2 different proto APIs (tailnet and agent) with their own client types.
| @@ -2062,6 +2062,80 @@ func TestAgent_DebugServer(t *testing.T) { | |||
| }) | |||
| } | |||
| funcTestAgent_ScriptLogging(t*testing.T) { | |||
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.
❤️
| RunOnStop:true, | ||
| Script:`#!/bin/sh | ||
| i=0 | ||
| while [ $i -ne 3000 ] |
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 wonder if 3000 is flake-risky, considering we're using WaitMedium?
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.
Running on its own, the script part completes in ~100ms, including queueing all the logs, sending them, and asserting them in the test. If it flakes, something else is very wrong.
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.
Sure 👍🏻. I was mostly just worried about Windows, it can be unfathomably slow 😄
425a947 to943991fCompareMerge activity
|
Uh oh!
There was an error while loading.Please reload this page.
Changes the agent to use the new v2 API for sending logs, via the logSender component.
We keep the PatchLogs function around, but deprecate it so that we can test the v1 endpoint.