-
Notifications
You must be signed in to change notification settings - Fork 849
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Should unbind be replaced with Scope or similar try/resources, AutoCloseable pattern? #3495
Comments
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
i.e., bind multiple times to the same instrument? |
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. |
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. |
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 |
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. |
@jsuereth Found this issue - can you clarify whether |
Would be mooted by #3913 though |
Sorry I missed this.
I'm against removing unbind if we have bind because it'll lead to guaranteed memory leaks or reliance on |
You can call bind mulitiple times per-instrument w/ the same attributes. 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. |
@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? 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. |
Closing this since bound instruments were removed in #5157. |
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?The text was updated successfully, but these errors were encountered: