Repository navigation
Conversation
Pull request dashboard statusWaiting on the author · refreshed 2026-10-17 04:16 UTC Respond to 9 review items (e.g. link a commit, explain why not, ask a follow-up): 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.65% +0.03%
- Complexity 10882 10930 +48
============================================
Files 1015 1018 +3
Lines 29731 29849 +118
Branches 3876 3882 +6
============================================
+ Hits 26942 27060 +118
Misses 1872 1872
Partials 917 917 ☔ 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
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
| && tracerResource.equals(loggerRp.getResource())) { | ||
| return tracerRp; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
it throws an exception above when getSdkResourceProvider() is called. That should do it huh?
| openTelemetrySdk.getPropagators()); | ||
| this.openTelemetrySdk = openTelemetrySdk; | ||
| this.configProvider = new ObfuscatedConfigProvider(configProvider); | ||
| this.resourceProvider = resolveSdkResourceProvider(openTelemetrySdk); |
There was a problem hiding this comment.
Nit: This feels like too much "work in the constructor". Prefer moving the resolve call to the create() method and passing in the true dependency (ResourceProvider).
|
|
||
| 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.
| if (resourceProvider != null && resourceExplicitlySet) { | ||
| throw new IllegalStateException( | ||
| "setSdkResourceProvider is mutually exclusive with setResource / addResource."); | ||
| } |
There was a problem hiding this comment.
I anticipate this being a little tricky to get thoroughly complete. Would moving this to the setters (and changing the wording) help us better understand the ordering when it happens (eg. was the resource set before the provider, or the other way around?)?
There was a problem hiding this comment.
Isn't the intention here to make sure that only one of setResource or setSdkResourceProvider was used?
I think its independent of order in which these methods were invoked, right ?
| if (resourceProvider != null && resourceExplicitlySet) { | ||
| throw new IllegalStateException( | ||
| "setSdkResourceProvider is mutually exclusive with setResource / addResource."); | ||
| } |
There was a problem hiding this comment.
Same comment/thought as above -- if we moved this check into the setters, can we both drop the explicit boolean member and just throw on set with better visibility into the ordering problem?
| if (resourceProvider != null && resourceExplicitlySet) { | ||
| throw new IllegalStateException( | ||
| "setSdkResourceProvider is mutually exclusive with setResource / addResource."); | ||
| } |
There was a problem hiding this comment.
same comment as the other two builders
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!
| .addConstantResource(RESOURCE_B) | ||
| .build(); | ||
|
|
||
| assertThat(provider.getResource()).isEqualTo(RESOURCE_A.merge(RESOURCE_B)); |
There was a problem hiding this comment.
nit: I think we need an overlapping attribute to actually test the merge order.
| if (resourceProvider != null && resourceExplicitlySet) { | ||
| throw new IllegalStateException( | ||
| "setSdkResourceProvider is mutually exclusive with setResource / addResource."); | ||
| } |
There was a problem hiding this comment.
Isn't the intention here to make sure that only one of setResource or setSdkResourceProvider was used?
I think its independent of order in which these methods were invoked, right ?
| private static Resource merge(List<ResourceDetector> detectors) { | ||
| Resource resource = Resource.empty(); | ||
| for (ResourceDetector detector : detectors) { | ||
| resource = resource.merge(detector.getResource()); |
There was a problem hiding this comment.
Just wanted confirm the pairwise merge functionality here. The documentation of the Resource.merge dictates that if the two resources being merged have non-null schema types, then the resulting merged resource has no schema URL
So when merging 3 or more resources with all different, non-null schema types then the resulting merged resource would have the schema type of the last resource.
Is this the expected behavior?
| * Returns the name of this detector, or {@code null} if unnamed. Names are informational (e.g. | ||
| * for logging / debugging) and are not required to be unique. |
There was a problem hiding this comment.
I got confused by this Javadoc because the spec on Resource Detector Name suggests that it should be unique and this Javadoc explicitly states otherwise.
Looking closely, this is probably not the responsible component for how the resource detectors are looked up. I'm thinking adding some clarification to the Javadoc around what this method is actually intended to be used for would be good.
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.