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

sampler and span processor prototype implementations for consistent sampling #226

Merged
merged 54 commits into from
Apr 20, 2022

Conversation

oertl
Copy link
Contributor

@oertl oertl commented Feb 7, 2022

Description:

This PR adds various Sampler and SpanProcessor prototype implementations for consistent sampling as defined by https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/tracestate-probability-sampling.md and open-telemetry/opentelemetry-specification#2047:

EDIT: ConsistentReservoirSamplingBatchSpanProcessor was removed to make this PR smaller as requested by @anuraaga. It can now be found in https://github.com/dynatrace-oss-contrib/opentelemetry-java-contrib/tree/consistent-reservoir-sampling.

@oertl oertl requested a review from a team February 7, 2022 09:33
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Did a very quick skim - 3700 lines is too huge though, let's start with a PR that only adds ConsistentProbabilityBasedSampler please

oertl and others added 4 commits April 9, 2022 09:11
…te/OtelTraceState.java

Co-authored-by: Joshua MacDonald <jmacd@users.noreply.github.com>
…te/OtelTraceState.java

Co-authored-by: Joshua MacDonald <jmacd@users.noreply.github.com>
…te/OtelTraceState.java

Co-authored-by: Joshua MacDonald <jmacd@users.noreply.github.com>
…te/OtelTraceState.java

Co-authored-by: Joshua MacDonald <jmacd@users.noreply.github.com>
@oertl
Copy link
Contributor Author

oertl commented Apr 19, 2022

@trask, thank you very much for your valuable suggestions! I hope I have not overlooked anything and have addressed all your points.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

@open-telemetry/java-contrib-approvers let me know if anyone is planning to review this further, otherwise I will merge before making the upcoming release, thx!

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

It looks like RandomGenerator can be moved into the samplers package and made package private? We can have package-private factories accepting it for use in unit tests only in this repo, but let's keep the public API using just a default threadlocalrandom based generator without user configuration.

For OtelTraceState it also looks like it can be made package private. If you expect other packages needing the utility methods, then it's also ok to move into io.opentelemetry.contrib.samplers.internal but suspect we don't need to do more than package private at least at this time.

Note that we cannot have packages present in multiple artifacts in our codebase. So currently the generic sounding packages io.opentelemetry.contrib.state and io.opentelemetry.contrib.util could cause problems in the future. The above internalizing suggestions will hopefully fix that naturally.

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Thanks!

@oertl
Copy link
Contributor Author

oertl commented Apr 20, 2022

@anuraaga, thanks for your suggestions. I addressed them in da42018

@trask trask merged commit 0b9ab8a into open-telemetry:main Apr 20, 2022
@trask
Copy link
Member

trask commented Apr 20, 2022

thanks @oertl 🎉

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.

6 participants