sdk-common: add ComponentRegistry
#375
Conversation
iRevive
force-pushed
the
sdk-trace/components-registry
branch
3 times, most recently
from
4d28519 to
dcf0fd1
Compare
iRevive
force-pushed
the
sdk-trace/components-registry
branch
from
dcf0fd1 to
befcb77
Compare
Since it's unspecified, what are the pros/cons of of caching and returning the same instances? |
| schemaUrl: Option[String], | ||
| attributes: Attributes | ||
| ): F[A] = | ||
| cache.evalModify { cache => |
There was a problem hiding this comment.
This implementation is a bit more defensive than the Java implementation from what I can make out.
Specifically, you are guaranteeing that buildComponent(...) will never be run twice for the same scope. However, the Java implementation doesn't actually seem to be guaranteeing that.
There was a problem hiding this comment.
I believe you're mistaken. it took a bit of digging, but it seems that ConcurrentHashMap#computeIfAbsent is guaranteed to execute the function exactly once if the key is absent, and exactly zero times if already present. this is specific to ConcurrentHashMap and not true of all ConcurrentMap implementations (e.g. ConcurrentSkipListMap)
There was a problem hiding this comment.
Aha you're right, thanks for taking a close look. I missed that the component is being added to two collections, and the lock is specifically for the allComponents collection.
It all depends on the use cases. Let's say you have several independent components within the library and each of them can be allocated independently. I assume, in Java world, especially with DI and reflective allocations, the caching may be useful. |
Yes, but why: what exactly is the optimization? Are we just saving memory, or are we saving some other (scarce) resource by sharing? |
| schemaUrl: Option[String], | ||
| attributes: Attributes | ||
| ): F[A] = | ||
| cache.evalModify { cache => |
There was a problem hiding this comment.
perhaps this is nitpicky, but I feel like it would be clearer without the name shadowing. perhaps map instead of cache for the function parameter?
| cache.evalModify { cache => | ||
| val key = Key(name, version, schemaUrl) | ||
|
|
||
| cache.get(key) match { |
There was a problem hiding this comment.
some part of me is going "but could this be implemented using updatedWith?" but idk if it makes the code any cleaner or is worth the effort
| v1 <- registry.get(name, None, None, Attributes.Empty) | ||
| v2 <- registry.get(name, None, None, attributes) | ||
| v3 <- registry.get(name, Some(version), None, attributes) | ||
| v4 <- registry.get(name, Some(version), Some(schemaUrl), attributes) |
There was a problem hiding this comment.
missing a case in each of the three tests for name and schema only, as well as a fourth test for name and schema only
| schemaUrl: Option[String], | ||
| attributes: Attributes | ||
| ): F[A] = | ||
| cache.evalModify { cache => |
There was a problem hiding this comment.
after thinking about it, it might be worth having a call to cache.get outside of the evalModify so that we can opportunistically avoid blocking if the cache is already populated
I still don't have an answer, to be honest. We also preserve the uniqueness of the instruments (meters, tracers). Perhaps if we allow multiple meters with the same name/version/schemaUrl, it may lead affect the exported values. So, since we don't know the component's purpose (at least yet). Should we hold off on the PR? |
I don't have a strong opinion. On the one hand, the Java SDK did it, so that's a precedent that it might be useful. On the other hand, this is adding an (almost certainly inconsequential) overhead due to synchronization and keeping stuff in memory indefinitely ... |
|
Let's move forward without component registry. It can be easily added later. |
|
Closing for now. |
The spec says:
Note
Tracers are identified by
name,version, andschema_urlfields. When more than one Tracer of the samename,version, andschema_urlis created, it is unspecified whether or under which conditions the same or different Tracer instances are returned. It is a user error to create Tracers with different attributes but the same identity.Java SDK uses ComponentRegistry to prevent duplicates.
Here is an example of how
ComponentRegistrycould be used in the SDK module:https://github.com/typelevel/otel4s/pull/325/files#diff-96194a2d20a59c6f3717b447b20305201f52f19802d11b677580d86839d69d4bR40.