Should unbind be replaced with Scope or similar try/resources, AutoCloseable pattern?

[anuraaga]

I noticed that it's a bit weird that unbind invalidates the variable as a side-effect - would it be more idiomatic to use try/resources instead?

[anuraaga]

Also is unbind necessary? Or is it ok to just let the bound counter get garbage-collected? I'm not sure so to confirm, is it possible to call

bound1 = counter.bind(attrs1);
bound2 = counter.bind(attrs2);
bound1.unbind()
bound2.unbind()

i.e., bind multiple times to the same instrument?

[jkwatson]

I don't think try-with-resources would work at all, since the unbinding is generally going to happen when a component is being shutdown, not right after the binding occurs.

Absolutely, you can create multiple bindings on the same instrument. The intent is that when you are binding the attributes, you can avoid any allocations in the bound instrument's usage. So you definitely might want to have multiple bindings on the same instrument to cover various fixed code-paths that might occur. A common usage I've seen is to have one binding for an error'd request, and one for a non-error.

[anuraaga]

since the unbinding is generally going to happen when a component is being shutdown

If this means app shutdown, then does it mean we can avoid having an unbind? If it's not an OS resource I guess closings isn't required. But not sure I follow completely.

For what it's worth on instrumentation we have a few (but very few...) bind and no unbind.

[jkwatson]

It can mean app shutdown, but you might also imagine having some kafka listener (or something) that gets started up only under special circumstances in the application lifecycle, and then gets shutdown again while the app continues to run. I don't know how common this use-case is, I definitely will admit.

@jsuereth do you have some common scenarios where unbind will be necessary, rather than just letting the app shut down and take all the bindings with it?

[anuraaga]

Ah by the way I guess I meant AutoCloseable more precisely than try/ resources. So it could be used for the latter but could also be unbound automatically by Spring on shutdown for example. Though again want to confirm this would be worth it since it's not something like a file descriptor, I think.

[anuraaga]

@jsuereth Found this issue - can you clarify whether unbind is really necessary?

[anuraaga]

Would be mooted by #3913 though

[jsuereth]

@jsuereth Found this issue - can you clarify whether unbind is really necessary?

Sorry I missed this.

1. Unbind will be necessary (at a minimum) at the SDK level.
2. common usage of metrics likely will not need an unbind that is public, as @jkwatson suggests. The idea here is pre-allocate memory for serving metrics to avoid GC in critical hot-paths.
3. I'm really hesitant giving people a way to allocate memory without de-allocating. John's right that component or process shutdown will need to clean up bound instruments. I think it's highly relevant for advanced scenarios (like containerized web-servers, tomcat, etc.) where you want to bind a bunch of metrics for a dynamically loaded application. I also suspect it's an API that's in the 5% of advanced cases, where it's critical.

I'm against removing unbind if we have bind because it'll lead to guaranteed memory leaks or reliance on finalize. I also think Scope doesn't make as much sense here because the lifetime of this style of thing is not tied to thread+stack, almost the inverse (we expect bound instruments to be used across a myriad of threads).

[jsuereth]

Also is unbind necessary? Or is it ok to just let the bound counter get garbage-collected? I'm not sure so to confirm, is it possible to call

bound1 = counter.bind(attrs1);
bound2 = counter.bind(attrs2);
bound1.unbind()
bound2.unbind()

i.e., bind multiple times to the same instrument?

You can call bind mulitiple times per-instrument w/ the same attributes. bind uses a ref-counting cleanup mechanism behind the scenes (details in AggregatorHandle).

Today, AggregatorHandle is shared-reference in the SDK metric cache, so it's preserved until that ref-count hits 0. This means GC never has a chance to collect (because SDK will reference it), so even implementing finalize really won't fix anything here.

[anuraaga]

@jsuereth Thanks for being ok with continuing to iterate on bound independent of the API stable release. I looked into the implementation to understand the reference counting - is it true that the reason for the reference counting is to unmap it from the collection storage?

https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/DeltaMetricStorage.java#L135

That was the only usage I could grok of it. If true, do we actually need this in addition to other mechanisms to controlling cardinality that we need, bound or no bound? I was actually somewhat surprised to see us removing aggregators from storage so aggressively on collection as almost all the time they would be recreated anyways. If an LRU eviction mechanism would overlap with the unbind use case, perhaps we can get away without unbind in the API simplifying it's usage.

[jack-berg]

Closing this since bound instruments were removed in #5157.