Skip to content

Conversation

@xrmx
Copy link
Contributor

@xrmx xrmx commented Dec 19, 2024

We should create a Resource instance and not use Resource.create because if we set OTEL_EXPERIMENTAL_RESOURCE_DETECTORS we will go into an infinite loop trying to load and instantiate all the resources detectors.

See Resource.create implementation

Fix #363

@xrmx xrmx requested a review from a team as a code owner December 19, 2024 15:00
@aabmass
Copy link
Collaborator

aabmass commented Dec 23, 2024

We have discourage using the constructor. If we only call the constructor, there is no way to get these attributes https://github.com/open-telemetry/opentelemetry-python/blob/b48223561acfdffd1411ed7d015ddabe6f6eb801/opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py#L217-L227 or OTEL_RESOURCE_ATTRIBUTES based ones.

I think we might need to fix it this in the SDK or at least update the documentation to say that detectors MUST call the constructor. It seems a bit fragile overall, wdyt?

@aabmass
Copy link
Collaborator

aabmass commented Dec 23, 2024

/gcbrun

@xrmx
Copy link
Contributor Author

xrmx commented Dec 23, 2024

We have discourage using the constructor. If we only call the constructor, there is no way to get these attributes https://github.com/open-telemetry/opentelemetry-python/blob/b48223561acfdffd1411ed7d015ddabe6f6eb801/opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py#L217-L227 or OTEL_RESOURCE_ATTRIBUTES based ones.

I think we might need to fix it this in the SDK or at least update the documentation to say that detectors MUST call the constructor. It seems a bit fragile overall, wdyt?

Opened a pr to update the docs

@aabmass
Copy link
Collaborator

aabmass commented Dec 23, 2024

/gcbrun

@aabmass
Copy link
Collaborator

aabmass commented Dec 23, 2024

@xrmx to fix the tests, can you regenerate the snapshots by running? That will highlight the difference in behavior too

tox -f py3xx-test -- --snapshot-update

xrmx added 2 commits December 24, 2024 10:13
We should create a Resource instance and not use Resource.create because
if we set OTEL_EXPERIMENTAL_RESOURCE_DETECTORS we will go into an
infinite loop trying to load and instantiate all the resources
detectors.

Fix GoogleCloudPlatform#363
@xrmx xrmx force-pushed the fix-detector-resource branch from 6e6a165 to b0ff830 Compare December 24, 2024 09:14
@xrmx
Copy link
Contributor Author

xrmx commented Dec 24, 2024

@xrmx to fix the tests, can you regenerate the snapshots by running? That will highlight the difference in behavior too

tox -f py3xx-test -- --snapshot-update

Updated the snapshosts, thanks for the hint

@aabmass
Copy link
Collaborator

aabmass commented Dec 24, 2024

/gcbrun

@aabmass aabmass enabled auto-merge (squash) December 24, 2024 15:09
@aabmass aabmass merged commit d7fe99e into GoogleCloudPlatform:main Dec 26, 2024
22 checks passed
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.

_detector.GoogleCloudResourceDetector misbehaving on Buildkite

2 participants