Sampling API #52

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.

Sign up for GitHub

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

Jump to bottom

Merged

mwear merged 17 commits into open-telemetry:master from mwear:sampling_api

Aug 26, 2019

Member

mwear commented Aug 14, 2019

This PR implements the sampling API and addresses #38.

There are ongoing discussions around sampling in the specification repo, so I would not be surprised if changes come in this area. The API is implemented per the current spec for now.

mwear requested review from bai, fbogsany, luvtechno and SergeyKanzhelev as code owners

August 14, 2019 00:20

fbogsany reviewed

View reviewed changes

api/lib/opentelemetry/trace/samplers/always_sample_sampler.rb Outdated Show resolved Hide resolved

fbogsany reviewed

View reviewed changes

api/lib/opentelemetry/trace/samplers/always_sample_sampler.rb Outdated Show resolved Hide resolved

fbogsany reviewed

View reviewed changes

api/lib/opentelemetry/trace/samplers/basic_decision.rb Outdated Show resolved Hide resolved

fbogsany reviewed

View reviewed changes

api/lib/opentelemetry/trace/samplers/decision.rb Outdated Show resolved Hide resolved

fbogsany reviewed

View reviewed changes

api/lib/opentelemetry/trace/samplers/never_sample_sampler.rb Outdated Show resolved Hide resolved

fbogsany reviewed

View reviewed changes

api/lib/opentelemetry/trace/samplers/always_sample_sampler.rb Show resolved Hide resolved

Member Author

mwear commented Aug 15, 2019

@fbogsany: I agree with all of these comments and really had the same questions as I was implementing this. There is ambiguity in the spec and the Java implementation as to what arguments are required for should_sample and whether or not Decision#attributes is immutable. Both of those have resulting impacts on the implementation.

I will open an issue on the Spec repo to get some clarification. Once we can definitively answer some of these questions, cleaning this will be pretty straightforward.

Member Author

mwear commented Aug 15, 2019

I opened a spec issue here: open-telemetry/opentelemetry-specification#218. I'll start making some changes here based on the feedback, and speculation that the proposed clarifications can be made.

Member Author

mwear commented Aug 16, 2019

This is hot off the presses: open-telemetry/oteps#24, so I'm not sure what that means for this work for now.

Member Author

mwear commented Aug 19, 2019

Does it make sense to try to get this in a mergeable state and make an issue to update the sampling API based on the proposed RFC once it has had sufficient discussion and approval?

fbogsany reviewed

View reviewed changes

api/lib/opentelemetry/trace/samplers/decision.rb Show resolved Hide resolved

fbogsany reviewed

View reviewed changes

api/lib/opentelemetry/trace/samplers/sampler.rb Outdated Show resolved Hide resolved

fbogsany reviewed

View reviewed changes

api/test/opentelemetry/trace/samplers/decision_test.rb Show resolved Hide resolved

mwear added 13 commits

August 26, 2019 10:50


          Add BasicDecision class

This is a utility class that will be used by the AlwaysSampleSampler
and NeverSampleSampler classes.


          Add Decision class

Similar to Decision, but allows user specified attributes.


          Add AlwaysSampleSampler

c83887c


          Add NeverSampleSampler

7d26de7


          Document the Samplers module

b8d8e05


          Doc fixes

1db640b


          Expect links to be a possibly nil Enumerable of Link objects

3f7a778


          Add Sampler abstract class with type checking

f6f873d


          Subclass Sampler and check arguments for Always/NeverSampleSampler

79d0dce


          Treat Decision#attributes as immutable; remove BasicDecision

4e6f2e3


          Be more consistent with parentheses usage

422096e


          Ensure attributes is a hash when initialized with nil

9ea9858


          Document that span_context, extracted_context can be nil for a root span

0692b6f

mwear added 2 commits

August 26, 2019 10:50


          Inline check_arguments to Sampler#should_sample

9267d0a


          Rename Sampler#should_sample -> Sampler#decision

0c645af

mwear force-pushed the sampling_api branch from 187bdc9 to 8b07742 Compare

August 26, 2019 17:51

mwear added 2 commits

August 26, 2019 10:55


          Test Decision#attributes are frozen

3f8aabb


          Documentation fixes

a500e84

mwear force-pushed the sampling_api branch from 8b07742 to a500e84 Compare

August 26, 2019 17:55

fbogsany approved these changes

View reviewed changes

Member Author

mwear commented Aug 26, 2019

Fixes #38

mwear merged commit 212acf1 into open-telemetry:master

fbogsany mentioned this pull request

API: implement Sampling API #38

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet