[dual sampling] using both score and priority sampler for priority 0 traces #345

ufoot · 2017-12-27T10:42:05Z

In the case of priority 0 traces, send traces to both samplers. This gives
a second chance to priority 0 traces to be picked up by the score sampler.
Also adds stats for -1 sampling priority traces.

…traces In the case of priority 0 traces, send traces to both samplers. This gives a second chance to priority 0 traces to be picked up by the score sampler. Also adds stats for -1 sampling priority traces.

… to it

ufoot · 2017-12-27T14:09:30Z

config/agent.go

@@ -324,8 +326,11 @@ APM_CONF:
 	if v, e := conf.GetFloat("trace.sampler", "max_traces_per_second"); e == nil {
 		c.MaxTPS = v
 	}
-	if v := strings.ToLower(conf.GetDefault("trace.sampler", "priority_sampling", "")); v == "yes" || v == "true" {
-		c.PrioritySampling = true


This probably never worked, the default was true, and we were overriding it to true...

Good catch!

Useless after all ;)

AlexJF · 2017-12-27T14:21:23Z

agent/agent.go

+		} else {
+			samplers = []*Sampler{a.ScoreEngine}
+		}
+	}


Following looks cleaner IMO (everything related to priority grouped together and ok -> hasPriority):

samplers := make([]*Sampler, 0, 2) priority, hasPriority := root.Metrics[samplingPriorityKey] if hasPriority && a.PriorityEngine != nil { samplers = append(samplers, a.PriorityEngine) if a.conf.ScorePriority0Traces && priority == 0 { samplers = append(samplers, a.ScoreEngine) } } else { samplers = append(samplers, a.ScoreEngine) }

Just realized that to be a 100% match, the a.PriorityEngine should go to a separate if

samplers := make([]*Sampler, 0, 2) priority, hasPriority := root.Metrics[samplingPriorityKey] if hasPriority { if a.PriorityEngine != nil { samplers = append(samplers, a.PriorityEngine) } if a.conf.ScorePriority0Traces && priority == 0 { samplers = append(samplers, a.ScoreEngine) } } else { samplers = append(samplers, a.ScoreEngine) }

Hmm actually none of them are equivalent to yours now that I look closer :/ Maybe there's no way around it :P

Yes, I also spent some time on it initially, the "problem" is that in one case we do an if ... if ... and in the other one an if ... else .... Thanks for the thorough review.

As suggested in the config.go comment, let's just have both mechanisms always on, it should greatly simplify the code here.

AlexJF · 2017-12-27T14:24:43Z

config/agent.go

@@ -324,8 +326,11 @@ APM_CONF:
 	if v, e := conf.GetFloat("trace.sampler", "max_traces_per_second"); e == nil {
 		c.MaxTPS = v
 	}
-	if v := strings.ToLower(conf.GetDefault("trace.sampler", "priority_sampling", "")); v == "yes" || v == "true" {
-		c.PrioritySampling = true


Good catch!

talwai · 2017-12-27T15:42:28Z

agent/agent.go

+	if a.conf.ScorePriority0Traces {
+		// Send traces to possibly several score engines.
+		if ok && a.PriorityEngine != nil {
+			// If Priority is defined, send to priority sampling, regardless of priority value.


Should the count of "priority < 0" traces affect the applied sample rate in the client? Some apps will routinely generate traces which the user will want to ignore via the priority flag e.g. healthchecks, debug endpoints, whatever. Should the frequency of such traces still influence the sample rate?

Good point. Indeed, since the client lib can only influence the 0/-1 choice, it totally makes sense it takes decisions only on those ones. But then, I'm thinking we could also exclude the +2 traces. The problem is quite symmetrical here. In any case, I think this is really sampling logic, so should probably go to sampler package, easier to unit-test and bullet-proof rather than in this "high-level" code.

Also, I'm worried about how we can explain exactly what that -1 does. Because, it can sound like it's "exclude this from tracing". When, it's really not. The stats are still computed for instance. That's the whole point of this -1 thing I think, make sure there's no trace sampled, but still trace it. I'm saying this because we had similar issues whit the "disabled tracer" mode, which did not really disabled the tracer, but only the sending of traces to the agent.

I'm going to exclude -1 and +2 flagged traces from the count anyway.

ufoot · 2017-12-29T12:56:59Z

sampler/prioritysampler.go

-	sampled := root.Metrics[samplingPriorityKey] > 0
+	sampled := samplingPriority > 0
+
+	if samplingPriority < 0 || samplingPriority > 1 {


@talwai this is where I'd exclude the traces which should have no impact on sampling ratio. I had a second thought about it and really do think the priority 2 traces should be excluded too. Not doing so would a) break the ratio for other traces (keeping less of them, when this is really about "decide to have some more") and b) give a false illusion that having 1 and 2 in the same bag does provide some fail-safe rate limiting. It does not. On a service sending 100 traces per second, with a max TPS of 10, the default target ratio would be 0.1. If we select 50% of them with priority set to 2, then we have 50 traces per sec no matter what max TPS is set to. So if we actually include priority 2 traces in the count, it will result in having only the one we selected for that service, but also possibly impact other services, and in any case, there's no capping to 10 TPS. The big picture is: priority 2 traces are something on top of the regular sampling performed by the library, it's not included in it.

LotharSee · 2018-01-02T17:56:14Z

config/agent.go

+		PreSampleRate:        1.0,
+		MaxTPS:               10,
+		PrioritySampling:     true,
+		ScorePriority0Traces: true,


As the goal is to simplify and have both samplers, I'd suggest to just drop both PrioritySampling and ScorePriority0Traces while always having both logics working. I see no case where these configuration should be used nor exposed to the user.

ufoot · 2018-01-04T10:16:20Z

agent/agent.go

 	priorityPtr := &ts.TracesPriorityNone
-	if a.PriorityEngine != nil {


No need to check for it now, it's always defined since we removed the PrioritySampling option

ufoot · 2018-01-04T10:18:58Z

Companion PRs:

LotharSee

Great, the removal of the extra options is great and makes the code much simpler.

Combined with the clear explanation of each priority values, it was easy to review. Let's be sure now to update https://github.com/DataDog/datadog-trace-agent/wiki/Distributed-Priority-Sampling

Looking good to me.

[dual sampling] using both score and priority sampler for priority 0 …

c07dd27

…traces In the case of priority 0 traces, send traces to both samplers. This gives a second chance to priority 0 traces to be picked up by the score sampler. Also adds stats for -1 sampling priority traces.

ufoot force-pushed the christian/dualsampling branch from 26bb8b1 to c07dd27 Compare December 27, 2017 10:45

[dual sampling] introduced option to disable dual sampling

48a5831

ufoot force-pushed the christian/dualsampling branch from 4f87c0f to 48a5831 Compare December 27, 2017 13:13

[dual sampling] added Sample() unit test

371b9b5

ufoot changed the title ~~[dual sampling] (WIP) using both score and priority sampler for priority 0 traces~~ [dual sampling] using both score and priority sampler for priority 0 traces Dec 27, 2017

[dual sampling] make sure priority sampler exists before sending data…

b4004c6

… to it

ufoot commented Dec 27, 2017

View reviewed changes

ufoot requested review from talwai and AlexJF December 27, 2017 14:10

AlexJF approved these changes Dec 27, 2017

View reviewed changes

talwai reviewed Dec 27, 2017

View reviewed changes

[dual sampling] excluding -1 and +2 traces from rate sample count

f2462c8

ufoot commented Dec 29, 2017

View reviewed changes

LotharSee reviewed Jan 2, 2018

View reviewed changes

[dual sampling] removed config options to simplify code

64906b7

ufoot commented Jan 4, 2018

View reviewed changes

LotharSee approved these changes Jan 4, 2018

View reviewed changes

ufoot added this to the 5.20.1 milestone Jan 4, 2018

ufoot merged commit 1c07282 into master Jan 4, 2018

dtilghman deleted the christian/dualsampling branch January 5, 2018 05:17

palazzem modified the milestone: 5.21.1 Jan 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dual sampling] using both score and priority sampler for priority 0 traces #345

[dual sampling] using both score and priority sampler for priority 0 traces #345

ufoot commented Dec 27, 2017 •

edited

Loading

ufoot Dec 27, 2017

AlexJF Dec 27, 2017

ufoot Jan 4, 2018

AlexJF Dec 27, 2017

AlexJF Dec 27, 2017

AlexJF Dec 27, 2017

ufoot Dec 29, 2017

LotharSee Jan 2, 2018

AlexJF Dec 27, 2017

talwai Dec 27, 2017

ufoot Dec 29, 2017

ufoot Dec 29, 2017 •

edited

Loading

LotharSee Jan 2, 2018

talwai Jan 2, 2018

ufoot Jan 4, 2018

ufoot commented Jan 4, 2018

LotharSee left a comment

		priorityPtr := &ts.TracesPriorityNone
		if a.PriorityEngine != nil {

[dual sampling] using both score and priority sampler for priority 0 traces #345

[dual sampling] using both score and priority sampler for priority 0 traces #345

Conversation

ufoot commented Dec 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ufoot Dec 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ufoot commented Jan 4, 2018

LotharSee left a comment

Choose a reason for hiding this comment

ufoot commented Dec 27, 2017 •

edited

Loading

ufoot Dec 29, 2017 •

edited

Loading