feat(sdk-trace-base): move Sampler declaration into sdk-trace-base

[legendecas]

Which problem is this PR solving?

Fixes #2961

Duplicated the declaration and definition of Sampler and IdGenerator in @opentelemetry/sdk-trace-base and deprecated the ones defined in @opentelemetry/core.

Type of change

• New feature (non-breaking change which adds functionality)

This is not breaking.

How Has This Been Tested?

• Samplers and IdGenerators are tested in the @opentelemetry/sdk-trace-base package.

Checklist:

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

[codecov]

Codecov Report

Merging #3088 (b3cfb38) into main (3db1056) will decrease coverage by 0.97%.
The diff coverage is 93.42%.

❗ Current head b3cfb38 differs from pull request most recent head cf68986. Consider uploading reports for the commit cf68986 to get more accurate results

@@            Coverage Diff             @@
##             main    #3088      +/-   ##
==========================================
- Coverage   93.08%   92.11%   -0.98%     
==========================================
  Files         189       87     -102     
  Lines        6293     2499    -3794     
  Branches     1329      548     -781     
==========================================
- Hits         5858     2302    -3556     
+ Misses        435      197     -238     

Impacted Files	Coverage Δ
...lemetry-core/src/trace/sampler/AlwaysOffSampler.ts	100.00% <ø> (ø)
...elemetry-core/src/trace/sampler/AlwaysOnSampler.ts	100.00% <ø> (ø)
...metry-core/src/trace/sampler/ParentBasedSampler.ts	83.87% <ø> (ø)
...core/src/trace/sampler/TraceIdRatioBasedSampler.ts	100.00% <ø> (ø)
...ckages/opentelemetry-sdk-trace-base/src/utility.ts	100.00% <ø> (ø)
...y-sdk-trace-base/src/sampler/ParentBasedSampler.ts	83.87% <83.87%> (ø)
...ckages/opentelemetry-sdk-trace-base/src/Sampler.ts	100.00% <100.00%> (ø)
...ackages/opentelemetry-sdk-trace-base/src/Tracer.ts	98.46% <100.00%> (+0.02%)	⬆️
...ackages/opentelemetry-sdk-trace-base/src/config.ts	88.37% <100.00%> (+1.19%)	⬆️
...try-sdk-trace-base/src/sampler/AlwaysOffSampler.ts	100.00% <100.00%> (ø)
... and 117 more

[Flarna]

Should we adapt the samplers here to use the type from SDK? Or is this planned for a followup PR?

[legendecas]

@Flarna we cannot, or we'll get a circular dependency between @opentelemetry/core and @opentelemetry/sdk-trace-base. I believe the exported Samplers in @opentelemetry/core can not be removed until v2.0.0. Before which, we can not change that.

[dyladan]

there isn't much left in the core package. we could copy the samplers and deprecate the package entirely. I think all thats left is some transformation code and clock code.

[Flarna]

There are W3CTraceContextPropagator and W3CBaggagePropagator. Not sure if they should be move to SDK or API or a complete separate package

[dyladan]

There are W3CTraceContextPropagator and W3CBaggagePropagator. Not sure if they should be move to SDK or API or a complete separate package

Most of the other SIGs actually put those propagators in the API, but they don't have need of a context manager so their API can be used standalone for propagation. We still at least need a context manager. We could discuss adding a default context manager to the API but then we're talking about much more substantial changes.

[Flarna]

We could discuss adding a default context manager to the API but then we're talking about much more substantial changes.

I think Java has a context manager in API but that's not really comparable because we have the need of at least 3 (zone.js, AsyncLocalStorage, AsyncHooks)....

[dyladan]

We could discuss adding a default context manager to the API but then we're talking about much more substantial changes.

I think Java has a context manager in API but that's not really comparable because we have the need of at least 3 (zone.js, AsyncLocalStorage, AsyncHooks)....

That's what I meant. Other languages have a single obvious context implementation where we need to support many. It would be possible to move the ALS/hooks one to the API and use it when it is available, but in web we would need to fall back to either noop or stack because we can't depend on zone.js

[dyladan]

@Flarna we cannot, or we'll get a circular dependency between @opentelemetry/core and @opentelemetry/sdk-trace-base. I believe the exported Samplers in @opentelemetry/core can not be removed until v2.0.0. Before which, we can not change that.

Might be worth @deprecateing them and moving the samplers to trace-base since it is a more natural place for them. Personally I would not be against deprecating the whole core package since it seems to just be a collection of things we didn't know where to put at the time.

[vmarchaud]

lgtm