OS and Process resource detectors

[nelsonghezzi]

Hi,

This PR adds detectors for most of the attributes under the OS and Process resources.

The only not implemented are:

• process.command
• process.command_line
• os.description

New functions are provided under the resource package:

• resource.WithOSType()
• resource.WithProcess()
• resource.WithProcessPID()
• resource.WithProcessExecutableName()
• resource.WithProcessExecutablePath()
• resource.WithProcessCommandArgs()
• resource.WithProcessOwner()
• resource.WithProcessRuntimeName()
• resource.WithProcessRuntimeVersion()
• resource.WithProcessRuntimeDescription()

Tests are TODO atm. Will add them if you think the PR it's a good fit for the package.

Some output examples:

Linux (container):

Darwin:

Windows:

[codecov]

Codecov Report

Merging #1788 (af8ec60) into main (7374d67) will increase coverage by 0.2%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##            main   #1788     +/-   ##
=======================================
+ Coverage   78.3%   78.5%   +0.2%     
=======================================
  Files        135     137      +2     
  Lines       7220    7288     +68     
=======================================
+ Hits        5656    5727     +71     
+ Misses      1318    1316      -2     
+ Partials     246     245      -1     

Impacted Files	Coverage Δ
sdk/resource/os.go	100.0% <100.0%> (ø)
sdk/resource/process.go	100.0% <100.0%> (ø)
sdk/trace/batch_span_processor.go	82.4% <0.0%> (ø)
exporters/trace/jaeger/jaeger.go	92.8% <0.0%> (+0.5%)	⬆️
sdk/resource/resource.go	67.8% <0.0%> (+3.5%)	⬆️

[Aneurysm9]

I think this seems like a reasonable way to add detection of these resources. Please add some tests and a CHANGELOG.md entry.

[nelsonghezzi]

@Aneurysm9 Done! Let me know if it's more or less what you had in mind.

[Aneurysm9]

I worry somewhat that the tests are just restatements of the logic being tested. Given the nature of these resource detectors though, I'm not sure there's a good way to mock up the environment for testing other than perhaps creating an interface and a struct with proxy methods hanging off of it so that you can mock that interface. Is that worth the effort here? Do we expect runtime to be a problem? v0v

[nelsonghezzi]

I had the same impression while writing the tests. I also thought about the interface/mock approach, as I have done it before to test functions with dependencies on rand, but ended up thinking it was not worth it in this case.

Additionally, there is a reason why I replicated the logic in the test file (the pid(), executableName() and similar functions).

I could have defined those functions in the process.go file and then exported them for reuse in the tests (the export_test.go trick). But doing so would only have reinforced the feeling of self-validating code.

Instead my reasoning was "I assume the os, runtime and user packages return the expected (and correct) information for these attributes, so this is the information that the WithProcess* functions should set on each of them", but avoiding direct reuse of the internal code implementing them.

Besides that (which was more of a conceptual relief I've to admit 😅), the tests mostly validates that each function sets the expected attribute name (with the exception of the runtime description test maybe, that also validates the string format).

I'm a bit concerned for the command args test, because is the most susceptible to what the test runner put on them. os.Args is writeable, but that would be a risky way to mock them, as it could affect subsequent tests if not restored I think. Anyway, the implementation as is now should not break if the test runner changes the arguments under the hood on each run.

I will search for other ways to mock the environment, but so far didn't find anything.

[nelsonghezzi]

I found this question, which gave me an idea for a possible lightweight form of indirection, allowing to mock the expected attribute values.

I've added 98a3c8d to further explore the idea.

But if you think the extra complexity doesn't pay for the benefits, I can revert to the previous version.

[Aneurysm9]

The approach of having local function variables that can be replaced in tests could work. I think I'd prefer to see those function variables only reference the os or runtime functions or variables and not contain formatting logic. The formatting logic should still live in the detector functions and thus be testable by altering inputs and asserting different outputs.

For instance, rather than have:

	executableName executableNameProvider = func() string { return filepath.Base(os.Args[0]) }
	commandArgs    commandArgsProvider    = func() []string { return os.Args }
...
// Detect returns a *Resource that describes the name of the process executable.
func (processExecutableNameDetector) Detect(ctx context.Context) (*Resource, error) {
	return NewWithAttributes(semconv.ProcessExecutableNameKey.String(executableName())), nil
}

I would have:

	commandArgs    commandArgsProvider    = func() []string { return os.Args }
...
// Detect returns a *Resource that describes the name of the process executable.
func (processExecutableNameDetector) Detect(ctx context.Context) (*Resource, error) {
	return NewWithAttributes(semconv.ProcessExecutableNameKey.String(filepath.Base(commandArgs()[0]))), nil
}

That way you can set up a table-based test for that detector that tests various inputs and ensures it produces the expected output:

/foo/bar/baz => baz
foo => foo
etc.

[nelsonghezzi]

That makes sense.

I've streamlined the initial implementation based on your feedback, let me know what you think.

These are the main changes:

• Splitted functions to set "providers" based on the package the wrapped functions/variables comes from (I think this could allow for more granular reuse in the OS detector tests).
• Added a way to restore "default" implementations for those functions (mostly for use on the tear down phase of the tests, I think it's safe if the tests don't leave this variables changed).
• Added tests to exercise the error code paths, as this is easy now.
• Added tests to verify the provider functions that wrap variables (this was mostly to not impact the coverage leaving those paths untested, but they don't add much value otherwise).

p.s.: I think the CI is failing randomly. Is there a way to manually trigger a new run?

[nelsonghezzi]

Hi! I'm keeping this PR up to date with main.

If you think the current approach ended up a bit twisted, maybe we can revert back to the more "naive" way of testing, and iterate the mocked approach in a separate PR?

I also have a cross platform implementation for the os.description attribute, but it will be better to submit it in a separate PR to keep this in a manageable size. Because it's partially based on the work made here, I'm holding it for now.

[MrAlias]

I think these detectors are a great idea, thanks for the contribution. I'm a bit worried about the tests being more change detectors than regression tests as was pointed out. I'm good with accepting them as is though and if we determine them to be truly deficient we can address then.

🥇

[nelsonghezzi]

Hi @MrAlias, thanks for the feedback! I think I solved all the points, but let me know otherwise (don't know why the CI stalled with the latests commits).

Yeah, I also feel that the tests could be expanded and/or improved somehow. Maybe as the codebase evolves (and some other resource detectors are added) a more common pattern emerges. At that point, count on me to revisit this implementation :)