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

Add device.manufacturer to semantic conventions for resources #2100

Merged
merged 8 commits into from
Dec 16, 2021
Merged

Add device.manufacturer to semantic conventions for resources #2100

merged 8 commits into from
Dec 16, 2021

Conversation

ladd
Copy link
Contributor

@ladd ladd commented Nov 4, 2021 •
edited
Loading

Changes

  • Proposed manufacturer field

@ladd ladd requested review from a team November 4, 2021 21:00
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 4, 2021 •
edited
Loading

CLA Signed

The committers are authorized under a signed CLA.

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 16, 2021
@ladd
Copy link
Contributor Author

ladd commented Nov 16, 2021

Could I get a review, please?

@arminru arminru added area:semantic-conventions Related to semantic conventions spec:resource Related to the specification/resource directory labels Nov 16, 2021
@arminru arminru changed the title Update device.md Add device.manufacturer to semantic conventions for resources Nov 16, 2021
@arminru
Copy link
Member

arminru commented Nov 16, 2021

@ladd Please note that you will need to update the YAML definitions in this folder following the readme there:
https://github.com/open-telemetry/opentelemetry-specification/tree/main/semantic_conventions
By running the command documented there, the markdown table you edited manually will be generated automatically an make the build pass.

@ladd
Copy link
Contributor Author

ladd commented Nov 16, 2021

@ladd Please note that you will need to update the YAML definitions in this folder following the readme there: https://github.com/open-telemetry/opentelemetry-specification/tree/main/semantic_conventions By running the command documented there, the markdown table you edited manually will be generated automatically an make the build pass.

Thanks -- done!

@Oberon00
Copy link
Member

Is this something we'd actually want to send? Since we already send device IDs, couldn't this be handled with a device database at the backend? Or do you think the practical advantages will be enough? Does one usually have an API that the instrumentation could call to get the manufacturer? Or would it have to have a DB?

@ladd
Copy link
Contributor Author

ladd commented Nov 16, 2021

Device ids are typically not mappable to manufacturer strings, unless you have a separate instrumentation mechanism to populate a DB. Device model strings can sometimes be mapped, but it normally requires licensing a proprietary database (like GSM Arena) I do think the practical advantages are there -- specific Android manufacturers often tweak the OS in compatibility breaking ways. The API exists for Android -- see my footnote in the PR. For iOS, you would normally just hardcode Apple.

@github-actions github-actions bot removed the Stale label Nov 17, 2021
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 25, 2021
@ladd
Copy link
Contributor Author

ladd commented Nov 26, 2021

Can I get a review / merge if the comments have been addressed?

Oberon00
Oberon00 approved these changes Nov 26, 2021
View reviewed changes
Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

Please add a CHANGELOG entry.

semantic_conventions/resource/device.yaml Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the Stale label Nov 27, 2021
@ladd
Copy link
Contributor Author

ladd commented Dec 6, 2021

Can I get a review / merge if the comments have been addressed?

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Dec 14, 2021
@jsuereth jsuereth enabled auto-merge (squash) December 15, 2021 17:08
auto-merge was automatically disabled December 15, 2021 17:57

Head branch was pushed to by a user without write access

ladd added 2 commits December 15, 2021 13:31
@ladd
Update device.md
2790052 
Proposed manufacturer field
@ladd
Update device.md
f583720
@jsuereth jsuereth enabled auto-merge (squash) December 16, 2021 00:21
@jsuereth jsuereth merged commit 86b4585 into open-telemetry:main Dec 16, 2021
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@ladd @jsuereth
Add device.manufacturer to semantic conventions for resources (open…
750faac 
…-telemetry#2100)

* Update device.md

Proposed manufacturer field

* Update device.md

* Generate tables

* PR comments

* Remove trailing whitespace

* Update CHANGELOG.md

Make linter happy?

* Fix merge

Co-authored-by: Josh Suereth <joshuasuereth@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iNikem iNikem iNikem approved these changes

@Oberon00 Oberon00 Oberon00 approved these changes

@trask trask trask approved these changes

@jsuereth jsuereth jsuereth approved these changes

Assignees

@jsuereth jsuereth

Labels
area:semantic-conventions Related to semantic conventions spec:resource Related to the specification/resource directory Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ladd @arminru @Oberon00 @jsuereth @trask @iNikem