Skip to content

[SDK] Add Entity support to Resource - #4490

Open
shashankxrm wants to merge 22 commits into
open-telemetry:mainfrom
shashankxrm:fix/resource-entity-3652
Open

shashankxrm wants to merge 22 commits into
open-telemetry:mainfrom
shashankxrm:fix/resource-entity-3652

Conversation

@shashankxrm

Copy link
Copy Markdown
Contributor

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 Resource library, which is the next step identified on #3652 now that EnvEntityDetector / OTEL_ENTITIES parsing exists (#3795).

This PR:

  • Introduces an Entity value type (type, identity, description, schema URL).
  • Stores entities on Resource together with unassociated attributes.
  • Keeps GetAttributes() as the flattened view: entity identity and description plus unassociated attributes, as required when entities are present.
  • Adds GetEntities() and GetUnassociatedAttributes().
  • Adds a three-argument Resource constructor that accepts attributes, schema URL, and entities, with entity validation and normalization.
  • Adds entity-aware Merge when either resource contains entities.
  • Adds Resource::Create(attributes, schema_url, entities) while leaving the existing two-argument Create in place.
  • Extends sdk/test/resource coverage (new entity_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:

  • Start from the old resource’s entities. For each updating entity of the same type, overlay descriptions if identity and schema URL match; otherwise keep the old entity (identity or schema mismatch). New types are appended.
  • Updating unassociated attributes take precedence over old unassociated attributes (same map-insert rule as today).
  • An updating loose attribute that uses a key owned by an entity evicts that entity (specification Example 2).
  • Entity type priority is taken from the updating resource’s entity order first, then types that exist only on the old resource. After ranking, construction-time key uniqueness drops the lower-priority entity when two entities share an attribute key.
  • If surviving entities share one schema URL, that becomes the resource schema URL; if they differ, the resource schema URL is blank. If no entities survive, the historical (non-entity) schema URL rule is used.

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:

  • Priority across two full resources. The data model describes merging entities in priority order (typically detector order). Here, types present on the updating resource rank above types that exist only on the old resource, which is what Example 3 requires in order to keep host and drop service.
  • Schema URL when the entity set becomes empty. The “all surviving entities share a schema URL” rule does not apply; this code falls back to the historical schema URL merge.

Create still does GetDefault().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

  • Existing two-argument Resource constructors are unchanged.
  • Existing Resource::Create(attributes, schema_url) is unchanged for callers; it delegates to the new overload with an empty entity list.
  • Attribute-only Merge behavior is preserved.
  • New APIs are additive (Entity, getters, three-argument constructor, Create overload).

Resource now stores entities_ and unassociated_attributes_ in addition to the flattened attributes_ 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 Resource first, then populate detectors and OTLP.

Intentionally not in this PR:

  • Populating Entity objects from resource detectors (including EnvEntityDetector, which still returns flattened attributes).
  • Changing OTEL_ENTITIES parsing.
  • OTLP / protobuf entity_refs (or exporter attribute helpers).
  • ResourceDetector::Create overload that accepts entities.
  • Running EnvEntityDetector from default Resource::Create() (it remains opt-in via opentelemetry_resource_detectors).
  • SdkBuilder / declarative configuration.

Those are follow-up steps, not omissions.

Testing

Locally:

  • entity_test: 8/8 passed
  • resource_test: 51/51 passed (existing Resource tests kept as regression coverage)
  • clang-format-18 --Werror -n passed

CI has not run on this PR yet.

References

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@shashankxrm
shashankxrm requested a review from a team as a code owner August 27, 2026 06:21
@codecov

codecov Bot commented Aug 27, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.75000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.82%. Comparing base (c7b64ec) to head (9479f1a).

Files with missing lines Patch % Lines
sdk/src/resource/resource.cc 98.39% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            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     
Files with missing lines Coverage Δ
sdk/include/opentelemetry/sdk/resource/resource.h 100.00% <ø> (ø)
sdk/src/resource/entity.cc 100.00% <100.00%> (ø)
sdk/src/resource/resource.cc 98.06% <98.39%> (+3.95%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@shashankxrm

Copy link
Copy Markdown
Contributor Author

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.

@dbarker

dbarker commented Aug 31, 2026

Copy link
Copy Markdown
Member

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.

@shashankxrm

Copy link
Copy Markdown
Contributor Author

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 dbarker left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Looks good overall and covers the Resource API expansion from spec 1.60.0

  • Create now 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 dbarker left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates. Approved with a request for minor cleanup and to add comments with spec details on the merge algorithms.

Comment thread sdk/src/resource/resource.cc Resolved
Comment thread sdk/src/resource/resource.cc Resolved
Comment thread sdk/src/resource/resource.cc Resolved
Comment thread sdk/src/resource/resource.cc Resolved

@lalitb lalitb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@shashankxrm
shashankxrm force-pushed the fix/resource-entity-3652 branch from 4f26c21 to 9b86ab5 Compare September 14, 2026 01:42

@om7057 om7057 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@shashankxrm

Copy link
Copy Markdown
Contributor Author

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.

@shashankxrm
shashankxrm force-pushed the fix/resource-entity-3652 branch from e8c5770 to c08b825 Compare September 22, 2026 20:51
@shashankxrm

Copy link
Copy Markdown
Contributor Author

@lalitb
Thanks for the follow-up. I agree with your proposal and implemented integer normalization in the Entity constructor in f65b142.

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.

dbarker and others added 2 commits September 29, 2026 13:34
Resolve CHANGELOG.md conflict: keep the Entity support entry and
upstream Unreleased entries, including the breaking-changes section.
@shashankxrm

Copy link
Copy Markdown
Contributor Author

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 int64_t in the Entity constructor (f65b142), including arrays and in-range uint32_t/uint64_t, while description attributes are left unchanged. MergeIntegerWidthIdentityOverlaysDescription covers the case you raised, checking that int32_t{123} and int64_t{123} identities merge and the second detector's description is kept.

I also added the nostd::get_if<std::string> guard in Resource::Create so a non-string process.executable.name no longer throws, with a regression test for it.

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!

@shashankxrm
shashankxrm requested a review from lalitb October 12, 2026 13:27
@shashankxrm

Copy link
Copy Markdown
Contributor Author

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants