cmd/trace-agent: combine sampler rates by raphaelgavache · Pull Request #494 · DataDog/datadog-trace-agent

raphaelgavache · 2018-10-15T15:47:45Z

Traces are sampled by various independent samplers. If one decides to sample it, the trace is sampled. The usual samplers are: a scoreSampler and a prioritySampler.

In sampled traces we add the applied sampling_rate. This value is currently wrong as it does not take in account the sampling rate applied by the prioritySampler.

This PR aims at fixing the sample_rate values

Fetching Priority sampler sample rate
Retrieving client applied rate with the key _sampling_priority_rate_v1. If it does not exist, applying the next rate that will be sent to clients.
Merging signature|error sampling rates with priority.
Signature/Error samplers are run in parallel to the Priority sampler. Both are independent.
Sampling rate is, signatureSamplingRate + prioritySamplingRate - signatureSamplingRate*prioritySamplingRate
Add sampling rate to the pre-sampler and agent sampler rates that are already set in the trace
Isolate sampling decision in a function and have specific tests for it

raphaelgavache · 2018-10-17T14:04:25Z

+	}
+
+	sampledTrace.Transactions = a.TransactionSampler.Extract(pt)
+	// TODO: attach to these transactions the client, pre-sampler and transaction sample rates.


Not sure of what was the goal of Transactions.
Should I attach specific sample rates there somehow?

You can forget about that TODO this will be addressed in https://github.com/DataDog/datadog-trace-agent/compare/benjamin/new-transaction-sampling?expand=1

raphaelgavache · 2018-10-17T14:04:59Z

 	SamplingPriorityKey = "_sampling_priority_v1"
-	syncPeriod          = 3 * time.Second
+	// SamplingPriorityRateKey is the key to get the sample rate applied to a trace
+	SamplingPriorityRateKey = "_sampling_priority_rate_v1"


Need to agree on the key format and it should be added to tracers payloads

Did you agree? Are we keeping this?

gbbr

There's a lot of questions here. I'd suggest we clarify all of them in Google Doc before starting the actual work. /cc @bmermet for questions regarding transactions.

It is not clear what this PR is solving. Also, I'd like very small tests running a suite of sub-tests that provide nothing but input and output so that it is clearly visible what is tested (e.g. check out table-driven tests). I'm happy to help with this. Next week I will spend time with you on this PR, hoping that you will be free. In the meantime, it'd be nice to put together a Google Doc where all the things that are outstanding questions here get clarified. Then, we can start the work on them.

gbbr

The logic is good, but we need to improve a bit on the testing and on making it slightly more idiomatic in terms of Go. I hope my comments make sense.

gbbr · 2018-10-22T08:15:19Z

-			// The sampler will keep or discard the trace, but we send everything so that it
-			// gets the big picture and can set the sampling rates accordingly.
-			samplers = append(samplers, a.PrioritySampler)
+		sampledTrace := a.sample(pt, hasPriority)


This part should look like this:

rate, ok := a.sample(pt) if !ok { return } sampler.AddSampleRate(pt.Root, rate) a.sampledTraceChan <- writer.SampledTrace{ Trace: pt.Trace, Transactions: a.TransactionSampler.Extract(pt), }

We shouldn't have to check if it's empty here. If we've reached this point, it must not be. If this check is needed here, it means it was missing somewhere else and we should add it there.

Also, if the Empty function is not used nor needed anywhere, let's remove it entirely.

gbbr · 2018-10-22T08:19:33Z


-		// Trace sampling.
-		var sampledTrace writer.SampledTrace
+func (a *Agent) sample(pt processedTrace, hasPriority bool) *writer.SampledTrace {


This function should have the following signature, just like all the other samplers:

// sample returns true if the trace was sampled and the rate at which it was sampled. func (a *Agent) sample(pt processedTrace) (rate float64, ok bool) {

The hasPriority value can be obtained at no cost by doing:

_, ok := pt.Root.Metrics[sampler.SamplingPriorityKey]

gbbr · 2018-10-22T08:22:16Z

+		sampledTrace.Trace = &pt.Trace
+	}
+
+	sampledTrace.Transactions = a.TransactionSampler.Extract(pt)


Even though this is called "sampler" it's not exactly the same as the other samplers in this function. Let's move it back to Process. It has no place here.

gbbr · 2018-10-22T08:27:35Z

-		var sampledTrace writer.SampledTrace
+func (a *Agent) sample(pt processedTrace, hasPriority bool) *writer.SampledTrace {
+	var sampledTrace writer.SampledTrace
+	var sampleRate, scoreSampleRate float64


I think it's a good idea when multiple things are of the same type, to prefix them with that type, instead of suffixing. For example:

var ( rate1, rate2 float64 ok1, ok2 bool )

or

var ( ratePriority, rateScore float64 ok1, ok2 bool )

As you prefer.

Similar to the standard library's net/http constants.

gbbr · 2018-10-23T07:58:40Z

+
+// MergeParallelSamplingRates merges two rates from Sampler1, Sampler2. Both samplers law are independant,
+// and {sampled} = {sampled by Sampler1} or {sampled by Sampler2}
+func MergeParallelSamplingRates(rate1 float64, rate2 float64) float64 {


Let's call it CombineRates, it sounds much nicer when put into context. We should always consider the package name as part of the function name, in this case it's sampler.CombineRates vs. sampler.MergeParallelSamplingRates.

Do we need the world "parallel" or "merge"? I am not familiar with this formula.

gbbr · 2018-10-23T08:02:37Z

 // Signature based on the hash of (env, service, name, resource, is_error) for the root, plus the set of
 // (env, service, name, is_error) of each span.
-func computeSignatureWithRootAndEnv(trace model.Trace, root *model.Span, env string) Signature {
+func ComputeSignatureWithRootAndEnv(trace model.Trace, root *model.Span, env string) Signature {


Please revert the changes in this file, we won't need to use these. We shouldn't export something (technically making it public API) for the sake of having it usable in tests.

gbbr · 2018-10-23T08:03:23Z

+	}
+}
+
+func TestSamplingRace(t *testing.T) {


Please remove this test, we don't need it. Tests already run in parallel and any races will be picked up by the race detector (if it is enabled).

Sorry I might have missed it.
What I'm testing here is parallel calls of a same trace signature on a unique sampler. This would trigger a race condition with the race test.

gbbr

Thanks a lot for making the changes. It is looking a lot better. You forgot to address some of my comments, and I've added a couple more.

gbbr · 2018-10-25T07:36:53Z

-		if sampled {
-			sampledTrace.Trace = &pt.Trace
-		}
+	_, hasPriority := pt.Root.Metrics[sampler.SamplingPriorityKey]


if _, ok := pt.Root.Metrics[sampler.SamplingPriorityKey]; ok {

gbbr · 2018-10-25T07:37:13Z

-	}()
+	sampled = sampled || scoreSampled
+	sampleRate = sampler.MergeParallelSamplingRates(scoreSampleRate, sampleRate)
+	sampler.AddSampleRate(pt.Root, sampleRate)


You didn't follow this comment. Move this to Process and only apply it if the decision is to sample.

gbbr · 2018-10-25T07:41:28Z

+				Service:  "serv1",
+				Start:    time.Now().UnixNano(),
+				Duration: (100 * time.Millisecond).Nanoseconds(),
+				Metrics:  map[string]float64{}}


Please put all closing curly braces on the next line.

gbbr · 2018-10-25T07:44:12Z

 	atomic.AddUint64(&s.totalTraceCount, 1)

-	if s.engine.Sample(t.Trace, t.Root, t.Env) {
+	sampled, sampleRate = s.engine.Sample(t.Trace, t.Root, t.Env)


Please follow this comment. The word "sample" is repeated too much. Makes it hard to follow reading the code.

Suggested change

sampled, sampleRate = s.engine.Sample(t.Trace, t.Root, t.Env)

sampled, rate = s.engine.Sample(t.Trace, t.Root, t.Env)

gbbr · 2018-10-25T07:45:16Z

+	SetTraceAppliedSampleRate(root, newRate)
+}
+
+// MockEngine mocks a sampler engine


This is a test component, so it must go in a test file, maybe coresampler_test.go?

gbbr · 2018-10-25T07:47:08Z

 	SamplingPriorityKey = "_sampling_priority_v1"
-	syncPeriod          = 3 * time.Second
+	// SamplingPriorityRateKey is the key to get the sample rate applied to a trace
+	SamplingPriorityRateKey = "_sampling_priority_rate_v1"


Did you agree? Are we keeping this?

gbbr · 2018-10-25T07:55:46Z

 	}
 }
+
+type mockSamplerEngine struct {


This must also go in a test file.

gbbr · 2018-10-25T07:55:57Z

+
+func newMockSampler(wantSampled bool, wantRate float64) *Sampler {
+	return &Sampler{
+		engine: sampler.NewMockEngine(wantSampled, wantRate)}


Curly on next line here too please 🙏

gbbr

Nice one! I like how it turned out. Thanks for being patient with all the changes.

raphaelgavache added the do-not-merge/WIP label Oct 16, 2018

raphaelgavache force-pushed the raphael/fix_sampling_rate branch from ed7bcf1 to fa7f681 Compare October 16, 2018 13:40

raphaelgavache removed the do-not-merge/WIP label Oct 16, 2018

raphaelgavache requested review from LotharSee and gbbr October 16, 2018 13:49

raphaelgavache force-pushed the raphael/fix_sampling_rate branch from fa7f681 to b83d6e8 Compare October 17, 2018 14:02

raphaelgavache commented Oct 17, 2018

View reviewed changes

gbbr suggested changes Oct 17, 2018

View reviewed changes

gbbr changed the title ~~Fix sample_rate~~ cmd/trace-agent: combine sampler rates Oct 23, 2018

gbbr reviewed Oct 23, 2018

View reviewed changes

Comment thread sampler/coresampler.go

raphaelgavache force-pushed the raphael/fix_sampling_rate branch from b83d6e8 to b1882e2 Compare October 24, 2018 15:09

gbbr reviewed Oct 25, 2018

View reviewed changes

raphaelgavache added 5 commits October 25, 2018 14:43

Fix sampling rate

d406e88

Add complete sampling rate test

8f5ad76

Add race test

595b6b8

Update after Gabriel's review

32807b2

Update2 after Gabriel's review

225e919

raphaelgavache force-pushed the raphael/fix_sampling_rate branch from 7c53e4f to 225e919 Compare October 25, 2018 12:43

gbbr added 3 commits October 25, 2018 18:18

Merge branch 'master' into raphael/fix_sampling_rate

90a7dfc

sampler: remove testing.go and move into coresampler_test.go

3ae5316

sampler: move MockEngine into testutil

26b33b2

gbbr approved these changes Oct 25, 2018

View reviewed changes

gbbr merged commit 456dc89 into master Oct 25, 2018

gbbr deleted the raphael/fix_sampling_rate branch October 25, 2018 16:34

gbbr added this to the 6.7.0 milestone Oct 25, 2018

	sampled, sampleRate = s.engine.Sample(t.Trace, t.Root, t.Env)
	sampled, rate = s.engine.Sample(t.Trace, t.Root, t.Env)

Uh oh!

Conversation

raphaelgavache commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gbbr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gbbr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gbbr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gbbr Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gbbr Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gbbr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

raphaelgavache commented Oct 15, 2018 •

edited

Loading

gbbr left a comment •

edited

Loading

gbbr Oct 25, 2018 •

edited

Loading

gbbr Oct 25, 2018 •

edited

Loading