Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Small fixups for resource detector code and tests #345

Merged
merged 3 commits into from
Jul 30, 2024

Conversation

aabmass
Copy link
Collaborator

@aabmass aabmass commented Jul 22, 2024

  • Updated the tests to not leak into os.environ
  • Fix code to expect numeric value for instance/id metadata
  • Mark metadata response JSON type annotation for instance.attributes as not-total (keys are not all required)

@aabmass aabmass changed the title Cleanup resource detector code and tests Small fixups for resource detector code and tests Jul 22, 2024
@aabmass aabmass force-pushed the resource-fixup-1 branch from d5cd63f to ed8ed26 Compare July 22, 2024 22:15
@aabmass aabmass marked this pull request as ready for review July 22, 2024 22:16
@aabmass aabmass requested a review from a team as a code owner July 22, 2024 22:16
@aabmass aabmass force-pushed the resource-fixup-1 branch 2 times, most recently from 24f4638 to 7340837 Compare July 23, 2024 03:57
- Updated the tests to not leak into os.environ
- Fix code to expect numeric value for `instance/id` metadata
- Mark `instance/attributes` as not-total (keys are not all required)
@aabmass aabmass force-pushed the resource-fixup-1 branch from 7340837 to f41b97a Compare July 23, 2024 16:58
@dashpole
Copy link
Contributor

is instance/id usually an int? I thought it was a UID on cloud run...

@aabmass
Copy link
Collaborator Author

aabmass commented Jul 30, 2024

is instance/id usually an int? I thought it was a UID on cloud run...

Hmm that's a good point. On GCE it's numeric. Let me see if I can just treat it as an opaque string I don't remember why I changed it anymore

I can type annotate the id key to be either str or int. I think the code is correct either way as we do str() conversion

@aabmass
Copy link
Collaborator Author

aabmass commented Jul 30, 2024

The "ops-python-e2e-cloud-functions-gen2 (opentelemetry-ops-e2e)" CI failure is because I enabled the checked but implementation is in a draft PR still. Merging

@aabmass aabmass merged commit a7fcc52 into GoogleCloudPlatform:main Jul 30, 2024
19 of 20 checks passed
@aabmass aabmass deleted the resource-fixup-1 branch July 30, 2024 22:39
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.

3 participants