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-trace-base): move Sampler declaration into sdk-trace-base #3088

Merged
merged 4 commits into from
Jul 29, 2022

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Jul 12, 2022

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

codecov bot commented Jul 12, 2022

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

@legendecas legendecas marked this pull request as ready for review July 12, 2022 03:03
@legendecas legendecas requested a review from a team July 12, 2022 03:03
@legendecas legendecas force-pushed the sampler branch 2 times, most recently from 413abd8 to f391cc0 Compare July 12, 2022 03:29
@legendecas legendecas force-pushed the sampler branch 2 times, most recently from 5499f4c to 08e6c82 Compare July 12, 2022 03:39
@Flarna
Copy link
Member

Flarna commented Jul 12, 2022

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

@legendecas
Copy link
Member Author

legendecas commented Jul 12, 2022

@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
Copy link
Member

dyladan commented Jul 12, 2022

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

Flarna commented Jul 12, 2022

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

@dyladan
Copy link
Member

dyladan commented Jul 12, 2022

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

Flarna commented Jul 12, 2022

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

dyladan commented Jul 12, 2022

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

dyladan commented Jul 20, 2022

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

Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

lgtm

@legendecas legendecas merged commit df58fac into open-telemetry:main Jul 29, 2022
@legendecas legendecas deleted the sampler branch July 29, 2022 03:08
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.

Move Sampler interface to an SDK package?
4 participants