Repository navigation
Conversation
Pull request dashboard statusWaiting on maintainers · refreshed 2026-10-20 20:25 UTC Merge when ready. Status above doesn't look right?
|
| * iteration - all detectors are cached at construction. TTL-based cache invalidation using this | ||
| * flag is planned for a future iteration. | ||
| */ | ||
| boolean shouldReinvoke(); |
There was a problem hiding this comment.
Since we're not doing anything with this today, we could punt on this. But the whole idea is sort of contrived and pointless without it. There's no need for a dedicated ResourceDetector / ResourceProvider framework when Resource / ResourceBuilder already allow you to iteratively construct a resource.
But Resource/ResourceBuilder break down with entity requirements. The design in this PR tries to accomplish the core goal of making SDK resource information accessible to things like opamp, process context sharing, etc with a forward looking api.
There was a problem hiding this comment.
Seems like a bit of future-proofing, which I think is usually discouraged in public APIs since it's so easily added later when it's needed. I'd leave it out, but don't feel strongly about it.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8865 +/- ##
============================================
- Coverage 90.61% 90.41% -0.21%
- Complexity 10882 10946 +64
============================================
Files 1015 1018 +3
Lines 29731 29976 +245
Branches 3876 3908 +32
============================================
+ Hits 26942 27104 +162
- Misses 1872 1921 +49
- Partials 917 951 +34 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| import io.opentelemetry.sdk.resources.Resource; | ||
| import javax.annotation.Nullable; | ||
|
|
||
| final class ConstantResourceDetector implements ResourceDetector { |
There was a problem hiding this comment.
oooh - interesting. Do you think this needs to be one of the 'out of the box' specified detectors?
There was a problem hiding this comment.
Not sure. I did this because I had to. I think it makes sense to build SdkResourceProvider around the idea that resource attributes may change at runtime via entity descriptive attributes, but in practice all resources today (and most in the future) are statically evaluated at config time. So having syntactic sugar to reflect the common case is a must.
BTW I decided (b82c1bd) that the whole notion of a ResourceDetector should be hidden from users. Given that we already have ResourceProvider and ComponentProvider (i.e the declarative config version), having yet another interface for providing resources is too confusing a UX.
Instead, we can just have methods in SdkResourceProviderBuilder which reflect the different styles of resources that might need to be associated with a provider:
public final class SdkResourceProviderBuilder {
public SdkResourceProviderBuilder addConstantResource(Resource resource);
public SdkResourceProviderBuilder addConstantResource(Resource resource, @Nullable String name);
public SdkResourceProvider build();
// Proposed future expansion
public SdkResourceProviderBuilder addDetector(Supplier<Resource> resourceSupplier, Duration refreshInterval);
public SdkResourceProviderBuilder addDetector(Supplier<Resource> resourceSupplier, Duration refreshInterval, @Nullable String name);
}
jsuereth
left a comment
jsuereth
left a comment
There was a problem hiding this comment.
Awesome! This looks a lot like an earlier prototype of Entities I had, but a good bit cleaner.
…ource to SdkResourceProviderBuilder
…y-java into sdk-resource-provider
|
|
||
| SdkResourceProvider(List<ResourceDetector> detectors) { | ||
| this.detectors = Collections.unmodifiableList(new ArrayList<>(detectors)); | ||
| this.cachedResource = merge(this.detectors); |
There was a problem hiding this comment.
Because the detectors might be doing actual, real work (like going out and discovering or finding stuff)....I don't love the idea of escaping the constructor doing work in the constructor like this, as I think it can lead to a weird class of problem that's hard to troubleshoot, and object construction having side effects is usually unexpected behavior.
Prefer a factory method and passing in the true dependency: Resource.
There was a problem hiding this comment.
Ok maybe escaping the constructor usually means leaking this outside the class...so that's not really happening here, but it still makes me twitchy because object construction could have side effects.
There was a problem hiding this comment.
Not sure what you mean by escaping the constructor.
This looks odd right now because all detectors are static. But in the future, we want to add detectors that needs to be periodically refreshed.
breedx-splk
left a comment
breedx-splk
left a comment
There was a problem hiding this comment.
Looking good here. There's already some approvals, so I'm just holding off to see if any of my suggestions merit follow-ups, but this is mostly solid to me!
This branch has not been deployed
5 participants
Introduce SdkResourceProvider, representing the ResourceProvider spec concept added in opentelemetry-specification#5057.
Shape:
In followups, we need to:
This functionality segues nicely with @jsuereth's #8464: each entity aware resource detector becomes one of these new ResourceDetector instances and are added to SdkResourceProvider. The work josh has done to update the Resource#merge behavior to merge entities is brought over here for free. And we have a natural starting point for refreshing / recomputing resource / entities.
@open-telemetry/java-approvers heads up on this - as well as embodying a new spec concept and helping the entities story, this is the key to unblocking the two additional use cases we identified where consumers of the SDK need to access the resolved resource: opamp and process context sharing.