Throttle debug traces #274

isaachier · 2018-03-26T18:38:30Z

No description provided.

codecov · 2018-03-26T19:45:11Z

Codecov Report

Merging #274 into master will increase coverage by 0.35%.
The diff coverage is 95%.

@@            Coverage Diff             @@
##           master     #274      +/-   ##
==========================================
+ Coverage   86.27%   86.62%   +0.35%     
==========================================
  Files          55       55              
  Lines        2972     3006      +34     
==========================================
+ Hits         2564     2604      +40     
+ Misses        287      282       -5     
+ Partials      121      120       -1

Impacted Files	Coverage Δ
metrics.go	`100% <ø> (ø)`	⬆️
span.go	`100% <100%> (+0.97%)`	⬆️
tracer.go	`89.62% <100%> (+1.05%)`	⬆️
internal/throttler/remote/throttler.go	`100% <100%> (ø)`	⬆️
config/config.go	`94.96% <100%> (+2.05%)`	⬆️
jaeger_thrift_span.go	`98.41% <100%> (+0.02%)`	⬆️
crossdock/endtoend/handler.go	`87.5% <25%> (-5.84%)`	⬇️
zipkin_thrift_span.go	`73.11% <0%> (+2.15%)`	⬆️

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 a7a6162...a7305c8. Read the comment docs.

vprithvi · 2018-03-26T20:13:09Z

config/config.go

+			),
+		)
+
+		fmt.Printf("Overriding default debug throttler, config = %+v, throttler = %+v\n", c.Throttler, debugThrottler)


Does this need to be cleaned up?

Good catch. I might have a handful of these I need to get rid of.

isaachier · 2018-03-26T20:22:53Z

internal/throttler/remote/throttler.go

 // requests are made.
 func (t *Throttler) SetProcess(process jaeger.Process) {
-	t.uuid.Store(process.UUID)
+	if clientID := process.ClientID(); clientID != "" {
+		fmt.Printf("Setting clientID to %+v\n", clientID)


Here too...

isaachier · 2018-03-26T20:28:07Z

internal/throttler/remote/throttler.go

-		if !t.synchronousInitialization {
+	value, ok := t.credits[operation]
+	if !ok || value == 0 {
+		if !ok {


@black-adder you might notice this changes the throttler to set operations to zero whenever they are encountered, ignoring the t.synchronousInitialization flag. The reason for this is to maintain the operations for the request that requires a list of operation names. Furthermore, that is why I changed the condition above to check for !ok || value == 0. When value is 0, it is either not initialized and just a placeholder, or it needs more credits. Either way, that indicates it is a good time to consider a synchronous request.

isaachier · 2018-03-26T20:28:41Z

internal/throttler/remote/throttler_test.go

-		resp := make([]creditResponse, len(operations))
-		for i, op := range operations {
-			resp[i] = creditResponse{Operation: op, Credits: h.credits}
+		operations := r.URL.Query()["operations"]


Minor difference. Not sure if this is more correct or less correct.

black-adder · 2018-03-26T20:45:56Z

config/config.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2017 Uber Technologies, Inc.
+// Copyright (c) 2017-2018 Uber Technologies, Inc.


Are you manually doing these?

Yes lol talk to @yurishkuro.

seems painful... I'll see how we can automate this

I think it can be done with the existing script if you append -<current-year> and limit the files to the list given by this command git diff --name-status master | grep -E '^M' | sed 's/^M\s\+//g'. That looks scarier than it is ;).

This doesn't seem to work on my Mac. Unclear why not.

Spent way too long researching this... Mac chokes on tab characters (see tab here https://github.com/git/git/blob/c6284da4ff4afbde8211efe5d03f3604b1c6b9d6/combine-diff.c#L1185). Maybe git diff --name-status master | grep -E '^M' | tr '\t' ';' | sed 's/^M;//g' instead.

Mac chokes on tab characters

with what error?

Actually more bizarre than that. In my terminal, any attempt to evaluate a tab character causes autocomplete. If I copy and paste this M process.go from the output into my input I get Makefileprocess.go. There seems to be no way to simply sed with a tab character on my Mac.

black-adder · 2018-03-27T14:32:01Z

config/config.go

+	// MaximumBalance limits the maximum number of credits the throttler will
+	// accumulate on a per-operation basis before it will stop requesting more
+	// credits. To leave it unbounded, leave this as zero.
+	MaximumBalance float64 `yaml:"maximumBalance"`


I am not a fan of this variable. The agent should handle all allocations and determining the maximum. Also, looking at the code, it looks like you have if maximumBalance is allocated, then the client doesn't make a request to the agent which means the operation will fall out of the agent's memory due to the TTL.

True. I will remove.

black-adder · 2018-03-27T14:36:55Z

internal/throttler/remote/throttler.go

-	Operation string  `json:"operation"`
-	Credits   float64 `json:"credits"`
-}
+type creditResponse map[string]float64


This isn't very future proof. If we add another field to the response (although I can't think of one at the moment), the json unmarshalling will fail. However, if we leave this as a struct, we can upgrade the agents to return the new field and still be backwards compatible with older versions of the client.

Interesting. I was avoiding a backend change but maybe this is worth fixing. In that case, we might as well use Thrift to enforce structure here.

Fix this one please

black-adder · 2018-03-27T14:40:55Z

tracer.go

@@ -151,7 +155,6 @@ func NewTracer(
 			"128 bit trace IDs, consider enabling the \"Gen128Bit\" option")
 	}
 	t.process = Process{
-		UUID:    "PLACEHOLDER", // TODO


the whole point of this field was to allow all the structs that implement ProcessSetter to use this client UUID. Yes it's unfortunate that we have to add the client uuid to both the tags and this field but I prefer passing the UUID as a first class variable rather than having all the ProcessSetter's iterate over tags to retrieve the client UUID.

Ya considered both. Essentially, I want the propagation through tags but the in process representation to be cached in a field. Wasn't sure if a field was preferred over a method. Up to you.

let's use the field. Since the ProcessSetter has already been released, removing UUID is a breaking change.

black-adder · 2018-03-27T14:44:07Z

metrics.go

+	ThrottledDebugSpans metrics.Counter `metric:"throttled_debug_spans"`
+
+	// Number of times throttler successfully updated.
+	ThrottlerUpdateSuccess metrics.Counter `metric:"throttler_update_success" tags:"result=ok"`


the tag result=ok kinda defeats naming the metric throttler_update_success

It should be metric:"throttler_updates" tags:"result=ok" and metric:"throttler_updates" tags:"result=err"

Ya I had no idea about these tags. Thanks for the explanation.

black-adder · 2018-03-27T15:04:49Z

also, looks like the config file needs to be covered

black-adder · 2018-04-13T16:09:59Z

I had a thought; do we want to let the throttler know of operations before IsAllowed is called? ie any time we see a new operation, we register it inside throttler.

EDIT: I guess it doesn't make sense if SynchronousInitialization is set. If it isn't set, might be useful. Meh

black-adder · 2018-04-16T18:17:28Z

tests are broken and I don't think we have 100% coverage

isaachier · 2018-04-16T20:15:44Z

Look at the errors. They don't make sense.

black-adder · 2018-04-16T21:40:20Z

internal/throttler/remote/throttler.go

 )

 type creditResponse struct {
-	Operation string  `json:"operation"`
-	Credits   float64 `json:"credits"`
+	BalancesByOperation map[string]float64 `json:"balancesByOperation"`


I still have the same 2 comments as before, I'd prefer this to not be a map but an array so it's more extensible in the future. The internal representation inside the client can be a map but json should be array. How much of a headache is this to change?

I wasn't sure if it was using maps at all or just a very unstructured map with no fields to encapsulate it. I don't mind a list so much just don't think it's inherently more extensible than a map. Some protocols implements maps while others don't. Thrift seems to prefer lists.

in the future, if we use the creditResponse to handle both debug throttling and normal sampling throttling, a 1 to 1 mapping won't work. You can either change it to a json list or map[string]struct{}. Where does thrift come into this? Looks like we don't have a thrift definition for the credits payload.

black-adder · 2018-04-16T21:41:30Z

process.go

-	UUID    string
-	Tags    []Tag
+	Service  string
+	ClientID string


This is a breaking change. Hopefully no one has used it yet but it feels very iffy given we've already cut a version of jaeger client with UUID

Right. I can change it back. Just changed the meaning in other places so I didn't think to leave this as UUID.

Just change this back to uuid, the rest can still be clientID

black-adder · 2018-04-17T16:24:06Z

damn DCO, squash all your commits into one, sign it and then do a force push.

isaachier · 2018-04-17T16:25:54Z

Ya lol I've been careful but it is futile because one of the first commits you had is missing it. Will squash in a sec.

Signed-off-by: Isaac Hier <isaachier@gmail.com>

black-adder · 2018-04-17T21:44:17Z

metrics.go

@@ -82,6 +82,15 @@ type Metrics struct {

 	// Number of times baggage restrictions failed to update.
 	BaggageRestrictionsUpdateFailure metrics.Counter `metric:"baggage_restrictions_updates" tags:"result=err"`
+
+	// Number of times debug spans were throttled.
+	ThrottledDebugSpans metrics.Counter `metric:"throttled_debug_spans"`


can you assert that these metrics are being used?

Signed-off-by: Isaac Hier <ihier@uber.com>

isaachier · 2018-04-20T17:16:41Z

Can someone please rerun the above build? I see a bizarre error occurred in Travis:

# cd .; git clone https://go.googlesource.com/lint /home/travis/gopath/src/golang.org/x/lint
Cloning into '/home/travis/gopath/src/golang.org/x/lint'...
fatal: git fetch-pack: expected ACK/NAK, got '?error: Git repository not found'
fatal: The remote end hung up unexpectedly

- Support throttling for debug traces (#274) <Isaac Hier> - Remove dependency on Apache Thrift (#303) <Yuri Shkuro> - Remove dependency on tchannel (#295) (#294) <Yuri Shkuro> - Test with Go 1.9 (#298) <Yuri Shkuro> Signed-off-by: Yuri Shkuro <ys@uber.com>

isaachier force-pushed the throttle_debug_traces branch 2 times, most recently from 940e402 to ff33499 Compare March 26, 2018 19:00

vprithvi reviewed Mar 26, 2018

View reviewed changes

isaachier force-pushed the throttle_debug_traces branch from 4c43b5d to 78bc99d Compare March 26, 2018 20:16

This was referenced Mar 26, 2018

Throttle debug traces #264

Closed

UUID #248

Closed

isaachier commented Mar 26, 2018

View reviewed changes

black-adder reviewed Mar 27, 2018

View reviewed changes

isaachier force-pushed the throttle_debug_traces branch from d5c7387 to 8a97bab Compare March 27, 2018 17:23

isaachier force-pushed the throttle_debug_traces branch from cdf520a to 5a13b65 Compare April 16, 2018 20:33

black-adder reviewed Apr 16, 2018

View reviewed changes

isaachier force-pushed the throttle_debug_traces branch 2 times, most recently from 819e06d to e1e59ee Compare April 17, 2018 16:28

Implement client throttler

7e16cbc

Signed-off-by: Isaac Hier <isaachier@gmail.com>

isaachier force-pushed the throttle_debug_traces branch from e1e59ee to 7e16cbc Compare April 17, 2018 16:39

black-adder approved these changes Apr 17, 2018

View reviewed changes

black-adder reviewed Apr 17, 2018

View reviewed changes

Isaac Hier added 2 commits April 18, 2018 15:13

Minor changes

9c61135

Signed-off-by: Isaac Hier <ihier@uber.com>

Test that metrics are used

a7305c8

Signed-off-by: Isaac Hier <ihier@uber.com>

black-adder merged commit fa1caaa into jaegertracing:master Apr 20, 2018

isaachier mentioned this pull request Jun 12, 2018

Why sign each and every commit? jaegertracing/jaeger#866

Closed

isaachier deleted the throttle_debug_traces branch June 12, 2018 13:42

		@@ -1,4 +1,4 @@
		// Copyright (c) 2017 Uber Technologies, Inc.
		// Copyright (c) 2017-2018 Uber Technologies, Inc.

Throttle debug traces #274

Throttle debug traces #274

Conversation

isaachier commented Mar 26, 2018

codecov bot commented Mar 26, 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

isaachier Mar 27, 2018 • 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

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 Mar 27, 2018

black-adder commented Apr 13, 2018 • edited Loading

black-adder commented Apr 16, 2018

isaachier commented Apr 16, 2018

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 Apr 17, 2018

isaachier commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaachier commented Apr 20, 2018

codecov bot commented Mar 26, 2018 •

edited

Loading

isaachier Mar 27, 2018 •

edited

Loading

black-adder commented Apr 13, 2018 •

edited

Loading