[SDK] Add Entity support to Resource - #4490
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4490 +/- ##
==========================================
+ Coverage 86.41% 86.55% +0.14%
==========================================
Files 524 525 +1
Lines 20435 20660 +225
==========================================
+ Hits 17657 17880 +223
- Misses 2778 2780 +2
🚀 New features to boost your workflow:
|
|
Hi @dbarker, I’ve opened a PR implementing the SDK Resource Entity support discussed here: #3652 It covers the Resource data model, entity-aware Create/Merge behavior, flattening, and tests. Detectors and OTLP integration are intentionally left for the follow-up steps you mentioned. All CI checks are currently passing. Would appreciate your feedback when you get a chance. |
Thanks for the PR! I will review later this week. We are working towards a release soon and this PR likely be held back until the release is completed. |
Thanks for the update! |
dbarker
left a comment
There was a problem hiding this comment.
Thanks for the PR! Looks good overall and covers the Resource API expansion from spec 1.60.0
Createnow takes Entities- Retrieve entities and unassociated attributes is supported with new accessors
The merge algorithms appear well tested with the legacy merge without entities is unaffected.
Please see minor feedback below.
dbarker
left a comment
There was a problem hiding this comment.
Thanks for the updates. Approved with a request for minor cleanup and to add comments with spec details on the merge algorithms.
|
|
||
| const std::string classic_schema = | ||
| updating.GetSchemaURL().empty() ? GetSchemaURL() : updating.GetSchemaURL(); | ||
| return Resource(unassociated, classic_schema, after_eviction); |
There was a problem hiding this comment.
Constructor normalization drops conflicting entities before loose-attribute cleanup and schema selection. This can retain a stale service.name and report schema-A instead of the required empty schema. Please compute both results from the pre-conflict entity set in the merge path, then preserve them through entity removal.
| attributes_.insert(entity.GetIdentity().begin(), entity.GetIdentity().end()); | ||
| attributes_.insert(entity.GetDescription().begin(), entity.GetDescription().end()); | ||
| } | ||
| attributes_.insert(unassociated_attributes_.begin(), unassociated_attributes_.end()); |
There was a problem hiding this comment.
Non-blocking: attribute-only resources now retain two owned copies of their attributes, so callers that never use entities still pay extra storage and copying costs. Please either avoid the second map for entity-free resources or document the intentional tradeoff. A fast path must also update the getter, legacy merge, and service-name fallback rather than merely skipping cache construction.
There was a problem hiding this comment.
Thanks, addressed. Attribute-only Resources now use unassociated_attributes_ directly as the flattened representation, while attributes_ remains the cache for entity-aware Resources.
I also updated GetAttributes(), MergeWithoutEntities(), and the service-name fallback accordingly, with regression coverage for the entity-free fallback and the all-entities-evicted merge case.
| return existing.GetIdentity() == incoming.GetIdentity() && | ||
| existing.GetSchemaURL() == incoming.GetSchemaURL(); | ||
| } |
There was a problem hiding this comment.
Should these identities match when only the C++ integer width differs? For example, two detectors could report the same process.pid as int32_t{123} and int64_t{123}. With the same entity type and schema URL, this still rejects the merge, so any additional description from the second detector is ignored. Is that intentional, or should we compare these as the same integer value?
There was a problem hiding this comment.
Entity identity comparison reuses ResourceAttributes / OwnedAttributeValue equality rather than defining a separate numeric comparison. In the C++ SDK, different integer variant alternatives are type-strict, so int32_t{123} and int64_t{123} are not equal, consistent with the existing attribute equality behavior.
The OpenTelemetry data model has a single signed 64-bit integer type; the C++ SDK supports additional integer widths as representation types. We don't want to introduce integer-width normalization only for Entity identity, since that would make Entity identity semantics differ from ResourceAttributes and Entity::operator==.
I will add a regression test documenting the current type-strict identity behavior. Callers should use a consistent integer representation for identifying attributes (preferably int64_t).
Happy to hear your thoughts if you see this differently.
There was a problem hiding this comment.
Thanks for clarifying. I still think these identities should match. OpenTelemetry has one signed integer type, and OTLP exports int32_t{123} and int64_t{123} identically. Rejecting the merge because of the C++ width loses the second entity’s description, even though both identify the same entity.
Could we normalize integer identity values to int64_t in the Entity constructor, including integer arrays and uint64_t values that fit? That would keep identity matching and Entity::operator== consistent without changing ResourceAttributes globally. Please also add a test showing that these two representations merge and retain the incoming description.
lalitb
left a comment
There was a problem hiding this comment.
The overall approach looks good. I’m requesting changes until we clarify the integer identity comparison below, since two detectors reporting the same process ID could fail to merge, losing additional details from the second detector.
Part of #3652. This PR implements the SDK Resource portion of entity propagation; follow-up work remains as noted below.
Changes
Adds Entity support to the SDK
Resourcelibrary, which is the next step identified on #3652 now thatEnvEntityDetector/OTEL_ENTITIESparsing exists (#3795).This PR:
Entityvalue type (type, identity, description, schema URL).Resourcetogether with unassociated attributes.GetAttributes()as the flattened view: entity identity and description plus unassociated attributes, as required when entities are present.GetEntities()andGetUnassociatedAttributes().Resourceconstructor that accepts attributes, schema URL, and entities, with entity validation and normalization.Mergewhen either resource contains entities.Resource::Create(attributes, schema_url, entities)while leaving the existing two-argumentCreatein place.sdk/test/resourcecoverage (newentity_test, plus Resource construction / merge / Create cases).This does not complete entity propagation end-to-end. Detectors still emit flattened attributes, and exporters are unchanged.
Merge behavior
Attribute-only merge is unchanged: if neither resource has entities, the existing attribute merge behavior is preserved.
If either resource has entities, merge follows the resource data model:
These rules match specification Examples 1–3 (loose attribute vs entity, updating loose attribute evicting an entity, same-type identity mismatch plus key conflict dropping the lower-priority entity).
Two points are specified less tightly than the examples, so this implementation is an interpretation:
hostand dropservice.Createstill doesGetDefault().Merge(OTELResourceDetector).Merge(user resource). When both attributes and entities are passed, the user resource is built so entity-owned keys are not left as unassociated attributes (equivalent to creating from attributes and then merging a resource that holds those entities).[service.name](http://service.name/)fallback inspects flattened attributes so an entity-owned[service.name](http://service.name/)is not overwritten.Compatibility
Resourceconstructors are unchanged.Resource::Create(attributes, schema_url)is unchanged for callers; it delegates to the new overload with an empty entity list.Mergebehavior is preserved.Entity, getters, three-argument constructor,Createoverload).Resourcenow storesentities_andunassociated_attributes_in addition to the flattenedattributes_map, so object layout changes. This is an SDK type; this PR does not claim ABI stability across versions.Scope / Follow-up work
This PR is limited to the SDK Resource library, as discussed on #3652: add entities to
Resourcefirst, then populate detectors and OTLP.Intentionally not in this PR:
Entityobjects from resource detectors (includingEnvEntityDetector, which still returns flattened attributes).OTEL_ENTITIESparsing.entity_refs(or exporter attribute helpers).ResourceDetector::Createoverload that accepts entities.EnvEntityDetectorfrom defaultResource::Create()(it remains opt-in viaopentelemetry_resource_detectors).Those are follow-up steps, not omissions.
Testing
Locally:
entity_test: 8/8 passedresource_test: 51/51 passed (existing Resource tests kept as regression coverage)clang-format-18 --Werror -npassedCI has not run on this PR yet.
References
EnvEntityDetector; out of scope here)CHANGELOG.mdupdated for non-trivial changes