open-telemetry / opentelemetry-collector

OpenTelemetry Collector
https://opentelemetry.io
Apache License 2.0
4.34k stars 1.44k forks source link

Collector Shutdown should block until Run cleans up #4947

Open cpheps opened 2 years ago

cpheps commented 2 years ago

Is your feature request related to a problem? Please describe. Discussion from 4878 led to a decision to design Shutdown to follow language patterns for how it and Run should operate. Following http.Server as an example the following behaviors should be met:

  1. Shutdown be safe to be called at any point. Even if there is no action to take
  2. If Shutdown is called before Run it will exit cleanly and the next call to Run will exit due to the previous Shutdown
  3. If Shutdown is called while Run is active it should block until Run has confirmed shutdown.

Point 3 isn't currently implemented but desired behavior.

Describe the solution you'd like Shutdown will block until Run has confirmed cleanup. Whatever Shutdown checks for should not stop it from cleanly exiting if Run hasn't been called yet or after Run was called.

Describe alternatives you've considered Alternative is don't block Shutdown and just exit like it does now after closing the channel.

Additional context Action item from #4878

If we want to go this route I think there needs to be some discussion on what Shutdown should block on. It seems like it should be when the state is Closing but that won't allow Shutdown to cleanly exit if called before or after Run.

dmitryax commented 2 years ago

@djaglowski said it can be upstreamed

djaglowski commented 2 years ago

Following up on this - I misunderstood the context of this issue, #4946, and 4939, and mistakenly believed they may have been resolved as part of #4878.

deepto98 commented 2 years ago

Can I work on this?

dmitryax commented 2 years ago

@deepto98 sure. Assigned to you. Thanks

Chinwendu20 commented 1 year ago

Hi @deepto98, are you still working on this? I would like to take this up

Chinwendu20 commented 1 year ago

Hi @cpheps I guess 1 and 2 has been addressed by other the other issues

michalpristas commented 11 months ago

going to take a look at this