[prototype] feat(sdk-trace): add option to opt-out from merging the resource with Resource.default()

[pichlermarc]

trafficstars

The spec states that

The SDK MUST provide access to a Resource with at least the attributes listed at Semantic Attributes with SDK-provided Default Value. This resource MUST be associated with a TracerProvider or MeterProvider if another resource was not explicitly specified.

However, we merge with Resource.default() regardless of if another resource was explicitly specified or not.

This leaves us with a difficult choice:

• stop merging with Resource.default() - this would likely break telemetry for users that rely on our non spec-compliant behavior of merging all the time, resource attributes that used to be there, would not be there anymore. (I'm not in favor of this)
• apply something akin to what's being proposed here, where users can explicitly turn this behavior off. It's not optimal as this will still leave us spec non-compliant, but slightly less so. Users will be able to choose. Downstream in NodeSDK we can make not merging it the default behavior, or choose to also expose this to the users (possibly even via an environment variable OTEL_NODE_MERGE_DEFAULT_RESOURCE or similar).
• do something else (still discussing to come up with ideas)

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 93.93%. Comparing base (f8ab559) to head (9e44234). Report is 17 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4617      +/-   ##
==========================================
+ Coverage   93.39%   93.93%   +0.53%     
==========================================
  Files          46      310     +264     
  Lines         712     8150    +7438     
  Branches      120     1639    +1519     
==========================================
+ Hits          665     7656    +6991     
- Misses         47      494     +447     

Files with missing lines	Coverage Δ
...imental/packages/opentelemetry-sdk-node/src/sdk.ts	95.54% <100.00%> (ø)
...perimental/packages/sdk-logs/src/LoggerProvider.ts	98.00% <100.00%> (ø)
experimental/packages/sdk-logs/src/config.ts	100.00% <ø> (ø)
...elemetry-sdk-trace-base/src/BasicTracerProvider.ts	95.57% <100.00%> (ø)
...ackages/opentelemetry-sdk-trace-base/src/config.ts	88.63% <ø> (ø)
packages/sdk-metrics/src/MeterProvider.ts	100.00% <100.00%> (ø)

... and 260 files with indirect coverage changes

[trentm]

Some notes, thinking out loud.

How to create NodeSDK without getting Resource.default() merged in?

// - Option 1: If we had to do it all over again it would be: If you explicitly
//   pass in a `resource` then `.default()` is NOT used.
//   Cons: Breaks backward compat. NodeSDK *is* pre-1.0, so we *could*, but
//   probably don't want to until a major version bump.
const sdk = new NodeSDK({ resource: new Resource({foo: 'bar'}) }); // no defaults
const sdk = new NodeSDK({ resource: Resource.empty() }); // no defaults
const sdk = new NodeSDK({ resource: Resource.default() }); // yes defaults
const sdk = new NodeSDK(); // yes defaults

// - Option 2: Boolean option to say not to merge defaults.
//   Defaults to always merge with Resource.default() for now.
//
//   Could consider later (for SDK 2.0?) changing `mergeResourceWithDefaults`
//   to a tri-state: true, false, or unspecified. Unspecified  means it
//   depends on whether `resource` is given. Eventually then the behaviour
//   would be as in Option 1, with the option to explicitly keep it stable
//   by specifying `mergeResourceWithDefaults`.
const sdk = new NodeSDK({ mergeResourceWithDefaults: false }); // no defaults

[github-actions[bot]]

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[trentm]

@pichlermarc Interested in adding a doc section for this in the sdk-node/README.md and then taking this out of draft?

See discussion at #4930

[pichlermarc]

@trentm sorry for letting this sit for so long. I'm picking this up again. I don't have good answers to these questions yet but I'll comment here and will take it out of draft once I figured out what's best for NodeSDK.

When we merge this I'll also follow up on the next branch to restore a spec-compliant behavior. I think in 2.0 we can then have a Default Resource detector in NodeSDK that would add the default resource and can be turned off via env var just like the other detectors.

Maybe for now we just allow the same flag as proposed in the NodeSDK for this PR.