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 9 commits into
open-telemetry:mainfrom
rads-1996:fix-detector-logic
Open

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

Conversation

@rads-1996

@rads-1996 rads-1996 commented Sep 14, 2026

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-22 15:25 UTC

Respond to 1 review item (e.g. link a commit, explain why not, ask a follow-up):

  • Top-level threads: 1
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.
  • Should this be with reviewers? Comment /dashboard route:reviewers to route it to them.
  • Anything wrong — including the routing? Report it with what you expected; it helps us improve the dashboard.

@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
Comment thread opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py Outdated

@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.

@rads-1996 rads-1996 changed the title [opentelemetry-sdk] Fix overwriting of the service.instance.id which has been populated through the resource detectors (WIP)[opentelemetry-sdk] Fix overwriting of the service.instance.id which has been populated through the resource detectors Sep 16, 2026
@rads-1996
rads-1996 force-pushed the fix-detector-logic branch 2 times, most recently from e9e6ae6 to fb426c2 Compare September 17, 2026 16:49
@rads-1996 rads-1996 changed the title (WIP)[opentelemetry-sdk] Fix overwriting 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 17, 2026
@rads-1996
rads-1996 requested a review from herin049 September 17, 2026 17:57
@herin049
herin049 requested a lite review from Copilot September 19, 2026 03:14

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Explicit detector ordering and fork-time resource refresh can still overwrite configured IDs, and direct user/environment precedence lacks coverage.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 Medium severity · 1 Low severity

Open (3)
What changed in this PR

This PR adjusts resource detector ordering to prevent generated service.instance.id values from overriding configured detector values.

Changes:

  • Prepends the service-instance detector.
  • Adds detector regression tests.
  • Adds a changelog entry.
File Description
opentelemetry-sdk/​src/​opentelemetry/​sdk/​resources/​__init__.py Changes detector ordering.
opentelemetry-sdk/​tests/​resources/​test_resources.py Adds service-instance regression tests.
.changelog/​5660.fixed Documents the fix.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py Outdated
Comment thread opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py Outdated
Comment thread opentelemetry-sdk/tests/resources/test_resources.py Outdated
@herin049 herin049 moved this to Reviewed PRs that need fixes in Python PR digest Sep 21, 2026
Comment thread opentelemetry-sdk/tests/resources/test_resources.py Outdated
@herin049 herin049 moved this from Reviewed PRs that need fixes to Approved PRs that need fixes in Python PR digest Sep 21, 2026
@github-project-automation github-project-automation Bot moved this from Approved PRs that need fixes to Approved PRs in Python PR digest Sep 22, 2026
@lzchen
lzchen added this pull request to the merge queue Sep 22, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 22, 2026

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

Status: Approved PRs

Development

Successfully merging this pull request may close these issues.

6 participants