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

Trace sampler interface is now just a Proc #1157

Merged
merged 2 commits into from
Dec 20, 2016

Conversation

dazuma
Copy link
Member

@dazuma dazuma commented Dec 20, 2016

Overall, clean up interfaces and documentation around sampling. Specifically:

  • Change the method on the sampler interface from check to call so a Proc can be used. Sorry for that annoyance; I've been writing too much Java these past couple years.
  • Make it explicit that the rack env is passed to the sampler.
  • Path blacklist is really just a special case of sampling, so move that out of Middleware into TimeSampler. That lets us simplify the middleware interface a bit.
  • Rename sampling.rb to time_sampler.rb because it defines TimeSampler.
  • Clarify documentation around sampling, by including a section in the main Google::Cloud::Trace guide and adding details to the TimeSampler docs.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 20, 2016
@blowmage
Copy link
Contributor

I like this change. 👍

def initialize qps: 0.1, path_blacklist: DEFAULT_PATH_BLACKLIST
@delay_secs = 1.0 / qps
@last_time = ::Time.now.to_f - @delay_secs
@path_blacklist = path_blacklist

This comment was marked as spam.

This comment was marked as spam.

@dazuma
Copy link
Member Author

dazuma commented Dec 20, 2016

@hxiong388 Updated docs to call out sampling and blacklisting. PTAL.

@dazuma dazuma merged this pull request into googleapis:stackdriver-core-dependencies Dec 20, 2016
@dazuma dazuma deleted the sampling-proc branch December 20, 2016 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants