-
Notifications
You must be signed in to change notification settings - Fork 893
EntityProvider prototype #7360
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
base: main
Are you sure you want to change the base?
EntityProvider prototype #7360
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7360 +/- ##
============================================
- Coverage 89.87% 89.85% -0.02%
- Complexity 6899 6921 +22
============================================
Files 786 789 +3
Lines 20793 20877 +84
Branches 2026 2034 +8
============================================
+ Hits 18687 18760 +73
- Misses 1465 1473 +8
- Partials 641 644 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start!!!
Just some random thoughts, brainstorms and questions on the implementation.
|
||
public interface EntityListener { | ||
|
||
void onEntityState(Entity state, Resource resource); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this returning a resource?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just following this part of the otep. My understanding was that it's something like "Here's the entity that changed and here's the new resource after it was applied".
Is it wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily, I was just curious on the motivation/use-case.
* be removed and a new instance will be inserted at the end of the list of entities. After the | ||
* entity is added, the resource is rebuilt and the listeners are notified. | ||
*/ | ||
void addEntity(String id, String name, Attributes attributes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Entities will need Schema URL with them.
I'd suggest dropping this method, even from the prototype (or keeping it private).
Feel free to copy-paste the entity + entity builder classes from my prototype (https://github.com/open-telemetry/opentelemetry-java/blob/cdd5f792b0f412948783e89762d2dc090fe77daa/sdk/common/src/main/java/io/opentelemetry/sdk/resources/EntityBuilder.java)
* the change is effectively made "in-place". After the Entity has been updated, the listeners | ||
* will be notified. | ||
*/ | ||
void updateEntity(String id, Attributes attributes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So entities are made of identifying and descriptive attributes and a type (or name).
I'd expect this method to either take in an entity wholesale or somehow take in EntityId
that is a combination of Identifying Attributes
, type/name
and SchemaUrl
.
* then it is effectively a no-op. If the Entity is removed, the Resource will be rebuilt and | ||
* listeners will then be notified. | ||
*/ | ||
void deleteEntity(String id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be by name/type or by all identifying components of the Entity?
My thinking is we may want both, or we want a "replace entity", something like:
void onSessionChange(Session session) {
entityManager.replaceEntity(createEntityFromSession(session);
}
public class SdkEntityProvider implements EntityProvider { | ||
|
||
private final Object lock = new Object(); | ||
private final AtomicReference<Resource> resource = new AtomicReference<>(Resource.empty()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this concurrency model.
You have a lock guarding the raw entity information, and then you publish Resource to threads via AtomicReference
.
Not needed for prototype, but I'd document this somewhere so it's easy to pick up and understand how to protect the resource reference.
private final AtomicReference<Resource> resource = new AtomicReference<>(Resource.empty()); | ||
|
||
@GuardedBy("lock") | ||
private final LinkedHashMap<String, Entity> entities = new LinkedHashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you using linked-hash-map to get predicatability in the final set of resource attributes or for another reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's to maintain the order in which the Entities were added, because that iteration order determines how the Resource gets merge()d to the final result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So resource.merge logic is in a previous otep.
For this prototype we shouldn't stress over details, but note that the previous otep as it reaches the specification will give you a merge method you can use here.
listeners = new ArrayList<>(this.listeners); | ||
} | ||
for (EntityListener listener : listeners) { | ||
listener.onEntityState(updatedEntity, resource); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: We may want to track whether there was an actual attribute change here before firing state.
Given we own the known state of the entity, it's unlikely callers of this method will actually know if an event they receive marks a real change in attirbutes, so we could filter down events to only real changes that have occurred.
Again, not needed for a prototype, just brainstorming features for final solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, that did occur to me as I was hacking this up. I would go so far as to say that there probably needs to be spec language that addresses this...something like "Add/update/delete actions that don't result in an underlying change to the Resource should not notify listeners and SHOULD be treated like a no-op".
|
||
private Resource rebuildResource() { | ||
Resource newResource = doRebuildResource(); | ||
resource.set(newResource); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, you can use lazySet
here to avoid flushing your local cache. The details on this are quite obtuse over time, but here's the best explanation (that I believe is still valid) for why: https://psy-lob-saw.blogspot.com/2012/12/atomiclazyset-is-performance-win-for.html
I believe you're abiding by single-writer principle, and this could help avoiding flushing cache on write until the time the rest of the SDK requests access to the resource.
* | ||
* @since ??.??.?? | ||
*/ | ||
public SdkTracerProviderBuilder addEntity(String id, String name, Attributes attributes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the plan to expose this on TraceProvider?
Why not force things through EntityProvider?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I dunno....this is almost certainly misplaced, good catch. I think I should remove it.
I think maybe I was just hammering through all the compilation problems and thought this needed parity with the addResource()
but that's not quite right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not sure how the user is supposed to get ahold of the EntityProvider
though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - I'm thinking we may need it to be an API level thing ... But hoping prototypes like this can teach us that.
@@ -76,19 +115,19 @@ public SdkTracerProviderBuilder setIdGenerator(IdGenerator idGenerator) { | |||
*/ | |||
public SdkTracerProviderBuilder setResource(Resource resource) { | |||
requireNonNull(resource, "resource"); | |||
this.resource = resource; | |||
this.entityProvider = new SdkEntityProvider(Collections.singletonList(Entity.create(resource))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to be much more sophisticated to deal with Resource vs. Entity shenanigans.
import io.opentelemetry.api.common.Attributes; | ||
import io.opentelemetry.sdk.resources.Resource; | ||
|
||
public interface EntityProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ResourceProvider?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if that's impossible, maybe EntitiesProvider?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about ResourceFromEntitiesProvider so that we can avoid the existing name conflict?
Here's an initial straw-man implementation in java of the
EntityProvider
described in otep-4316.My first impressions are that the implementation itself is pretty basic and open to future tweaking/improvements (it's really just supposed to be a caching thread-safe CRUD bucket), but the interesting and more complicated stuff begins to happen when the EntityProvider is integrated with or otherwise used by other places in the code.
Right now, it is wired up into the
SdkTracerProvider
and has exposed builder methods on theSdkTracerProviderBuilder
. I think there are bound to be breaking changes in that, so please feel free to call those out. Right now, there's also no way for users or instrumentations to get any handle to the "live"EntityProvider
in order to add/update/delete any entities...it would have to be wired up entirely by the user and retained.I also did not yet attempt to wire it up metrics or logs, because I figured there would be enough to talk about first with traces. :)