open-telemetry / opentelemetry-collector-contrib

Contrib repository for the OpenTelemetry Collector
https://opentelemetry.io
Apache License 2.0
3.08k stars 2.37k forks source link

healthcheckextension: Add an ability to delay collector shutdown #34454

Open yuri-rs opened 3 months ago

yuri-rs commented 3 months ago

Component(s)

extension/healthcheck, extension/healthcheckv2

Is your feature request related to a problem? Please describe.

We have a problem with 503 errors in the K8s environment for the otel endpoint. These errors occur during the distribution/change of the collector configuration. We have a process to restart the collector inside the pod with the new configuration. During the configuration rollout, the collector shuts down too quickly, so K8s doesn't update the pod's healthy state and continues sending requests to this pod, leading to 503 errors.

Describe the solution you'd like

I'd like to have a way to delay the collector/endpoint shutdown after the HC state changes to NotReady. This way, pod readiness would catch the NotReady status and move the traffic to the other pods before the actual collector shutdown.

It could look like: https://github.com/yuri-rs/opentelemetry-collector-contrib/commit/55e0c5ab5a5228e308c108d0f32717a453059523

Describe alternatives you've considered

An alternative could be a delay in the collector service itself, like (added sleep line):

// Shutdown the service. Shutdown will do the following steps in order:
// 1. Notify extensions that the pipeline is shutting down.
// 2. Shutdown all pipelines.
// 3. Shutdown all extensions.
// 4. Shutdown telemetry.
func (srv *Service) Shutdown(ctx context.Context) error {
    // Accumulate errors and proceed with shutting down remaining components.
    var errs error

    // Begin shutdown sequence.
    srv.telemetrySettings.Logger.Info("Starting shutdown...")

    if err := srv.host.serviceExtensions.NotifyPipelineNotReady(); err != nil {
        errs = multierr.Append(errs, fmt.Errorf("failed to notify that pipeline is not ready: %w", err))
    }

    time.Sleep(time.Second)

    if err := srv.host.pipelines.ShutdownAll(ctx, srv.reporter); err != nil {
        errs = multierr.Append(errs, fmt.Errorf("failed to shutdown pipelines: %w", err))
    }

However, doing it inside the healthcheck extension seems better to me.

Additional context

What do you think about this feature? Would you accept a PR with this functionality?

github-actions[bot] commented 3 months ago

Pinging code owners:

jpkrohling commented 3 months ago

The current health check extension will give way to the health check v2 extension, which should be ready soon. @mwear, do you remember if this use-case is part of the scope for v2 already?

github-actions[bot] commented 1 month ago

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

atoulme commented 4 weeks ago

I would recommend you write an extension that waits a number of seconds on shutdown. That's an easy thing to maintain and will do better for this use case, no need to tack this on the health check extension imo.