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

Allow to collect host architecture #1483

Merged
merged 8 commits into from
Mar 5, 2021
Merged

Allow to collect host architecture #1483

merged 8 commits into from
Mar 5, 2021

Conversation

rakyll
Copy link
Contributor

@rakyll rakyll commented Mar 1, 2021

Introduce "arch" to represent the CPU architecture of the host, e.g. the processor on a physical or virtual host.
We may predefine some commonly used architecture as a follow up to this change.

Host type is not enough to represent the processor architecture
of the host. Add "arch" to allow users to collecting processor
architecture. We may predefine some commonly used architecture
as a follow up to this change.
@rakyll rakyll requested review from a team as code owners March 1, 2021 05:58
@arminru arminru added area:semantic-conventions Related to semantic conventions spec:resource Related to the specification/resource directory labels Mar 1, 2021
@Oberon00
Copy link
Member

Oberon00 commented Mar 1, 2021

I think we should look a bit into how this would work with "mixed-arch" scenarios. E.g. it is common to have a 32 bit process running on an 64 bit OS on x86_64. That gives potential for os.arch, process.arch in addition to host.arch in this PR. Or is host.arch the only architecture, and the rest would only be ABIs? Interesting in that regard is the existence of the x32 ABI, i.e. 64 bit instructions with 32 bit pointers on a 64 bit Linux.

@rakyll
Copy link
Contributor Author

rakyll commented Mar 1, 2021

@Oberon00 good point about how this will affect the architecture we may collect for the process and OS. Let me think about this a bit before we merge anything. @anuraaga I'll also provide a predefined list on the next iteration.

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@rakyll
Copy link
Contributor Author

rakyll commented Mar 3, 2021

PTAL

semantic_conventions/resource/host.yaml Outdated Show resolved Hide resolved
| `host.image.name` | string | Name of the VM image or OS install the host was instantiated from. | `infra-ami-eks-worker-node-7d4ec78312`; `CentOS-8-x86_64-1905` | No |
| `host.image.id` | string | VM image ID. For Cloud, this value is from the provider. | `ami-07b06b442921831e5` | No |
| `host.image.version` | string | The version string of the VM image as defined in [Version Attributes](README.md#version-attributes). | `0.1` | No |

`host.arch` MUST be one of the following or, if none of the listed values apply, a custom value:
Copy link
Contributor

Choose a reason for hiding this comment

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

Values here neither match the common convention for Linux / Debian arch, e.g. not matching what's reported by uname -m, nor match what Node.js os.arch() API would have reported; nor what Rust architecture uses for the standard set of arch.... For different languages - you'd have different strings. And none of them would match the list proposed here 😄 So it's going to be quite an adventure to normalize to that proposed list 🤷

I believe a better set could be this:

Architecture Applies to
x86 x86, wow64, i386, i486, etc.
amd64 x86_64, amd64, x64
arm32 arm32
arm64 arm64, aarch64
ppc32 32-bit PowerPC
ppc64 64-bit PowerPC
ia64 Itanium
other indistinguishable, or not listed here. Architecture as reported by the language of choice without normalizing it / without bringing it to common denominator.

C++, Rust, or Node - typically have their own API to obtain the process arch string. This string is going to be in their own common format. For Node - this list is well-described here: https://nodejs.org/api/os.html#os_os_arch - I personally believe Node's list is quite reasonable.

More complete set of architectures is described here in Linux kernel tree:
https://github.com/torvalds/linux/tree/master/arch

If you truly intend to describe the host arch , it would be reasonable to respect the Linux standard names for the archs, as the most prominent host OS. That's also what uname -m command would typically report, namely [arm|arm64|ia64|mips|x86]. I would've left the loophole in this list to allow reporting what OS language-specific API reports as architecture. Any data analysis within a slice of data reported by agents written in specific language is consistent: a language (e.g. node) would report only a fixed set of archs. There is also not going to be any need to normalize.

I don't like this proposed list because it does not seem to align with the arch lists typically captured by prominent language APIs, neither matches the Linux kernel arch list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks so much better. Go has a similar approach here. I initially wanted to propose something more aligned with uname but the formatting of OS names made me want to follow the UPPER_CASE style here. I'm modifying this PR in a bit.

@rakyll
Copy link
Contributor Author

rakyll commented Mar 3, 2021

PTAL

@rakyll
Copy link
Contributor Author

rakyll commented Mar 4, 2021

PTAL

@Oberon00 Oberon00 dismissed their stale review March 4, 2021 11:00

AMD64 / x86-64 duplication is resolved

@carlosalberto
Copy link
Contributor

@Oberon00 @maxgolov Please confirm this is ready to go (I think it is ;) )

@rakyll Usually marking comments as resolved makes it easier to review, btw.

@rakyll
Copy link
Contributor Author

rakyll commented Mar 5, 2021

@carlosalberto will resolve them from this point on, thanks for pointing it out.

@carlosalberto
Copy link
Contributor

@maxgolov @Oberon00 Feedback as been properly addressed. In case of any further detail, please fill a follow up ;)

@carlosalberto carlosalberto merged commit 664d7ca into open-telemetry:main Mar 5, 2021
ThomsonTan pushed a commit to ThomsonTan/opentelemetry-specification that referenced this pull request Mar 30, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions spec:resource Related to the specification/resource directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants