feat: agent shutdown #3022
Merged
feat: agent shutdown #3022
Conversation
This file contains 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
mathnogueira
commented
Aug 2, 2023
Comment on lines
29
to
32
| go func() { | ||
| <-c.done | ||
| cancelCtx() | ||
| }() |
There was a problem hiding this comment.
Initially, I was going to use the same goroutine for that, however, stream.RecvMsg(&resp) is a blocking operation. Meaning that if we got a shutdown command before receiving a polling request, it would just freeze because the server would probably remove this agent from the agent poll.
By using another goroutine, this doesn't happen. We can cancel the context as soon as we receive the shutdown message.
There was a problem hiding this comment.
Does it make sense to add a comment here too?
There was a problem hiding this comment.
managed to solve this issue in a better place, so no explanation is needed.
mathnogueira
commented
Aug 2, 2023
| @@ -10,20 +10,27 @@ import ( | |||
| ) | |||
|
|
|||
| func (c *Client) startPollerListener() error { | |||
| ctx := context.Background() | |||
| ctx, cancelCtx := context.WithCancel(context.Background()) | |||
There was a problem hiding this comment.
I cannot defer cancelCtx here because this function ends while the goroutine keeps going on. If I defer the call, the goroutine will break.
There was a problem hiding this comment.
Can you add a comment explaining this situation on the code?
danielbdias
approved these changes
Aug 2, 2023
| @@ -10,20 +10,27 @@ import ( | |||
| ) | |||
|
|
|||
| func (c *Client) startPollerListener() error { | |||
| ctx := context.Background() | |||
| ctx, cancelCtx := context.WithCancel(context.Background()) | |||
There was a problem hiding this comment.
Can you add a comment explaining this situation on the code?
Comment on lines
29
to
32
| go func() { | ||
| <-c.done | ||
| cancelCtx() | ||
| }() |
There was a problem hiding this comment.
Does it make sense to add a comment here too?
Comment on lines
+39
to
+40
| // TODO: get context from request | ||
| c.shutdownListener(context.Background(), &resp) |
There was a problem hiding this comment.
Do you plan to do that in another PR?
There was a problem hiding this comment.
yes, several places require this. I want to have a PR to instrument the agent
schoren
pushed a commit
that referenced
this pull request
Aug 9, 2023
* shutdown agent when receive shutdown request * remove unused function * fix brittle test * centrilize stop function
schoren
pushed a commit
that referenced
this pull request
Aug 11, 2023
* shutdown agent when receive shutdown request * remove unused function * fix brittle test * centrilize stop function
mathnogueira
added a commit
that referenced
this pull request
Aug 18, 2023
* shutdown agent when receive shutdown request * remove unused function * fix brittle test * centrilize stop function
mathnogueira
added a commit
that referenced
this pull request
Aug 31, 2023
* feat: add structure for agent (#2964) * feat: add structure for agent * add short and long description for agent command * fix build * add go.work * feat: create client for receiving trigger requests from the server (#2970) * wip: connect to grpc server * feat: add connection workflow * add protobuf for trigger request * add flow to receive trigger requests * add comments and rename files * cleanup makefile * configure CI to run agent tests * fix makefile * add flow for sending trigger result back to the server * feat: agent client polling methods (#2972) * add flow for listening for polling requests * feat: send spans to server * feat: send trigger requests from agent (#2975) * feat: add empty trigger worker * move executor/trigger package to agent and adapt it * adapt test * implement trigger * test trigger flow * send trigger response to server * add TODO * feat: agent polling (#2979) * feat: implement trace polling on agent * initialize polling worker on agent start * pass worker functions directly to client hooks * feat: agent internal collector (#2997) * feat: add otlp and http servers with no operation yet * feat: implement a very basic collector in the agent * remove tracer from otlp_server structure * Update agent/collector/http_server.go Co-authored-by: Daniel Baptista Dias <danielbdias@users.noreply.github.com> * Update agent/collector/http_server.go Co-authored-by: Daniel Baptista Dias <danielbdias@users.noreply.github.com> * reuse otlp servers * apply daniel's comments to the otlp server code * remove dependency on ioutil from http otlp server * delete http server from agent collector * make collector ingester use otlp ingester interface --------- Co-authored-by: Daniel Baptista Dias <danielbdias@users.noreply.github.com> * feat: agent shutdown (#3022) * shutdown agent when receive shutdown request * remove unused function * fix brittle test * centrilize stop function * update mod * fix merge errors; * fix merge errors; * more fixes * feat: add agent token to proto and client (#3068) feat: add agent identification to agent client proto * add token to shutdown listener method (#3071) * fix(agent): rename traceid response property in proto (#3076) fix(agent): rename traceid response property to traceid * update go.work.sum * feat(agent): add kafka agent proto (#3080) add kafka response * fix module name * feat(agent): inject env variables in agent configuration (#3097) * feat(agent): inject env variables in agent configuration * add tests to cover all config values are configurable via env vars * add log at startup and set hostname as default agent name * remove dev mode config * fix config defaults test * feat: make agent wait until it's disconnected to exit (#3109) feat(agent): make agent wait until connection end This prevents the agent from exiting right after connecting to the server --------- Co-authored-by: Daniel Baptista Dias <danielbdias@users.noreply.github.com> Co-authored-by: Sebastian Choren <sebastian.choren@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR allows the server to send a shutdown signal to the agent and make it shutdown remotely.
Checklist