OS description attribute detector

[nelsonghezzi]

This is a follow up of #1788 that I had in the works.

It adds the following resource options functions:

• WithOSDescription: detects and adds the os.description attribute.
• WithOS: detects and adds both os.type and os.description attributes.

Although the examples in the spec only shows a short, high level description, I thought it would be useful to include a bit more detail.

As the purpose of this attribute is to provide human-readable information, I think there's room to add details when it makes sense, and no implementation (processor, observability backend, etc) should make assumptions on its format or amount of information included.

Example captures

Here are some captures showing how the OS description is displayed for different OSes.

Windows

Ubuntu (running natively on WSL2)

Alpine (container)

Debian (container)

macOS

Implementation details

A reference to golang.org/x/sys was added to access platform specific syscalls and functions.

The primary (and only) function consumed by the osDescriptionDetector is platformOSDescription(). This function returns a (string, error) with the description of the OS.

There are two definitions for this function: one for Unix-like OSes and other for Windows.

In the case of *NIX systems, the OS description is composed by two parts:

• Unix name (uname): this is implemented the same for all *NIX systems thanks to the golang.org/x/sys package, which hides differences for systems which doesn't have a proper uname syscall (like Darwin). This part of the OS description it's assumed to exist on all these systems. Implemented as an uname() Go function.
• OS Release information: this has two implementations, one for systems supporting os-release files (only tested on some linux distros, but it seems FreeBSD follows the same convention), and a separate impementation for Darwin, which has the release information in a different file and format. This part of the OS description is optional, meaning that if errors are found building the OS release string, it's not included in the final OS description. Implemented as an osRelease() Go function.

The implementation is distributed in the following files:

• os_unix.go: it defines the platformOSDescription() and uname() functions, common for al *NIX systems. Build tags are included to target these common OSes.
• os_release_unix.go: it defines the osRelease() for al *NIX systems except Darwin. The darwin tag is omitted.
• os_release_darwin.go: it defines the osRelease() for Darwin.
• os_windows.go: it defines the platformOSDescription() for Windows.

When it comes to os-release files, there are differences between distros on the number of properties present and the amount of detail in their values.

Based on the following examples:

• os-release_alpine.txt
• os-release_debian.txt
• os-release_ubuntu.txt

I took some decisions on which properties to select to compose the final release description string, trying to maximize the amount of detail and avoiding redundancy. This is of course open to debate.

Tests for most unexported functions are TODO at the moment, but can/should be included.

Let me know what you think and any improvements you would make.

[codecov]

Codecov Report

Merging #1840 (1e7645f) into main (d8c9a95) will increase coverage by 0.0%.
The diff coverage is 81.6%.

@@          Coverage Diff          @@
##            main   #1840   +/-   ##
=====================================
  Coverage   72.7%   72.8%           
=====================================
  Files        164     166    +2     
  Lines       8081    8168   +87     
=====================================
+ Hits        5878    5949   +71     
- Misses      1974    1989   +15     
- Partials     229     230    +1     

Impacted Files	Coverage Δ
sdk/resource/os_unix.go	74.0% <74.0%> (ø)
sdk/resource/os_release_unix.go	84.7% <84.7%> (ø)
sdk/resource/os.go	90.0% <85.7%> (-10.0%)	⬇️

[nelsonghezzi]

I've added an os_unsupported.go file with a placeholder implementation of platformOSDescription.

Right now it doesn't include android or illumos GOOSs in the build tags because something unexpected happened while building the sdk module for those platforms (for example, with GOOS=android go build ./...):

# go.opentelemetry.io/otel/sdk/resource
resource/os_unsupported.go:23:6: platformOSDescription redeclared in this block
	previous declaration at resource/os_unix.go:43:39

It seems that android is aliased to linux, and illumos to solaris somehow 🤔. Made a quick check by removing the linux and solaris build tags from os_unix.go (while keeping android and illumos on os_unsupported.go) and the duplicate declaration error disappeared.

Makes sense? Good news is that as it's now, the project also compiles under these four GOOSs (and maybe even the OS description detector runs OK).

[cbandy]

👍🏻 I love linked references!

[nelsonghezzi]

Glad you liked it!

[cbandy]

These files aren't removed if the test fails. I'm not sure what Go versions we intend to support in tests, but

• Go 1.14 has t.Cleanup() which acts a bit like defer. It will call something even if the test fails.
• Go 1.15 has t.TempDir() which is automatically cleaned up.
• Go 1.16 has effectively deprecated the ioutil package.

[nelsonghezzi]

I originally went with os.CreateTemp instead of ioutil.TempFile, but reverted back (725d90f) when the CI failed for Go < 1.16.

Go 1.14 was still listed on the project's README when I started to work on this, but it has been updated since then to reflect only the last two versions of Go as supported, so I guess it's safe to assume we can use t.TempDir().

I've updated the code based on your suggestion to use t.TempDir() here ea07b21, and realized I can use t.Cleanup() in a couple of other places (f94b783).

I will leave this thread open just in case a maintainer has something to comment on.

[cbandy]

Should this be the opposite of the known/supported tags? Like, "not darwin and not linux and not windows…"

The question I suppose is what should happen for a new or unexpected GOOS? By filling this file with negations, a new GOOS will successfully build using this file. Supporting it would be a matter of including its tag in its implementation and adding the negation here.

[nelsonghezzi]

I see your point here. The negation approach will be more future-proof, guaranteeing that the project will always build.

Including all but Linux, macOS and Windows (!linux,!darwin,!windows) on the os_unsupported file will be the right way to demonstrate on which platforms we tested on, and thus are confident the resource works.

On the other hand, I wasn't sure to preclude the availability of a working OS description for *BSD in that way. Altought I haven't tested on such platforms, technically it should work.

Going with the current approach, in the wake of a new GOOS, it shouldn't be much of a hassle to include it either, but it can be more daunting for someone that is just learning about the project to face a compilation error at first try.

I think if we go for the "strict negation" strategy, the other _unix files should be stripped from all but the linux and darwin tags too.

[cbandy]

I don't mean to limit it to those three. I didn't want to type out everything in _unix in my question.

I don't have as strong opinion on what counts as "supported". Your choice to trust the sys/unix package makes sense to me.

[nelsonghezzi]

I don't mean to limit it to those three. I didn't want to type out everything in _unix in my question.

Oh, got it! Got confused for a sec because Linux, macOS and Windows are the platforms with a "compatibility guarantee" (as stated in the README) so I thought you were referring exclusively to them.

Pushed this change for others to also see the final form of this approach.

[nelsonghezzi]

Hi! Just wanted to ask if we're OK to merge this one, or if it's preferred to postpone it after the v1.0.0 milestone.

I'm good either way, will check periodically to keep it merge-ready.

[Aneurysm9]

Hi! Just wanted to ask if we're OK to merge this one, or if it's preferred to postpone it after the v1.0.0 milestone.

I'm good either way, will check periodically to keep it merge-ready.

We discussed in the last SIG meeting that we would like to minimize new feature additions in RCs, but I also don't want to keep PRs that are ready to go from landing. These detectors are going to land, whether that's in 1.0.0-RC# or 1.0.1 shouldn't matter too much. I'll poll the SIG at today's meeting and see if we have consensus to land it.

[jmacd]

🎉