feat: use constants for resource detectors keys

[rogercoll]

Changes

Use semantic conventions statics for Resource keys instead of raw values.

No changes on the code logic.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.3%. Comparing base (9f0ac7d) to head (8cd8635).

❗ Current head 8cd8635 differs from pull request most recent head bb7ad51. Consider uploading reports for the commit bb7ad51 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #1610     +/-   ##
=======================================
- Coverage   68.3%   67.3%   -1.0%     
=======================================
  Files        139     138      -1     
  Lines      19806   19656    -150     
=======================================
- Hits       13530   13245    -285     
- Misses      6276    6411    +135     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lalitb]

Thanks

[cijothomas]

We need to plan this further before accepting.

SemanticConventions for most of the Resource is experimental : https://github.com/open-telemetry/semantic-conventions/blob/main/docs/resource/os.md#operating-system

So the semantic conventions crate would also likely remain experimental (or we split it to multiple or feature flags). Having SDK take a dependency on that can impact SDK's own stable release.

[lalitb]

I think using constants from the semantic conventions crate instead of hardcoding strings is a safe bet. Any updates to these constants (eg deprecated/removed) shouldn't impact our SDK's API. Pretty much every other language is doing it without issues.

[rogercoll]

Good point, I see that other SDK's use the semconv statics as well, but at the same time, their API version is aligned with the semconv version too.

The Go SDK is stable, but using the OS experimental SemanticConventions. The same happens in the Python SKD. While most semantic conventions attributes are not stable, their deprecation won't affect the current API.

[TommyCpp]

Since it's only 7 strings I think we can just make them crate public constant inside SDK until semconv stabled

[lalitb]

Makes sense to have public constants within SDK as an interim solution if we don't want to have dependency on semconv till it is stable.

[hdost]

I think using constants from the semantic conventions crate instead of hardcoding strings is a safe bet. Any updates to these constants (eg deprecated/removed) shouldn't impact our SDK's API. Pretty much every other language is doing it without issues.

I'd agree, it's mainly from the perspective of API functions/type. It shouldn't affect is there.

[hdost]

I think using constants from the semantic conventions crate instead of hardcoding strings is a safe bet. Any updates to these constants (eg deprecated/removed) shouldn't impact our SDK's API. Pretty much every other language is doing it without issues.

I'd agree, it's mainly from the perspective of API functions/type. It shouldn't affect is there.

We could look at moving the resourcedectors out into the SemConv crate.

[rogercoll]

Since it's only 7 strings I think we can just make them crate public constant inside SDK until semconv stabled

Sounds good to me, I will port the resource.rs file from the SemConv crate with only the current used values.

We could look at moving the resourcedectors out into the SemConv crate.

Although, I would keep the SemConv crate free of code's logic and just use it for static data, it could really fit with a resource detection feature flag (to avoid unneeded dependencies, etc.).

[cijothomas]

I think using constants from the semantic conventions crate instead of hardcoding strings is a safe bet. Any updates to these constants (eg deprecated/removed) shouldn't impact our SDK's API. Pretty much every other language is doing it without issues.

If semantic conventions changes, user was relying on SDK to provide resource detectors, it is breaking the behavior. Agree it is not the SDK's APIs breaking, but behavior breaking.

Pretty much every other language is doing it without issues.

I doubt that. The "without issues." part! When semantic convention makes a breaking change, it'll be affecting SDK users. The conventions may not have changed that much, but we shouldn't rely on that.

SDK crate should not be in the job of providing resource detectors, other than the ones required by spec which is only telemetry and service (both are stable already. Everything else should be moved out of sdk to own crates.

[cijothomas]

Although, I would keep the SemConv crate free of code's logic and just use it for static data

+1.

[cijothomas]

@rogercoll
Will discuss this in today's community call and update how to proceed, since there seems to diff. opinions.
Feel free to join, if you'd like, but otherwise, I'll share the decision after the meeting today (~starting in 45 mins)

[cijothomas]

From Mar 12 2024, Community discussion:

Cijo - pub(crate) constants, Remove all non-spec mandated ResourceDetectors from SDK, and make a new -contrib crate for that
Zhongyang - pub(crate) constants, NOT required to remove ResourceDetectors for Env/OS/Process etc. from SDK at the moment.
Lalit - pub(crate) constants, NOT required to remove ResourceDetectors for Env/OS/Process etc. from SDK at the moment.
Harold - pub(crate) constants, okay to move ResourceDetectors to contrib repo, and keep SDK clean.
Roger - pub(crate) constants,
Python, Golang does keep detectors in SDK -
C++, .NET does not do this.

Roger to create a new crate in contrib repo and move all detectors from SDK (the non-spec mandated ones - telemetry, service) to that new crate. Make it easy for users to discover the new home.

https://github.com/open-telemetry/semantic-conventions/blob/main/docs/resource/README.md#semantic-attributes-with-sdk-provided-default-value - the ones we need to keep in sdk. (4 constants)

[rogercoll]

In addition to the telemetry and service ones, I think we should keep the environment one too: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/sdk.md#specifying-resource-information-via-an-environment-variable

The SDK MUST extract information from the OTEL_RESOURCE_ATTRIBUTES environment variable and merge this, as the secondary resource, with any resource information provided by the user, i.e. the user provided resource information has higher priority.

[cijothomas]

In addition to the telemetry and service ones, I think we should keep the environment one too: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/sdk.md#specifying-resource-information-via-an-environment-variable

The SDK MUST extract information from the OTEL_RESOURCE_ATTRIBUTES environment variable and merge this, as the secondary resource, with any resource information provided by the user, i.e. the user provided resource information has higher priority.

yes. it has no semantic convention dependency.

[rogercoll]

PR regarding the migration of the OS and process detectors to the contrib crate: open-telemetry/opentelemetry-rust-contrib#49

Your feedback will be greatly appreciated. Should we proceed with the sdk removal once the new crate is available in the contrib?

[TommyCpp]

Given the open-telemetry/opentelemetry-rust-contrib#49 we can remove the resource detector and close this PR?

[rogercoll]

@TommyCpp I have just opened a new PR with the agreed changes: #1610

Closing this one.

[TommyCpp]

@TommyCpp I have just opened a new PR with the agreed changes: #1610

Closing this one.

Sorry I realize I may be misleading on the previous comment. I meant we can remove the resrouce detector and this PR should be good to merge. Can we reopen this one? If you prefer we can also review and merge the new one.