Entity SDK - Initial opt-in SDK features#8464
Conversation
- Create new Entity Detectors that are completely separate from existing reosurce detectors so entity must be FULL opt-in before behavior changes, even if OTLP is non-breaking. - Wire "either/or" scenarios into resource factory and declarative config. TODOs we need to sort out: - Features of ResourceConfiguration spec that interact poorly with entities, e.g. attribute-specific filters that aren't entity aware, raw attribute definitions. - Adding `env` as a supported detector for reading in Entity env variable propagation. - Dealing with oddities of ResourceBuilder/Factory wanting to only use attributes in Java - we worked around it for now.
jack-berg
left a comment
There was a problem hiding this comment.
You'll need to handle some merge conflicts since declarative config has been undergoing construction:
- It moved out of the incubator into a dedicated artifact in #8265
- We started commiting generated POJOs to VCS in #8408
As mentioned in the SIG meeting, I don't think you need the incubator copies of Entity interfaces. I pushed a commit here to delete them: jack-berg@3ea17f4
Per the discussion, resource detectors will need to updated to optionally produce entity-aware resources. If the user opts in, then those resource detectors can call the internal APIs from opentelemetry-sdk-common to produce those.
| * | ||
| * @return a map of attributes. | ||
| */ | ||
| abstract Attributes getRawAttributes(); |
There was a problem hiding this comment.
These are loose attributes, i.e. those that weren't attached to an Enttiy. We need this for backwards compatibility
There was a problem hiding this comment.
Now that you're memoizing during initialization, is this still needed? I see exactly one usage in ResourceBuilder.putAll(Resource). I suppose keeping this preserves the rawness of the attributes (as opposed to being associated with an entity) during merging, but is there any functional difference from just changing:
To:
/** Puts all attributes from {@link Resource} into this. */
public ResourceBuilder putAll(Resource resource) {
if (resource != null) {
// Preserve entities when merging resources.
entities.addAll(resource.getEntities());
attributesBuilder.putAll(resource.getAttributes());
}
return this;
}
Or better yet, compute the raw attributes during putAll:
/** Puts all attributes from {@link Resource} into this. */
public ResourceBuilder putAll(Resource resource) {
if (resource != null) {
// Preserve entities when merging resources.
entities.addAll(resource.getEntities());
AttributesBuilder rawAttributes = resource.getAttributes().toBuilder();
rawAttributes.removeIf(
key ->
resource.getEntities().stream()
.anyMatch(
entity ->
entity.getId().get(key) != null
|| entity.getDescription().get(key) != null));
attributesBuilder.putAll(rawAttributes.build());
}
return this;
}
Or better better yet (😛), extract a dedicated static function for computing the raw attributes for a resource (since I did end up finding one more use of raw attributes in EntityUtil):
public static Attributes getRawAttributes(Resource resource) {
AttributesBuilder rawAttributes = resource.getAttributes().toBuilder();
rawAttributes.removeIf(
key ->
resource.getEntities().stream()
.anyMatch(
entity ->
entity.getId().get(key) != null
|| entity.getDescription().get(key) != null));
return rawAttributes.build();
}
| if (context.isEntitiesEnabled()) { | ||
| List<ExperimentalResourceDetectorModel> detectorModels = detectionModel.getDetectors(); | ||
| if (detectorModels != null) { | ||
| List<EntityDetector> detectors = new ArrayList<>(); |
There was a problem hiding this comment.
SPIs work differently in declarative config than in autoconfigure. Rather than having a dedicated SPI per extension plugin interface (i.e. ResourceProvider, ConfigurableSpanExporterProvider, etc), there's a single ComponentProvider SPI. Implementations specify the type of extension plugin interface they provide via ComponentProvider.getType().
Also, we we're discussing in DMs about adjusting the semantics of existing declarative configuration resource provider types to make them entity aware, such that YAML like:
resource:
attributes_list: ${OTEL_RESOURCE_ATTRIBUTES}
detection/development: # /development properties may not be supported in all SDKs
detectors:
- env: # We add this - it allows runtimes, like Cloud Run or AWS lambda to pass resource identity via ENV variable without blowing away existing working setups.
- service:
- host:
- process:
- container:
Which is valid today. The env, service, host, process, container detectors would be updated to produce entity-aware Resources.
Going this route, I think there's basically nothing that needs to be done in ResourceFactory. It will start configuring entity-aware resources when the detectors are updated to produce them.
There was a problem hiding this comment.
Yeah - I think this is a LOT cleaner and goes towards the no-breaking-change / easy-to-adopt goals we had for entities.
I'll cut all this out and come back to you with a renewed PR.
There was a problem hiding this comment.
Ok - first cut is done, doing some more cleanup - but probably worth a review already
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (77.45%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #8464 +/- ##
============================================
- Coverage 90.94% 90.82% -0.13%
- Complexity 10210 10269 +59
============================================
Files 1013 1020 +7
Lines 27175 27509 +334
Branches 3184 3238 +54
============================================
+ Hits 24714 24984 +270
- Misses 1734 1763 +29
- Partials 727 762 +35 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Simplify API for using entities. - Update so we can use current resource detection unchanged with entities. - Memoize full attributes manually when creating a resource.
| * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change | ||
| * at any time. | ||
| */ | ||
| final class EntityRefMarshaler extends MarshalerWithSize { |
There was a problem hiding this comment.
I don't believe this is wired into the ResourceMarshaler so it doesn't actually do anything. Let's either wire in, or pull until a future PR when we're wiring in. No use having unused code published.
There was a problem hiding this comment.
My bad I think I accidentally reverted that - will fix
| /** Constants for experimental entity SDK features. */ | ||
| final class EntityExperimentConstants { | ||
|
|
||
| static final String EXPERIMENTAL_ENTITIES_ENABLED = "otel.experimental.entities.enabled"; |
There was a problem hiding this comment.
Just add this as a constant in EnvironmentResource - no need for a separate class.
There was a problem hiding this comment.
This is used by all the resource detectors - I can put it in each individually or do you think they should use different values?
There was a problem hiding this comment.
You mean it will be used by all the resource detectors? Because right now I only see usage in one place - EnvironmentResrouce.
If its going to be used by resource detectors, should go in opentelemetry-sdk-extension-autoconfigure-spi because that's the dependency detectors take when implementing ResourceProvider
|
|
||
| private EnvironmentResource() {} | ||
|
|
||
| private static final class Segment { |
There was a problem hiding this comment.
Did you see the comment at the top of this class about us having build tooling to make a copy of this for declarative config?
* <p>This class is intentionally self-contained (no dependencies on other autoconfigure-internal
* classes) so that it can be copied wholesale into declarative configuration without pulling in
* additional dependencies. Do not add dependencies on non-API, non-SPI classes.
The idea is we want to reuse this logic for parsing .resource.attributes_list:
file_format: "1.1"
resource:
attributes_list: ${OTEL_RESOURCE_ATTRIBUTES}
Maybe you have the same thing in mind and need the entity parsing to come over to declarative config too? Actually, looking at the code, there is no opportunity for the declarative config code path to reach this entities stuff since it only spoofs otel.resource.attributes. Should break apart and wire in the entities bit in ResourceConfiguration.
There was a problem hiding this comment.
Yeah - I think likely we need to split some of this apart.
I think the way declarative config and entities interact with env is actually at odds - we've had this discussion in the past.
I'll detangle this and show you what I mean
| props.put(EntityExperimentConstants.EXPERIMENTAL_ENTITIES_ENABLED, "true"); | ||
| props.put( | ||
| EnvironmentResource.ENTITIES_PROPERTY, | ||
| "process{process.pid=1234}[process.executable.name=java]@http://schema;host{host.id=myhost}"); |
There was a problem hiding this comment.
Should probably add a parameterized test to hammer this entities parsing logic with a bunch of different success and failure cases.
There was a problem hiding this comment.
I was working on that :)
| otelJava.osgiServiceLoaderProvides.set(listOf( | ||
| "io.opentelemetry.sdk.autoconfigure.spi.ResourceProvider", | ||
| "io.opentelemetry.sdk.autoconfigure.spi.internal.ComponentProvider", | ||
| "io.opentelemetry.sdk.extension.incubator.resources.EntityDetector", |
| api(project(":sdk-extensions:autoconfigure-spi")) | ||
|
|
||
| compileOnly(project(":api:incubator")) | ||
| compileOnly(project(":sdk-extensions:incubator")) |
There was a problem hiding this comment.
I think both these changes in this file are remnant from when you still had work in the sdk-extensions:incubator.
| * | ||
| * @return a map of attributes. | ||
| */ | ||
| abstract Attributes getRawAttributes(); |
There was a problem hiding this comment.
Now that you're memoizing during initialization, is this still needed? I see exactly one usage in ResourceBuilder.putAll(Resource). I suppose keeping this preserves the rawness of the attributes (as opposed to being associated with an entity) during merging, but is there any functional difference from just changing:
To:
/** Puts all attributes from {@link Resource} into this. */
public ResourceBuilder putAll(Resource resource) {
if (resource != null) {
// Preserve entities when merging resources.
entities.addAll(resource.getEntities());
attributesBuilder.putAll(resource.getAttributes());
}
return this;
}
Or better yet, compute the raw attributes during putAll:
/** Puts all attributes from {@link Resource} into this. */
public ResourceBuilder putAll(Resource resource) {
if (resource != null) {
// Preserve entities when merging resources.
entities.addAll(resource.getEntities());
AttributesBuilder rawAttributes = resource.getAttributes().toBuilder();
rawAttributes.removeIf(
key ->
resource.getEntities().stream()
.anyMatch(
entity ->
entity.getId().get(key) != null
|| entity.getDescription().get(key) != null));
attributesBuilder.putAll(rawAttributes.build());
}
return this;
}
Or better better yet (😛), extract a dedicated static function for computing the raw attributes for a resource (since I did end up finding one more use of raw attributes in EntityUtil):
public static Attributes getRawAttributes(Resource resource) {
AttributesBuilder rawAttributes = resource.getAttributes().toBuilder();
rawAttributes.removeIf(
key ->
resource.getEntities().stream()
.anyMatch(
entity ->
entity.getId().get(key) != null
|| entity.getDescription().get(key) != null));
return rawAttributes.build();
}
| * @param entityType the entity type string of this entity. | ||
| */ | ||
| static EntityBuilder builder(String entityType) { | ||
| return SdkEntity.builder(entityType); |
There was a problem hiding this comment.
#nit:
| return SdkEntity.builder(entityType); | |
| return new SdkEntityBuilder(entityType) |
Can get rid of EntityBuilder builder(String) method from SdkEntity.
| * | ||
| * @return the entity identity. | ||
| */ | ||
| Attributes getId(); |
There was a problem hiding this comment.
Can entities have empty IDs? Curious if ID should be a required, non-empty constructor parameter.
| @SuppressWarnings("JdkObsolete") // Recommended alternative was introduced in java 10 | ||
| static Resource createEnvironmentResource(ConfigProperties config) { | ||
| boolean entitiesEnabled = | ||
| config.getBoolean(EntityExperimentConstants.EXPERIMENTAL_ENTITIES_ENABLED, false); |
There was a problem hiding this comment.
This is an interesting way to get around having OTEL_ENTITIES and not OTEL_EXPERIMENTAL_ENTITIES or something similarly qualified as experimental. Allows us to use the ultimate target env var OTEL_ENTITIES while retaining the ability to change the semantics since users have to opt in.
No precedent for this type of thing but I think its fine. WDYT @open-telemetry/java-approvers ?
This is a more up-to-date PR accounting for changes from OpenTelemetry Configuraiton work for Entities.
What this does:
sdk-extensions/incubatingA few issues though:
Resourceis exposed in the SDK as an autovalue - which has compatibility limitations. I"m not sure how to keep the auto API compat bot happy here as effectively you can't subclass this without breaking Java's visibiltiy rules, but also it appears like it can be subclassed if you do bytecode rewritting so it technically is a bincompat change.