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 3 commits 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

All modified and coverable lines are covered by tests ✅

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
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

Copy link

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
Copy link
Contributor

trentm commented Aug 22, 2024

@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 pichlermarc force-pushed the feat/default-resource-merge-opt-out branch from 1eb145d to 9e44234 Compare September 24, 2024 16:30
@pichlermarc
Copy link
Member Author

@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.

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

Successfully merging this pull request may close these issues.

2 participants