Skip to content

[opentelemetry-sdk] Fix overwriting of the service.instance.id which has been populated through the resource detectors - #5660

Open
rads-1996 wants to merge 2 commits into
open-telemetry:mainfrom
rads-1996:fix-detector-logic
Open

[opentelemetry-sdk] Fix overwriting of the service.instance.id which has been populated through the resource detectors#5660
rads-1996 wants to merge 2 commits into
open-telemetry:mainfrom
rads-1996:fix-detector-logic

Conversation

@rads-1996

@rads-1996 rads-1996 commented Sep 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Prevent ServiceInstanceIdResourceDetector from overwriting an existing service.instance.id set by another resource detector(ex. Azure) or user configuration. Add regression coverage for detector-, initial-resource-, and environment-provided IDs, while retaining UUID generation when no ID is present.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Unit tests for opentelemetry-sdk
  • E2E application has been tested to ensure the relevant service instance id is populated

Does This PR Require a Contrib Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@rads-1996 rads-1996 changed the title Fix overwritting of the service.instance.id which has been populated from the user provided values from the resource detectors (WIP) Fix overwritting of the service.instance.id which has been populated from the user provided values from the resource detectors Sep 14, 2026
@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Sep 14, 2026

Copy link
Copy Markdown

Pull request dashboard status

Waiting on the author · refreshed 2026-09-14 16:34 UTC

Move out of draft to request review.

Status above doesn't look right?
  • Just replied or pushed? Anything around or after the refresh time above may not be picked up yet — give it a few minutes.
  • Anything look wrong? Report it with what you expected; it helps us improve the dashboard.

@opentelemetry-pr-dashboard opentelemetry-pr-dashboard Bot mentioned this pull request Sep 14, 2026
Open
@rads-1996
Fix overwritting of the service.instance.id which has been populated …
e476652 
…from the user provided values from the resource detectors
@rads-1996 rads-1996 changed the title (WIP) Fix overwritting of the service.instance.id which has been populated from the user provided values from the resource detectors [opentelemetry-sdk] Fix overwritting of the service.instance.id which has been populated from the user provided values from the resource detectors Sep 14, 2026
@rads-1996 rads-1996 changed the title [opentelemetry-sdk] Fix overwritting of the service.instance.id which has been populated from the user provided values from the resource detectors [opentelemetry-sdk] Fix overriding of the service.instance.id which has been populated from the user provided values from the resource detectors Sep 14, 2026
@rads-1996 rads-1996 changed the title [opentelemetry-sdk] Fix overriding of the service.instance.id which has been populated from the user provided values from the resource detectors [opentelemetry-sdk] Fix overriding of the service.instance.id which has been populated from the user provided values through the resource detectors Sep 14, 2026
@rads-1996
rads-1996 marked this pull request as ready for review September 14, 2026 22:36
@rads-1996
rads-1996 requested a review from a team as a code owner September 14, 2026 22:36
@rads-1996 rads-1996 changed the title [opentelemetry-sdk] Fix overriding of the service.instance.id which has been populated from the user provided values through the resource detectors [opentelemetry-sdk] Fix overriding of the service.instance.id which has been populated through the resource detectors Sep 14, 2026
@rads-1996 rads-1996 changed the title [opentelemetry-sdk] Fix overriding of the service.instance.id which has been populated through the resource detectors [opentelemetry-sdk] Fix overwriting of the service.instance.id which has been populated through the resource detectors Sep 14, 2026
herin049
herin049 reviewed Sep 14, 2026
View reviewed changes
Comment thread opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py
detected_resource = future.result(timeout=timeout)

if (
isinstance(detector, ServiceInstanceIdResourceDetector)

@herin049 herin049 Sep 14, 2026

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.

Can we note just move this detector to the front of the detectors list?

@jeremydvoss jeremydvoss Sep 14, 2026

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.

Agreed that there should be a more generalized solution pattern. Is it established though that detectors that go later should always override ones that came before? Because it seems like it would actually be more intuitive that detectors that come earlier should take precedent. I'm just not sure if there's spec on this exact issue.

@rads-1996 rads-1996 Sep 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That is a good suggestion as it would handle initial resource creation by allowing later platform or user-configured detectors to override the generated UUID.

However, these changes would not cover post-fork refreshes, where only process-dependent detectors are rerun and the newly generated UUID could overwrite the existing platform/user-provided service.instance.id.

We could handle that separately by updating the fork-refresh logic to replace the instance ID only when the current value is the SDK-generated ID; otherwise, preserve the existing value.

@herin049 herin049 Sep 15, 2026

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.

However, these changes would not cover post-fork refreshes, where only process-dependent detectors are rerun and the newly generated UUID could overwrite the existing platform/user-provided service.instance.id.

If a resource detector populates service.instance.id, then it should regenerate it post fork (i.e. it needs to be a process-dependent resource detector) per the spec: service.namespace,service.name,service.instance.id should be globally unique.

We could handle that separately by updating the fork-refresh logic to replace the instance ID only when the current value is the SDK-generated ID; otherwise, preserve the existing value.

This could result in multiple processes having the same service.namespace,service.name,service.instance.id tuple.

Shriprasad-P
Shriprasad-P reviewed Sep 16, 2026
View reviewed changes

@Shriprasad-P Shriprasad-P left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for the PR. Approach looks sensible — please confirm edge cases and failure paths are covered by tests.

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

Reviewers

@herin049 herin049 herin049 left review comments
+2 more reviewers
@jeremydvoss jeremydvoss jeremydvoss left review comments
@Shriprasad-P Shriprasad-P Shriprasad-P left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

Python PR digest
Status: No status

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rads-1996 @jeremydvoss @herin049 @Shriprasad-P