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 8 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 8 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 •
edited
Loading

Copy link
Copy Markdown

Pull request dashboard status

Waiting on the author · refreshed 2026-09-21 21:24 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.

@opentelemetry-pr-dashboard opentelemetry-pr-dashboard Bot mentioned this pull request Sep 14, 2026
Open
@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 Outdated
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.

@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 reviewed Sep 19, 2026
View reviewed changes

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
herin049
herin049 approved these changes Sep 21, 2026
View reviewed changes
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

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

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Lite
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@herin049 herin049 herin049 approved these changes
+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

Assignees

No one assigned

Labels

None yet

Projects

Python PR digest
Status: Approved PRs that need fixes

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

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