TelemetrySettings.ReportComponentStatus(...) error: unsure how to handle returned error

[atoulme]

We are using new component status reporting capabilities that replace host.ReportFatalError.

The new method to report component status has a signature that returns an error:
TelemetrySettings.ReportComponentStatus(...) error

How should we handle the error being returned? It is ignored in most places right now.

[atoulme]

It appears an error can be returned if the status transition is invalid:

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

Lines 32 to 37 in 6114091

	return fmt.Errorf(
	"cannot transition from %s to %s: %w",
	m.current.Status(),
	ev.Status(),
	errInvalidStateTransition,
	)

[atoulme]

The transitions themselves are defined here:

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

Lines 50 to 82 in 6114091

	transitions: map[component.Status]map[component.Status]struct{}{
	component.StatusNone: {
	component.StatusStarting: {},
	},
	component.StatusStarting: {
	component.StatusOK: {},
	component.StatusRecoverableError: {},
	component.StatusPermanentError: {},
	component.StatusFatalError: {},
	component.StatusStopping: {},
	},
	component.StatusOK: {
	component.StatusRecoverableError: {},
	component.StatusPermanentError: {},
	component.StatusFatalError: {},
	component.StatusStopping: {},
	},
	component.StatusRecoverableError: {
	component.StatusOK: {},
	component.StatusPermanentError: {},
	component.StatusFatalError: {},
	component.StatusStopping: {},
	},
	component.StatusPermanentError: {},
	component.StatusFatalError: {},
	component.StatusStopping: {
	component.StatusRecoverableError: {},
	component.StatusPermanentError: {},
	component.StatusFatalError: {},
	component.StatusStopped: {},
	},
	component.StatusStopped: {},
	},

[mwear]

There are two situations that ReportComponentStatus will return an error. You've pointed out the first, an invalid state transition. The second, is reporting status before the service start (https://github.com/open-telemetry/opentelemetry-collector/blob/main/service/service.go#L151). The way the underlying finite state machine works, components do not need to worry about keeping track of their current status, or be concerned about invalid state transitions. At least, I have not found a case where this is needed. So, perhaps returning an error in this situation is not the right thing to do. If we wanted to return anything, it could a bool ok, or multiple return values of current status and ok. I think these would also be ignored in all, or most cases. If we wanted to remove the return value altogether, we could consider logging at debug level for these cases.

[atoulme]

Could you make it that an invalid transition is by itself a transition to an end state of "invalid transition"?

[mwear]

No, that would not be helpful. The finite state machine does mainly two things. It ensures that the component adheres to its lifecycle, and also when to send a new status event. New status events are only sent on state changes. For example, a component cannot transition from StatusOK to StatusOK. It's already OK, so we don't want a new status event, but it's fine for the component to report that it is OK anyways. If it had been in a RecoverableError state, it would have made a transition and emitted a new event.

Another example is the PermanentError state. It is a terminal state. Once a component is in this state, it cannot transition out of it. The purpose of this state to indicate that a person needs to intervene and fix something. Any attempts to transition out of this state will fail. So, it wouldn't be possible to set this back to a RecoverableError or StatusOK by accident.

[atoulme]

How about, instead of proceeding to the invalid transition, we make the component transition to the invalid state and back to the original state it was in? Do we record states in a way that can be audited?

[mwear]

There is really no reason to go to an invalid state and then back again. Preventing certain transitions is really a feature of the finite state machine. It spares the component from having to keep track of its current state and knowing anything about what transitions are valid. In practice, a mostly healthy component will transition between StatusOK and StatusRecoverableError. The component can report StatusOK when it's ok and StatusRecoverableError when it has encountered a recoverable error. It doesn't need to know its current state to report either of those statuses. If either of those did result in a state transition, you a new status event will be emitted, however, if they were duplicate reports of the current status, nothing has changed. It's not an error per se, but no transition resulted and no new events were emitted.

[mwear]

It might make sense to look at a PR that puts this to use. I have one that adds status reporting to the core exporters. It adds a WithStatusReporting option to exporter helper that adds basic status reporting to an exporter by wrapping the Consume* methods. See: https://github.com/open-telemetry/opentelemetry-collector/pull/8684/files#diff-f9f8fd062c94906498ca33e7d0b9d4788c57e583b9ec9c8a19f5e4e314bd51c7R16-R31

If consume succeeds it will report StatusOK. If consume fails it will report StatusRecoverableError. It does not keep track of the current state before reporting either of these. If the component is already OK, nothing happens. Likewise, if the component was already in a RecoverableError state, and a StatusRecoverableError was reported, nothing happens. However, if the component was OK and a StatusRecoverableError is reported it will transition to RecoverableError. If a subsequent call is OK, the state will transition to StatusOK, and the component will have recovered. StatusEvents are emitted only on state changes. The attempted invalid state transitions are expected, and not really a problem.

It goes further and adds some special cases for PermanentErrors to the otlp exporter. A permanent error is an error detected a runtime that will require human intervention to resolve. What constitutes a permanent error is likely to be controversial, and the choices in the PR are certainly up to debate. Let me at least walk through one scenario and how that plays out with default status reporting added by exporter helper.

Within the exporter itself these status codes will be treated as permanent errors and reporting a permanent error here will make subsequent status reporting (e.g. from the exporter helper) effectively a no-op, which is intentional: https://github.com/open-telemetry/opentelemetry-collector/pull/8684/files#diff-a13e181e395f2cb03704cecf9c08f081decf273f1042b7244ad732b01637ce24R173-R219

Let's consider http.StatusRequestEntityTooLarge, likely a controversial choice, but nevertheless, the idea is that the batch size is misconfigured for this exporter. This will be something that needs to be corrected with a config change (needing an intervention of some kind). During the course of the collector running, it could be that it has some successful requests and is initially reporting StatusOK. It could be that next, it encounters a timeout and transitions to the RecoverableError state. It could then have another successful request and transition back to StatusOK. When it eventually sends a request that is too large and receives a 413 it will transition to PermanentError. Once in this state, it cannot make any further transitions. It might be the case that subsequent requests succeed because they do not send an full sized batch, but the attempted reports of StatusOK effectively no-op (are invalid), which is what we want to happen. Because we detected a 413 at runtime, we know a config change is needed to fix this problem. It's not going to go away, even though some requests may succeed because they do not max out the batch size.

Let me know if this helps clear anything up, or if it just adds more confusion 😅.

[atoulme]

Yep got it. Trying to see how to go from there.

[atoulme]

Opening #9175 to see if there's an elegant way to report invalid transition errors as part of the reporter.