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

feat(sdk-*)!: align merge resource behavior with spec #5219

Conversation

david-luna
Copy link
Contributor

Which problem is this PR solving?

Aligns SDKs resource merging strategy with the specification:

Fixes #5132

Short description of the changes

  • remove config mergeResourcesWithDefault option
  • do not merge default resource if one provided in config
  • update tests

Type of change

Please delete options that are not relevant.

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Compiled and ran the tests in all packages.

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added

@david-luna david-luna requested a review from a team as a code owner November 29, 2024 10:17
Copy link

codecov bot commented Nov 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.62%. Comparing base (8dc74e6) to head (229a559).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5219      +/-   ##
==========================================
- Coverage   94.63%   94.62%   -0.01%     
==========================================
  Files         323      323              
  Lines        8083     8068      -15     
  Branches     1643     1637       -6     
==========================================
- Hits         7649     7634      -15     
  Misses        434      434              
Files with missing lines Coverage Δ
...imental/packages/opentelemetry-sdk-node/src/sdk.ts 96.93% <100.00%> (-0.04%) ⬇️
...perimental/packages/sdk-logs/src/LoggerProvider.ts 97.77% <100.00%> (-0.23%) ⬇️
experimental/packages/sdk-logs/src/config.ts 100.00% <ø> (ø)
...elemetry-sdk-trace-base/src/BasicTracerProvider.ts 96.19% <100.00%> (-0.08%) ⬇️
...ackages/opentelemetry-sdk-trace-base/src/config.ts 88.37% <ø> (ø)
packages/sdk-metrics/src/MeterProvider.ts 100.00% <100.00%> (ø)

@david-luna david-luna changed the title refactor(sdk-*): aling merge resource behavior with spec refactor(sdk-*): align merge resource behavior with spec Nov 29, 2024
CHANGELOG_NEXT.md Outdated Show resolved Hide resolved
Co-authored-by: Marc Pichler <[email protected]>
@david-luna david-luna changed the title refactor(sdk-*): align merge resource behavior with spec feat(sdk-*): align merge resource behavior with spec Dec 2, 2024
@david-luna david-luna changed the title feat(sdk-*): align merge resource behavior with spec feat(sdk-*)!: align merge resource behavior with spec Dec 2, 2024
@pichlermarc pichlermarc added this to the OpenTelemetry SDK 2.0 milestone Dec 4, 2024
@pichlermarc pichlermarc added the target:next-major-release This PR targets the next major release (`next` branch) label Dec 4, 2024
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for working on this 🙌
Just one question about the NodeSDK implementation of this.

experimental/packages/opentelemetry-sdk-node/src/sdk.ts Outdated Show resolved Hide resolved
CHANGELOG_NEXT.md Outdated Show resolved Hide resolved
@pichlermarc
Copy link
Member

just needs to be re-targeted to main and then we can merge :)

@david-luna david-luna changed the base branch from next to main December 20, 2024 12:20
@david-luna
Copy link
Contributor Author

just needs to be re-targeted to main and then we can merge :)

done!

@pichlermarc pichlermarc added this pull request to the merge queue Dec 20, 2024
Merged via the queue into open-telemetry:main with commit 90afa28 Dec 20, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
target:next-major-release This PR targets the next major release (`next` branch)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[sdk-*] remove mergeResourceWithDefaults option and align resource merging behavior with spec
2 participants