Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pichlermarc
Copy link
Member

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)

Copy link

codecov bot commented Apr 9, 2024

Codecov Report

Merging #4617 (1eb145d) into main (c046867) will increase coverage by 0.50%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4617      +/-   ##
==========================================
+ Coverage   92.83%   93.33%   +0.50%     
==========================================
  Files         329      292      -37     
  Lines        9528     8357    -1171     
  Branches     2053     1755     -298     
==========================================
- Hits         8845     7800    -1045     
+ Misses        683      557     -126     
Files Coverage Δ
...elemetry-sdk-trace-base/src/BasicTracerProvider.ts 95.57% <100.00%> (-0.08%) ⬇️
...ackages/opentelemetry-sdk-trace-base/src/config.ts 88.37% <ø> (ø)

... and 65 files with indirect coverage changes

@trentm
Copy link
Contributor

trentm commented Apr 11, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants