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

refactor(sdk-trace-base): make resource private and remove getActiveSpanProcessor API #5192

Conversation

david-luna
Copy link
Contributor

Which problem is this PR solving?

Resource and active span processor should not be accessible once the tracer provider is constructed. This PR makes the necessary changes to have both as private properties and inject them into any tracer created via getTracer API.

Fixes #4791
Fixes #4792

Short description of the changes

  • made resource private in BasicTracerProvider
  • remove getActiveSpanProcessor in BasicTracerProvider
  • refactor SpanImpl constructor
  • update Tracer.startSpan to use the new constructor
  • refactor Tracer constructor to get only necessary deps (processor & resource) instead of the provider
  • update tests

Changes in SpanImpl constructor

I've decided to change the constructor method signature for SpanImpl. This way we inject the necessary dependencies that are held in the Tracer class making unnecessary to have public properties or getters for span processor and resource. The class was made private (not exported) in #5048

Type of change

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

How Has This Been Tested?

  • npm run compile && npm run test

Checklist:

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

@david-luna david-luna requested a review from a team as a code owner November 22, 2024 12:07
Copy link

codecov bot commented Nov 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.13%. Comparing base (4a394cc) to head (ddc574b).
Report is 4 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #5192      +/-   ##
==========================================
- Coverage   93.15%   93.13%   -0.03%     
==========================================
  Files         315      315              
  Lines        8113     8102      -11     
  Branches     1633     1633              
==========================================
- Hits         7558     7546      -12     
- Misses        555      556       +1     
Files with missing lines Coverage Δ
...elemetry-sdk-trace-base/src/BasicTracerProvider.ts 96.26% <100.00%> (+0.57%) ⬆️
packages/opentelemetry-sdk-trace-base/src/Span.ts 97.60% <100.00%> (-0.02%) ⬇️
...ackages/opentelemetry-sdk-trace-base/src/Tracer.ts 98.55% <100.00%> (-0.03%) ⬇️

... and 1 file with indirect coverage changes

---- 🚨 Try these New Features:

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 👍

@pichlermarc pichlermarc merged commit c28abac into open-telemetry:next Nov 25, 2024
18 checks passed
@david-luna david-luna deleted the 4791-resource-private-basic-provider branch November 25, 2024 16:14
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.

2 participants