feat(sdk-trace-base)!: remove addSpanProcessor API

[david-luna]

Which problem is this PR solving?

After making the activeSpanProcessor private there was a discussion in #4792 about also dropping addSpanProcessor from the BasicTraceProvider class. Allowing users to add more processors at runtime is (IMHO):

• a use case difficult to justify
• a risk of getting issues that are hard to spot

Ref #4792

Short description of the changes

• Removed addSpanProcessor method from BasicTraceProvider
• Add a new TracerConfig option to add Span processors in the constructor
• Update NodeTracerProvider class from @opentelemetry/sdk-node
• Remove TracerProviderWithEnvExporters class from @opentelemetry/sdk-node
• Update all tests

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 at root level
• npm run test at root level

NOTE; The tests for the node SDK already covered the scenarios where exporters have been configured through environment vars. So there is no need to add extra tests from the removed TracerProviderWithEnvExporters class

Checklist:

• Followed the style guidelines of this project

[codecov]

Codecov Report

Attention: Patch coverage is 91.35802% with 7 lines in your changes missing coverage. Please review.

Project coverage is 93.18%. Comparing base (b057c93) to head (d382b26).
Report is 3 commits behind head on next.

Files with missing lines	Patch %	Lines
...ental/packages/opentelemetry-sdk-node/src/utils.ts	88.70%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #5134      +/-   ##
==========================================
- Coverage   93.24%   93.18%   -0.06%     
==========================================
  Files         318      317       -1     
  Lines        8231     8205      -26     
  Branches     1651     1650       -1     
==========================================
- Hits         7675     7646      -29     
- Misses        556      559       +3     

Files with missing lines	Coverage Δ
...imental/packages/opentelemetry-sdk-node/src/sdk.ts	97.35% <100.00%> (+1.86%)	⬆️
...elemetry-sdk-trace-base/src/BasicTracerProvider.ts	96.33% <100.00%> (+0.79%)	⬆️
...ental/packages/opentelemetry-sdk-node/src/utils.ts	87.95% <88.70%> (+2.23%)	⬆️

... and 1 file with indirect coverage changes

[pichlermarc]

Looks good already, thanks for working on this 🙌

One question: do you think it would be a lot of work to backport this as a feature to the main branch? 🤔 We did a similar change recently with MeterProvider#addMetricReader() - see #4427, #4419

Doing so could

• be helpful for users to start to transition to the "new" way of doing things
• be helpful for us to merge changes from main to next while we still have to do it

[pichlermarc]

I think we should not register if there's no SpanProcessor- having it there and working through spans instead of it being a no-op introduces additional overhead for the user - so your current implementation is fine.

What we should consider, though is registering a ContextManager and Propagator (not in this PR, but later) if no SpanProcessor is registered but either a logs SDK or metrics SDK is. Having a ContextManager and Propagator can be useful in these cases (having a TraceID for logs when there's an active trace from another service but no trace SDK in this service can be very helpful and is something that users have asked for previously)

[david-luna]

Okay, I'll create an issue for that

[pichlermarc]

would it be possible to re-purpose some of the tests from this file?
I see that the new utils have some lines that are not covered by tests. 🤔

[david-luna]

I'll do another review and put the selected tests into the sdk tests

[david-luna]

Looks good already, thanks for working on this 🙌

One question: do you think it would be a lot of work to backport this as a feature to the main branch? 🤔 We did a similar change recently with MeterProvider#addMetricReader() - see #4427, #4419

Doing so could

• be helpful for users to start to transition to the "new" way of doing things
• be helpful for us to merge changes from main to next while we still have to do it

I guess what you want would be to have the deprecation of the API plus the constructor option. It's not too much work and also I think you 1st point is a very good one :)

[pichlermarc]

Looks good already, thanks for working on this 🙌
One question: do you think it would be a lot of work to backport this as a feature to the main branch? 🤔 We did a similar change recently with MeterProvider#addMetricReader() - see #4427, #4419
Doing so could

• be helpful for users to start to transition to the "new" way of doing things
• be helpful for us to merge changes from main to next while we still have to do it

I guess what you want would be to have the deprecation of the API plus the constructor option. It's not too much work and also I think you 1st point is a very good one :)

Exactly. Ideally we'd also have the changes to the test files across the packages and @opentelemetry/sdk-node to reduce merge conflicts (since the constructor option is there then we can also update tests), but these are secondary. 🙂

Another benefit of moving the other changes over is that SDK 2.0 will be less of a big bang release and if there are any problems we can get aware them earlier.

[david-luna]

@pichlermarc I've created #5138 targeting main. I think it's better to:

• review and merge that PR into main 1st
• update next with changes in main
• update this PR accordingly

Thanks for your time :)

[pichlermarc]

@pichlermarc I've created #5138 targeting main. I think it's better to:
..

Let's do that - thanks for taking the time to backport the feature-part of this to 1.x 🙂

[david-luna]

Will open a new PR once main gets merged into next branch.