Use opencensus's DefaultSampler instead of AlwaysSampler as default

[rueian]

The GRPCServerTrace and HTTPServerTrace provided in the tracing/opencensus package will use trace.AlwaysSample() to trace span by default.

But I think using trace.AlwaysSample() as default is not a good idea, because it might cost a lot of money, especially when sending spans to cloud service providers, for example Stackdriver.

This pull request remove the lines that setting trace.AlwaysSample() as default value of cfg.Sampler, and leave the cfg.Sampler default to nil.

And the trace.StartSpanWithRemoteParent() and trace.StartSpan() will use their default sampler if cfg.Sampler is nil.

Users can change their default sampler by trace.ApplyConfig() first:

import "go.opencensus.io/trace"

sampler := trace.AlwaysSample() // or trace.ProbabilitySampler(1e-4)

trace.ApplyConfig(trace.Config{
    DefaultSampler: sampler,
})

[basvanbeek]

I'm conflicted about this one. I strongly disagree with AlwaysSample being a bad default as typically the first steps in distributed tracing for people is to see the propagation between and within services work properly, This needs everything to show up where as at a much later stage, moving to production means finding the right configuration (which is different for everyone) and enforcing it on production builds. The amount of time waisted by people new to tracing figuring out what is happening when it is simply because spans are dropped is awful.

Unfortunately OpenCensus has set a default probability sampler of 1e4 instead of an AlwaysSample or rate limited sampler. Given the default for OpenCensus it will be the assumption of people having worked with OpenCensus before that we follow the default behavior.

I'm tempted to accept this PR but would like a couple of others to chime in.

WDYT @adriancole @jcchavezs @peterbourgon @odeke-em

[codefromthecrypt]

I agree that using a low probability for sampling as a default repeats the lessons learned a few years ago with finagle's default low probability. It will certainly cause a support burden and likely a second order one (ex in zipkin channel not here).

something more appropriate will feel like amazon's which is 1 per second with an overflow, or lacking that always sampler https://docs.aws.amazon.com/xray/latest/devguide/xray-console-sampling.html

[jcchavezs]

I think anything different than AlwaysSample will lead to confusion to starters (1 per sec is great but user might be wondering why some traces are not there).

I am tempted to propose a more human solution which is dropping a log into stdout saying something like "no sampler defined, AlwaysSample is enabled but this is not meant to be used in production".

What about that?

[codefromthecrypt]

I think something more than 1 per second is probably good (especially for the voracious refresh clickers), but agree always is a less confusing default.

…

On Thu, Jan 24, 2019 at 7:07 PM José Carlos Chávez ***@***.***> wrote: I think anything different than AlwaysSample will lead to confusion to starters (1 per sec is great but user might be wondering why this is failing). I am tempted to propose a more human solution which is dropping a log into stdout saying something like no sampler defined, AlwaysSample is enabled but this is not meant to be used in production. What about that? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#830 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD613VjQq1HxyoEtZLPM19iI0AJoxa-ks5vGZP8gaJpZM4aQUe5> .

[rueian]

I also agree that it might confuse starters if the default sampler is not AlwaysSample.

However I think that setting AlwaysSample as default sampler in GRPCServerTrace and HTTPServerTrace makes these two functions inconvenient for applying a global sampler.

Users can't apply a global sampler just by doing this:

import "go.opencensus.io/trace"

sampler := trace.AlwaysSample() // or trace.ProbabilitySampler(1e-4)

trace.ApplyConfig(trace.Config{
    DefaultSampler: sampler,
})

but need to pass a opencensus.WithSampler() to all the GRPCServerTrace and HTTPServerTrace for each endpoint.

[basvanbeek]

Yes I agree. Even though I'm very unhappy with the OpenCensus default sampler choice the current way we force explicit setting of sampler at the Go kit level if needing something else than AlwaysSample is undesirable.

Merged... thanks for the PR and conversation @rueian