Start moving components of adaptive sampling to OSS #973

black-adder · 2018-08-07T03:23:52Z

Which problem is this PR solving?

This PR is the start of moving adaptive sampling (#365) over to OSS. This is moving over calculation_strategy and leader_election code.

codecov · 2018-08-07T03:38:09Z

Codecov Report

❗ No coverage uploaded for pull request base (master@c296998). Click here to learn what that means.
The diff coverage is 100%.

@@           Coverage Diff           @@
##             master   #973   +/-   ##
=======================================
  Coverage          ?   100%           
=======================================
  Files             ?    157           
  Lines             ?   7101           
  Branches          ?      0           
=======================================
  Hits              ?   7101           
  Misses            ?      0           
  Partials          ?      0

Impacted Files	Coverage Δ
...sampling/internal/calculationstrategy/interface.go	`100% <100%> (ø)`
pkg/testutils/logger.go	`100% <100%> (ø)`
...nstrategy/percentage_increase_capped_calculator.go	`100% <100%> (ø)`
...ampling/internal/leaderelection/leader_election.go	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c296998...5f52d42. Read the comment docs.

yurishkuro · 2018-08-08T03:11:59Z

pkg/testutils/logger.go

@@ -88,3 +89,17 @@ func (b *Buffer) Write(p []byte) (int, error) {
 	defer b.Unlock()
 	return b.Buffer.Write(p)
 }
+
+// LogMatcher is a helper func that returns true if the subStr appears more than 'occurrences' times in the logs.
+var LogMatcher = func(occurrences int, subStr string, logs []string) bool {


(non-blocking) nit: not a fun of such function since the way you would use it is with assert.True(), which provides no feedback in case of mismatch. I prefer passing the T object and doing asserts directly.

passing in T makes this hard to test since if we try to test the failure case, the test itself will fail (unless I can mock T which seems overkill). I'll update this func to return (bool, errMsg)

yurishkuro · 2018-08-08T03:13:33Z

plugin/sampling/strategystore/adaptive/calculationstrategy/interface.go

@@ -0,0 +1,28 @@
+// Copyright (c) 2018 The Jaeger Authors.


why is this under strategystore?

yurishkuro · 2018-08-08T03:16:01Z

plugin/sampling/strategystore/adaptive/calculationstrategy/interface.go

+}
+
+// Func wraps a function of appropriate signature and makes a ProbabilityCalculator from it.
+type Func func(targetQPS, curQPS, prevProbability float64) (newProbability float64)


Do you envision ever having other methods in the interface? Consider:

type Calculate func(targetQPS, curQPS, prevProbability float64) (newProbability float64)

yurishkuro · 2018-08-08T03:20:29Z

plugin/sampling/strategystore/adaptive/internal/leader_election.go

@@ -0,0 +1,117 @@
+// Copyright (c) 2018 The Jaeger Authors.


again, weird package path. Too long on one hand, while .../internal/leader_election/*.go would make more sense than being directly in 'internal'.

yurishkuro · 2018-08-08T03:21:32Z

plugin/sampling/strategystore/adaptive/internal/leader_election.go

+// ElectionParticipant partakes in leader election to become leader.
+type ElectionParticipant interface {
+	IsLeader() bool
+	Start()


I am not sure Start() semantically belongs in this interface.

yurishkuro · 2018-11-13T04:31:01Z

plugin/sampling/internal/calculationstrategy/interface.go

+}
+
+// Calculate wraps a function of appropriate signature and makes a ProbabilityCalculator from it.
+type Calculate func(targetQPS, curQPS, prevProbability float64) (newProbability float64)


std lib calls this XyzFunc

yurishkuro · 2018-11-13T04:31:08Z

pkg/testutils/logger.go

@@ -88,3 +89,17 @@ func (b *Buffer) Write(p []byte) (int, error) {
 	defer b.Unlock()
 	return b.Buffer.Write(p)
 }
+
+// LogMatcher is a helper func that returns true if the subStr appears more than 'occurrences' times in the logs.
+var LogMatcher = func(occurrences int, subStr string, logs []string) bool {


yurishkuro · 2018-11-13T04:32:55Z

plugin/sampling/internal/calculationstrategy/interface.go

@@ -0,0 +1,28 @@
+// Copyright (c) 2018 The Jaeger Authors.


what is the thinking for this thing to be under plugins? I can see swappable storage to be under plugins, but this seems like the core impl. I would keep it inside the collector app.

This is core only to the adaptive sampling strategystore. Don't feel the collector app is the right place for this

gonna go ahead and land, we can figure out where these files should live later

yurishkuro · 2018-11-13T04:33:35Z

plugin/sampling/internal/leaderelection/leader_election.go

+)
+
+var (
+	acquireLockErrMsg = "Failed to acquire lock"


it's a const

plugin/sampling/internal/leaderelection/leader_election.go

Signed-off-by: Won Jun Jang <[email protected]>

black-adder requested review from jpkrohling, pavolloffay, vprithvi and yurishkuro as code owners August 7, 2018 03:23

ghost assigned black-adder Aug 7, 2018

ghost added the review label Aug 7, 2018

yurishkuro reviewed Aug 8, 2018

View reviewed changes

black-adder force-pushed the move_adaptive_sampling_over branch from c05121d to a3eb125 Compare November 12, 2018 23:55

black-adder requested review from objectiser and tiffon as code owners November 12, 2018 23:55

black-adder force-pushed the move_adaptive_sampling_over branch from a3eb125 to 41ec3ba Compare November 12, 2018 23:56

black-adder mentioned this pull request Nov 13, 2018

Move adaptive sampling processor over #1172

Closed

yurishkuro approved these changes Nov 13, 2018

View reviewed changes

black-adder added 6 commits November 14, 2018 02:00

Move adaptive sampling to OSS

09f226d

Signed-off-by: Won Jun Jang <[email protected]>

more

342ee1a

Signed-off-by: Won Jun Jang <[email protected]>

increase coverage

3c2fe4f

Signed-off-by: Won Jun Jang <[email protected]>

rename test

4605109

Signed-off-by: Won Jun Jang <[email protected]>

address comments

0b4a788

Signed-off-by: Won Jun Jang <[email protected]>

address comments

e272f60

Signed-off-by: Won Jun Jang <[email protected]>

black-adder force-pushed the move_adaptive_sampling_over branch from 807668d to fcaf701 Compare November 13, 2018 18:01

code coverage

5f52d42

Signed-off-by: Won Jun Jang <[email protected]>

black-adder force-pushed the move_adaptive_sampling_over branch from fcaf701 to 5f52d42 Compare November 13, 2018 18:13

black-adder merged commit 4cc25e6 into master Nov 13, 2018

ghost removed the review label Nov 13, 2018

pavolloffay deleted the move_adaptive_sampling_over branch November 13, 2018 20:57

yurishkuro mentioned this pull request Apr 7, 2019

Adaptive Sampling #365

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start moving components of adaptive sampling to OSS #973

Start moving components of adaptive sampling to OSS #973

black-adder commented Aug 7, 2018 •

edited by yurishkuro

Loading

codecov bot commented Aug 7, 2018 •

edited

Loading

yurishkuro Aug 8, 2018

yurishkuro Nov 13, 2018

black-adder Nov 13, 2018

yurishkuro Aug 8, 2018

yurishkuro Aug 8, 2018

yurishkuro Aug 8, 2018

yurishkuro Aug 8, 2018

yurishkuro Nov 13, 2018

yurishkuro Nov 13, 2018

yurishkuro Nov 13, 2018

black-adder Nov 13, 2018

black-adder Nov 13, 2018

yurishkuro Nov 13, 2018

Start moving components of adaptive sampling to OSS #973

Start moving components of adaptive sampling to OSS #973

Conversation

black-adder commented Aug 7, 2018 • edited by yurishkuro Loading

Which problem is this PR solving?

codecov bot commented Aug 7, 2018 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

black-adder commented Aug 7, 2018 •

edited by yurishkuro

Loading

codecov bot commented Aug 7, 2018 •

edited

Loading