[component] component.go - when can component.Shutdown return errors?

[atoulme]

When does it make sense for component.Shutdown to return errors?

We don't set any expectations as to when Shutdown may return errors or what to expect.

When Shutdown returns an error, we associate this to a permanent error on the component status instead of stopped status:

opentelemetry-collector/service/internal/graph/graph.go

Lines 435 to 442 in c5a2c78

	if compErr := comp.Shutdown(ctx); compErr != nil {
	errs = multierr.Append(errs, compErr)
	g.telemetry.Status.ReportStatus(
	instanceID,
	component.NewPermanentErrorEvent(compErr),
	)
	continue
	}

Is this status report useful? Could it be the implementer's responsibility to emit error events?

[mwear]

I made a suggestion that we remove the error return status from component.Start and use status reporting for errors. I'd propose that we do the same for component.Shutdown.

[TylerHelmuth]

I agree that an error returned from shutdown doesn't really help. It should ultimately just be logged somewhere and the collector should continue shutting down. It implies the component shutdown didn't happen correctly, but there isn't anything the collector can do about it at this point in the lifecycle.

[yurishkuro]

It is pretty common in stdlib and others (e.g. grpc) to return error from Close/Shutdown. The caller can log the error.

[TylerHelmuth]

Agreed, and that's what we do today. I was saying that it doesn't have to be the caller that logs the error, as long as something does it.

[atoulme]

Looking at the code, I see that errors returned during Shutdown eventually make their way all the way up to the cobra command.
It shows the following:

opentelemetry-collector/cmd/otelcorecol/main.go

Lines 53 to 55 in fb9d80d

	if err := cmd.Execute(); err != nil {
	log.Fatalf("collector server run finished with error: %v", err)
	}

Any error on shutdown will make the collector exit with an error code 1. Is that expected?

[TylerHelmuth]

Exiting with a non-zero code feels appropriate since it wasn't a clean shutdown.

[mwear]

I agree that the non-zero exit code makes sense, so I think we need to keep the error return value. My suggestion to remove the error return values from Start and Shutdown and rely on status reporting instead was an attempt to simplify error handling during those phases. However, based on this discussion and the one on #9324, it seems that maybe things are already as simple as they can be. Both the error return value and status reporting have their places.

[codeboten]

I agree that it makes sense to keep the returned error with Shutdown. Unless others feel strongly about this, I suggest we close this issue

[atoulme]

Agreed. Closing as complete.