Add an option to add host resources automatically - #68
Conversation
Added unconditionally: SDK name, version & language Added conditionally with 3 selectable categories: - host: arch - os: os type - process: runtime name & version
inge4pres
left a comment
inge4pres
left a comment
There was a problem hiding this comment.
Nice work, thanks for the proposal Antoine 🙏🏼
Left a couple of comments
| const os_type: []const u8 = switch (builtin.os.tag) { | ||
| .macos, .ios, .tvos, .watchos => "darwin", | ||
| .dragonfly => "dragonflybsd", | ||
| .illumos => "solaris", |
There was a problem hiding this comment.
I thought they had removed solaris support from 0.17.0-dev onwards, due to how difficult it was to maintain a valid linker for it?
Definitely remember the discussion, but if it's gone after 0.16.0, compiler will tell 😄
There was a problem hiding this comment.
Yes they removed solaris, which is not open-source, but kept illumos, which is open-source.
Quote from 0.16 release notes:
Support for Oracle's Solaris and IBM's AIX and z/OS has been removed. In general, the Zig project cannot support proprietary operating systems that make it unreasonably difficult to obtain system headers and thus audit contributions. Note that this does not affect illumos; being an open source fork from OpenSolaris, it remains supported.
| const host_arch: []const u8 = switch (builtin.cpu.arch) { | ||
| .x86_64 => "amd64", | ||
| .aarch64 => "arm64", | ||
| .arm => "arm32", | ||
| .powerpc => "ppc32", | ||
| .powerpc64, .powerpc64le => "ppc64", | ||
| else => @tagName(builtin.cpu.arch), | ||
| }; |
There was a problem hiding this comment.
Why even bothering converting these and not awlasy use@tagName(builtin.cpu.arch)?
Arewe trying to mirror the Go/Rust SDK names or adhere to a specific convention?
There was a problem hiding this comment.
OTel semantic convention specs:
host.arch has the following list of well-known values. If one of them applies, then the respective value MUST be used; otherwise, a custom value MAY be used.
| fn parseResourceDetectors(env_map: *const EnvMap) ResourceDetectors { | ||
| const value = env_map.get("OTEL_EXPERIMENTAL_RESOURCE_DETECTORS") orelse return .{}; | ||
| var detectors: ResourceDetectors = .{}; | ||
| var iter = std.mem.splitScalar(u8, value, ','); | ||
| while (iter.next()) |item| { | ||
| const name = std.mem.trim(u8, item, &std.ascii.whitespace); | ||
| if (std.ascii.eqlIgnoreCase(name, "host")) detectors.host = true; | ||
| if (std.ascii.eqlIgnoreCase(name, "os")) detectors.os = true; | ||
| if (std.ascii.eqlIgnoreCase(name, "process")) detectors.process = true; | ||
| } | ||
| return detectors; | ||
| } |
There was a problem hiding this comment.
nit: can we have a unit test for this?
Or actually, since we're parsing the key/value form also for other env vars (see #78), can we factor out a common function that makes the parsing and returns a slice of tuple { key: []const u8, val: []const u8 }?
There was a problem hiding this comment.
Agreed, let's make an iterator for this
|
|
||
| // Global settings | ||
| sdk_disabled: bool, | ||
| resource_detectors: ResourceDetectors, |
There was a problem hiding this comment.
What do you think of hvaing this field as optional, and initialized to null?
It would make all changes to constructors in resource.zig unneeded, and would not break compilation for existing users of the Configuration struct.
I feel like the fact that the feature is experimental supports the suggestion, let me know.
|
I realized that these should be enabled by default:
So I will make these opt-out rather than opt-in. |
Add a flag to control the inclusion of the SDK-related attributes the env var parsing now handles: - "" to mean default, - "*" to mean all, - "none" to disable all Assisted-by: Opus 5
2 participants
Added unconditionally: SDK name, version & language
Added conditionally with 3 selectable categories:
For now only comptime-known values are added, the remaining work for runtime detection is tracked in #17
(Moved from zig-o11y/opentelemetry-sdk#159)