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.

Sign up for GitHub

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

Jump to bottom

sdk: Implement basic host resource detector #4182

Merged
merged 32 commits into from
Oct 15, 2024
Merged

sdk: Implement basic host resource detector #4182

merged 32 commits into from
Oct 15, 2024

Conversation

oliver-zhang
Copy link
Contributor

Description

This PR will create a resource module in contrib repo which will fetch the host.name，host.arch properties

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

  • Test A : By providing unit test cases

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

@oliver-zhang oliver-zhang requested a review from a team September 7, 2024 06:16
@oliver-zhang oliver-zhang changed the title Resource detector host sdk: Implement basic host resource detector Sep 7, 2024
pmcollins
pmcollins reviewed Sep 11, 2024
View reviewed changes
Copy link
Member

@pmcollins pmcollins left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. Added some comments.

opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py Outdated Show resolved Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py Outdated Show resolved Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py Outdated Show resolved Hide resolved
opentelemetry-sdk/tests/resources/test_resources.py Outdated Show resolved Hide resolved
@xrmx
Copy link
Contributor

xrmx commented Sep 12, 2024

@oliver-zhang please setup pre-commit (see CONTRIBUTING.md) so it'll take care of the formatting

@oliver-zhang
Copy link
Contributor Author

@oliver-zhang please setup pre-commit (see CONTRIBUTING.md) so it'll take care of the formatting
Ok, it's done

@pmcollins
Copy link
Member

Do we want to add this ResourceDetector to the list of entry points here or were you thinking that would be for another PR?

@emdneto
Copy link
Member

emdneto commented Sep 17, 2024

Do we want to add this ResourceDetector to the list of entry points here or were you thinking that would be for another PR?

In #3992 we added os resource detector to the list of entry points in the same PR

@emdneto emdneto added the Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary label Sep 17, 2024
@oliver-zhang
Copy link
Contributor Author

Do we want to add this ResourceDetector to the list of entry points here or were you thinking that would be for another PR?

In #3992 we added os resource detector to the list of entry points in the same PR

@emdneto Added to list of entry points

pmcollins
pmcollins approved these changes Sep 18, 2024
View reviewed changes
Copy link
Member

@pmcollins pmcollins left a comment

Choose a reason for hiding this comment

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

LGTM -- thanks for doing this. Left a comment about linting.

opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py Outdated Show resolved Hide resolved
@lzchen
Copy link
Contributor

lzchen commented Sep 18, 2024 •
edited
Loading

What should our guidance be for adding in resource detectors that have attributes that are not yet stable in the semantic conventions? Should we be including this as part of the sdk package? I know we might have (mistakenly) added the os resource detector as part of the sdk. Should we place these in contrib for now until sem convs are stable?

@lzchen
Copy link
Contributor

lzchen commented Sep 19, 2024

@oliver-zhang

I would be good with adding an underscore _ in front of the resource detector to indicate that it is currently in experimental state.

@oliver-zhang
Copy link
Contributor Author

@oliver-zhang

I would be good with adding an underscore _ in front of the resource detector to indicate that it is currently in experimental state.

@lzchen It's done

@oliver-zhang oliver-zhang reopened this Sep 20, 2024
@pmcollins
Copy link
Member

Looks like we are good to merge, no? There's a change requested but it looks like it has been addressed. cc @xrmx 🙏

@xrmx xrmx merged commit a36587c into open-telemetry:main Oct 15, 2024
346 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmcollins pmcollins pmcollins approved these changes

@xrmx xrmx xrmx approved these changes

@emdneto emdneto emdneto approved these changes

@lzchen lzchen lzchen approved these changes

Assignees
No one assigned
Labels
Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@oliver-zhang @xrmx @pmcollins @emdneto @lzchen