Start order of extensions is not guaranteed?

[yurishkuro]

Describe the bug
Maybe I am misreading the situation, but this code iterates over map when starting extensions, so it cannot guarantee in which order they are started:

opentelemetry-collector/service/extensions/extensions.go

Line 34 in e97ceca

for extID, ext := range bes.extMap {

Even though the documentation implies that the order is define by the declaration order:

opentelemetry-collector/docs/service-extensions.md

Lines 63 to 66 in e97ceca

	service:
	# extensions lists the extensions added to the service. They are started
	# in the order presented below and stopped in the reverse order.
	extensions: [health_check, pprof, zpages]

I observed some spontaneous failures in my experiments with JaegerV2 which declares extensions with an order expectation: extensions: [jaeger_storage, jaeger_query] but the logs show differently:

2023-10-24T01:17:01.143Z	info	extensions/extensions.go:33	Starting extensions...
2023-10-24T01:17:01.143Z	info	extensions/extensions.go:36	Extension is starting...	{"kind": "extension", "name": "jaeger_query"}

Steps to reproduce
It's random behavior, hard to reproduce reliably.

What did you expect to see?
Extensions should be starting in the order defined in the config.

What did you see instead?
Seeing different order.

What version did you use?
Version: v0.87.0

What config did you use?
https://github.com/jaegertracing/jaeger/blob/28520b31471b36d9e949de4ffaa016c2640a3be7/cmd/jaeger-v2/internal/all-in-one.yaml#L1

Environment
OS: MacOS, Linux
Compiler(if manually compiled): go 1.20.x

[atoulme]

I'd be happy to work on this one.

[dmitryax]

I'm not sure this is a bug. I think this was implemented intentionally. All extensions that we have in core and contrib do not depend on each other. This assumption is used in some discussions around config grouping/merging, e.g.: open-telemetry/opentelemetry-collector-contrib#18509 and #8394.

I think we need a strong argument to introduce this behavior.

@yurishkuro do you have an extension that depends on another extension? Can you please let us know why it's needed in your use case?

[yurishkuro]

The use case is explained here: https://docs.google.com/document/d/1s4_6VgAS7qAVp6iEm5KYvpiGw3h2Ja5T5HpKo29iv00/view#heading=h.1ded8prqa1xm

flowchart LR
    E[Exporter] -->|Get storage| S(StorageExtension)
    Q[Jaeger-Query extension] -->|Get storage| S(StorageExtension)

Loading

I believe this may also be required by the Encoding extensions (#6272), which is a similar use case.

The alternative to declaration order would be allowing to express dependencies explicitly and then building a dependency graph to control the initialization order.

[yurishkuro]

@djaglowski You mentioned service graph in open-telemetry/opentelemetry-collector-contrib#18509 (comment), is there a way to declare dependencies between extensions into that graph?

[atoulme]

I believe this may also be required by the Encoding extensions (open-telemetry/opentelemetry-collector-contrib#28686), which is a similar use case.

The encoding extensions don't do much with their lifecycle, since the plan is just to cast them to marshaler/unmarshaler types.

I'm more worried that our current documentation is making a promise it is not keeping. I don't really see the harm in being deterministic in the start/shutdown order, it might pay off to have the pprof extension load before others for example.

[yurishkuro]

The encoding extensions don't do much with their lifecycle, since the plan is just to cast them to marshaler/unmarshaler types.

it's not about their lifecycle, but about other components that depend on them, some of which may also be extensions.

[yurishkuro]

I don't really see the harm in being deterministic in the start/shutdown order, it might pay off to have the pprof extension load before others for example.

I agree. Current non-determinism leads to difficult to troubleshoot bugs - it took me a while to realize that this was the root cause of the failures I encountered.

[djaglowski]

@djaglowski You mentioned service graph in open-telemetry/opentelemetry-collector-contrib#18509 (comment), is there a way to declare dependencies between extensions into that graph?

Not currently. The relationships between pipeline components are very rigidly defined, but the way extensions are used by pipeline components is more of a convention. e.g. the component defines a parameter called storage and expects to find an extension with that ID. We could in theory formalize this but it would be quite some work and it's not clear to me that it's necessary.

I agree extensions should be started deterministically. Since the current behavior is non-deterministic, users already must implicitly accept that extensions may start in any order, including the order we would choose deterministically.

Would it be sufficient to start extensions in order, then start pipeline components as we do now? Stopping would be the opposite - pipeline components, then extensions in reverse order.

[yurishkuro]

Would it be sufficient to start extensions in order, then start pipeline components as we do now? Stopping would be the opposite - pipeline components, then extensions in reverse order.

That would work for me, and what the open PR is doing.

[yurishkuro]

Just to hold off until discussion in #8732 is resolved

@dmitryax since you blocked the PR, could you explain what form of resolution you expect to happen?

[dmitryax]

The use case is explained here: https://docs.google.com/document/d/1s4_6VgAS7qAVp6iEm5KYvpiGw3h2Ja5T5HpKo29iv00/view#heading=h.1ded8prqa1xm

If users would have to point the jaeger_query extension and the exporter to the same storage extension, maybe it can be just one extension (jaeger_query or any other name) implementing both serving the query path and the storage interface that exporters can use. The storage part of the extension can be configurable to use any storage under the hood and provide it to the exporter via the storage interface. I believe, that way, we would avoid users misconfiguring exporter and the jaeger_query extension to use different storages. WDYT?

I agree. Current non-determinism leads to difficult to troubleshoot bugs - it took me a while to realize that this was the root cause of the failures I encountered.

I'm just worried that by introducing this behavior, we limit possible options for grouping/extending configs which are currently being discussed in different issues. Also, if we accept this change and decide to integrate the extensions into the pipeline graph later, it'll be a breaking change for the users expecting the order of initialization according to the config definition. And given that that order is currently not applicable to receivers and exporters, this change doesn't bring any consistency.