Repository navigation
[SDK] Add Entity support to Resource - #4490
shashankxrm wants to merge 22 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4490 +/- ##
==========================================
+ Coverage 86.69% 86.82% +0.14%
==========================================
Files 527 528 +1
Lines 20902 21129 +227
==========================================
+ Hits 18119 18344 +225
- Misses 2783 2785 +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.
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.
4f26c21 to
9b86ab5
Compare
om7057
left a comment
There was a problem hiding this comment.
Focused on the Resource::Create change since that overlaps with #4535, which I looked at closely while working on a duplicate fix for it (closed in favor of #4540).
This PR touches the exact crash site from #4535 but does not fix it. The renamed lookup (resource.attributes_ to resource_attributes = resource.GetAttributes()) still feeds into nostd::get<std::string>(it_process_executable_name->second) unguarded, so process.executable.name set to anything other than a std::string still throws bad_variant_access out of Create() here, same as before this PR.
Since #4540 already has an approved fix for this exact line (nostd::get_if<std::string> with a fallback to plain unknown_service), whichever of these two PRs merges second will need to reconcile with the other's change to this block anyway. Given this PR is already modifying these lines for the resource_attributes/entities rework, it would avoid that conflict and avoid carrying the crash forward if the same guard were adopted here directly rather than left for a follow-up rebase.
Thanks for catching this. You're right that Resource::Create could still throw bad_variant_access when process.executable.name is present with a non-string value. I've addressed this by guarding the lookup with nostd::get_ifstd::string. Non-string values now leave the fallback as unknown_service, while string values retain the existing unknown_service: behavior. I also added a regression test for the non-string case. This is included in the latest commit. |
e8c5770 to
c08b825
Compare
|
@lalitb Identity integers are now normalized to int64_t (including arrays and in-range uint32_t/uint64_t), while description attributes are unchanged. I also added entity and resource merge tests, including MergeIntegerWidthIdentityOverlaysDescription, to verify that int32_t{123} and int64_t{123} identities merge and the incoming description is retained. Please take another look when you have a chance. |
Resolve CHANGELOG.md conflict: keep the Entity support entry and upstream Unreleased entries, including the breaking-changes section.
|
Hi @lalitb, gentle ping on this when you have a moment. Since your review, I've addressed the integer identity concern: identity integers are now normalized to I also added the dbarker and ThomsonTan have approved, so yours is the remaining review. Could you take another look and update your review if this resolves your concern? Thanks! |
|
Hi @lalitb, gentle ping on this one. I addressed your earlier feedback, and it's been approved by @dbarker and @ThomsonTan . Could you take another look when you get a chance? @marcalff , if lalitb is busy, would it be possible to dismiss the stale review so this can move forward? Happy to make any further changes. |
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