TracerProvider.shutdown should be optional

[anuraaga]

After doing some investigation into usage of the SDK, I'm feeling that TracerProvider.shutdown may not be the best idiom at least in all languages. For example, a stateful span processor may be shared by multiple providers, and this seems reasonable. But this allows one provider to shutdown another one. It seems like if a span processor has state, it should be closed independently, similar to many objects that are shared, such as any old gRPC client used in an app. In Java this would be very idiomatic by just having anything that has state (e.g. Batch span processor, exporter) to implement Closeable - it makes it obvious what needs to be closed, and that can happen maybe with try/resources, or a DI container, without doing so in the TracerProvider, which is really just a bag of objects, not in itself requiring closing. Of course the shutdown maybe idiomatic in other languages so I don't know if it needs to be removed completely but it seems nice to give options that can use language features.

[Oberon00]

I propose to forbid using the same SpanProcessor for multiple TracerProvider, unless the SpanProcessor explicitly supports it. Similarly I think we should forbid using the same Exporter for multiple SpanProcessors unless it explicitly supports it. I would much prefer this solution, it would also make other things much easier to reason about & implement, e.g. you don't need to be prepared for having different resources (maybe even in the same batch) in your exporter.

An alternative solution only for the shutdown problem would be using something like COM's AddRef/Release mechanism. It could work as follows: If a SpanProcessor is added to a TracerProvider, so that the TracerProvider's shutdown would call the shutdown of the SpanProcessor, a new method Attach is called on the SpanProcessor. That way, stateful SpanProcessors can implement reference counting to only actually shut down once. This reference counting could also be implemented in a composable wrapper class e.g. SpanProcessor.refCountedAttach(naiveSpanProcessor).

without doing so in the TracerProvider, which is really just a bag of objects, not in itself requiring closing

Counter-argument: "A HTTP connection is just a bag of file descriptors, but the connection itself does not require closing". This "bag of smaller closeable thing" pattern applies to most closeable objects.

Ultimately this is a question of ownership. Does the TracerProvider (exclusively) own its SpanProcessors? Then it should shut them down. Otherwise, the user is responsible for shutting stuff down, but only after any objects referencing it are shut down if applicable.

I'm feeling that TracerProvider.shutdown may not be the best idiom at least in all languages

I agree, for example Rust and C++ have better solutions available (Drop traits, destructor, shared_pointer, Arc). But Java has no better solution available IMHO. Whether the SDK is owning the TracerProvider or not should be the same across all languages.

[anuraaga]

I would much prefer this solution, it would also make other things much easier to reason about & implement, e.g. you don't need to be prepared for having different resources (maybe even in the same batch) in your exporter.

Are you suggesting a change to the exporter API to provide Resource separately from SpanData? I think this can be done pretty easily as just passing them as separate parameters to export without losing the ability to share the exporter - pushing the Resource into the exporter at construction time so it's really only seen once seems complex though maybe doable - I guess instead of adding exporters to providers, we add exporter factories?

But the restriction seems more complicated than just allowing sharing in general, which our API seems to have no corner cases with.

Also, even if we did this, it still doesn't necessarily mean shutdown on the provider is idiomatic more talking points below.

Counter-argument: "A HTTP connection is just a bag of file descriptors, but the connection itself does not require closing". This "bag of smaller closeable thing" pattern applies to most closeable objects.

I would say the difference with a TracerProvider is a connection must always be closed, while a tracer provider doesn't necessarily, e.g. if it uses just a logging exporter, there are actually no closeable objects there. So I think this optional-ness of the resources in a TracerProvider indicates that having it be shutdown is doing it at the wrong layer. I would generally except the object that must always be closed, which includes wrappers of such types, to have a close method, but not others (since why not just close what needs to be closed).

Agree though about ownership, if we say the TracerProvider owns the span processors, it probably should shut it down. I would want to model that in the API by configuring the tracerprovider with factories rather than actual objects though - I don't see a clean way to enforce the ownership otherwise (the reference counting could work, but it seems to be more complex than just allow user, or more likely framework, to own).

Also one more point is about scalability - for example, will we add Sampler.shutdown? Remote samplers will have resources and will need to be closed - but it seems quite sane to allow the users (frameworks) to handle this instead of adding yet another object that can't be shared / needs to be shutdown by the TracerProvider. Without adding Sampler.shutdown, TracerProvider.shutdown seems to not actually be fitting the bill, I'd suggest precisely because it's overextending its bounds.

[andrewhsu]

discussed at the spec sig mtg, but would be helpful to have more input from @anuraaga at the spec mtg later today if possible

[tedsuo]

Summing up discussion from the Spec call:

• We are thinking that, for now, we want to discourage users to share components, and wait for real world use cases to motivate that requirement. In the meantime, a user could implement a ref counted wrapper if they wanted to share components.

• We think that Samplers may also need to have shutdown, given that we may have remote samplers.

• We think that SDKs should have a global shutdown mechanism, which users call, rather than managing the shutdown of individual components directly.

[carlosalberto]

I suggest we fill an additional issue for the Samplers part, and we mark this issue as after-GA. Does it make sense @anuraaga @tedsuo ?

[anuraaga]

@carlosalberto Yeah filed #1295. Thanks