feat: switch agent to use v2 API for sending logs #12068
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Feb 8, 2024
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @spikecurtis and the rest of your teammates on |
This was referenced Feb 9, 2024
4a32bba to
14d0583
Compare
1c29364 to
6f02c02
Compare
14d0583 to
2014110
Compare
6f02c02 to
7d7df61
Compare
2014110 to
446cb12
Compare
7d7df61 to
3106dac
Compare
Base automatically changed from
spike/10534-log-queue-limit
to
spike/10534-log-sender
February 9, 2024 12:57
mafredri
reviewed
Feb 12, 2024
5f74643 to
f25da2c
Compare
This was referenced Feb 15, 2024
3106dac to
72a8e58
Compare
40bd260 to
08764ff
Compare
72a8e58 to
67539d6
Compare
08764ff to
4ce9f1d
Compare
67539d6 to
d92399d
Compare
4ce9f1d to
7ecf10e
Compare
d92399d to
6b20b22
Compare
7ecf10e to
2f05031
Compare
6b20b22 to
249076e
Compare
2f05031 to
5cf1bb4
Compare
249076e to
103c6e3
Compare
5cf1bb4 to
f9ca6b2
Compare
103c6e3 to
8e4eab1
Compare
f9ca6b2 to
d9b360c
Compare
8e4eab1 to
425a947
Compare
mafredri
approved these changes
Feb 22, 2024
| // 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.
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.
The client contains no state other than the conn itself. It just maps RPC methods to Invoke or NewStream calls on the conn.
So, I think it's totally reasonable to make a new one per routine. That allows me to keep the connMan generic with a drpc.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) { | |||
| }) | |||
| } | |||
|
|
|||
| func TestAgent_ScriptLogging(t *testing.T) { | |||
| RunOnStop: true, | ||
| Script: `#!/bin/sh | ||
| i=0 | ||
| while [ $i -ne 3000 ] |
There was a problem hiding this comment.
I wonder if 3000 is flake-risky, considering we're using WaitMedium?
There was a problem hiding this comment.
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.
Sure 👍🏻. I was mostly just worried about Windows, it can be unfathomably slow 😄
425a947 to
943991f
Compare
Merge activity
|
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.