Skip to content

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

@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

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.

@lzchen
Copy link
Contributor

lzchen commented Sep 18, 2024

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

@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

Labels

Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants