Add Jaeger throttling config #233

isaachier · 2018-05-23T19:33:44Z

No description provided.

CLAassistant · 2018-05-23T19:33:51Z

All committers have signed the CLA.

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

codecov · 2018-05-24T19:27:16Z

Codecov Report

Merging #233 into dev will increase coverage by 0.42%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##              dev     #233      +/-   ##
==========================================
+ Coverage   93.42%   93.84%   +0.42%     
==========================================
  Files          40       40              
  Lines        2721     2194     -527     
==========================================
- Hits         2542     2059     -483     
+ Misses        130       85      -45     
- Partials       49       50       +1

Impacted Files	Coverage Δ
main.go	`91.74% <84.61%> (-0.57%)`	⬇️
templateargs/interpolate/parse.go	`90.09% <0%> (-1.96%)`	⬇️
thrift/const.go	`94.28% <0%> (-0.72%)`	⬇️
thrift/response.go	`96.82% <0%> (-0.68%)`	⬇️
peerprovider/interface.go	`94.11% <0%> (-0.62%)`	⬇️
peerprovider/parse.go	`96.15% <0%> (-0.52%)`	⬇️
sorted/mapkeys.go	`100% <0%> (ø)`	⬆️
ratelimit/time_period.go	`100% <0%> (ø)`	⬆️
limiter/limiter.go	`100% <0%> (ø)`	⬆️
unmarshal/unmarshal.go	`100% <0%> (ø)`	⬆️
... and 27 more

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 2f9bbfd...a290a0f. Read the comment docs.

black-adder

duck typing an interface in the internal directory of another repo might be dangerous but I don't see a better way to do this. The alternative is to mock the agent but that's mocking the dependency of a dependency which seems even weirder. TLDR; this is ok(?)

isaachier · 2018-05-25T17:14:00Z

Ya I agree. @yurishkuro @vprithvi any thoughts about how to test debug trace throttling in this code without mocking Jaeger components here?

prashantv

Thanks for the PR, I have a couple of questions:

Will this impact tracer creation of run on a machine that doesn't have the Jaeger agent? E.g., local developer laptops?
Why are credits floats? It seems like they're used as ints throughout.

prashantv · 2018-06-01T15:21:47Z

integration_test.go

@@ -89,13 +89,10 @@ func verifyBaggage(ctx context.Context) error {
 		return errors.New("missing span")
 	}

-	spanCtx, ok := span.Context().(jaeger.SpanContext)
+	_, ok := span.Context().(jaeger.SpanContext)
 	if !ok {


nit: can you inline the ok := into the if,

if _, ok := span.Context().(jaeger.SpanContext); !ok { [...] }

prashantv · 2018-06-01T15:22:59Z

main.go

+		tracer, closer, err = jaeger_config.Configuration{
+			ServiceName: opts.TOpts.CallerName,
+			Throttler: &jaeger_config.ThrottlerConfig{
+				SynchronousInitialization: true,


can you explain this a little -- i'm assuming it means we try to get the throttling information synchronously? if that fails (e.g., on machines without jaeger), what is the impact?

Synchronous initialization is required for scripting tools such as yab that will likely exist in a short-lived process. For a service, we can periodically fetch the credit information (i.e. every 10 seconds). That is not feasible in a short script, so we set this flag to indicate we need to request credits from the Jaeger agent synchronously if there are no credits available for a given operation (credits represent number of debug spans we can emit without violating the throttling API). On machines without Jaeger, this would fail the request and refuse to emit debug spans, which is fine for those machines without agents. I would have thought the initial query could take a while to determine that the agent is not running and return an error, but local tests indicate no significant lag.

@isaachier add this justification as a code comment.

prashantv · 2018-06-01T15:24:26Z

utils_for_test.go

+	return getTestTracerWithCredits(serviceName, credits, nil)
+}
+
+func getTestTracerWithCredits(serviceName string, credits float64, t *testing.T) (opentracing.Tracer, io.Closer) {


why is credits a float?

Just because the rate limiting API uses a token bucket to increment the total... floats are easier to use when you have a constant rate over time. The spending amount is always 1.

Changing this to ints for internal mocks.

prashantv · 2018-06-01T15:24:36Z

utils_for_test.go

+	return getTestTracerWithCredits(serviceName, credits, nil)
+}
+
+func getTestTracerWithCredits(serviceName string, credits float64, t *testing.T) (opentracing.Tracer, io.Closer) {


make t *testing.T the first argument

Makes sense. Good idea.

prashantv · 2018-06-01T15:24:47Z

utils_for_test.go

 func getTestTracer(serviceName string) (opentracing.Tracer, io.Closer) {
-	return jaeger.NewTracer(serviceName, jaeger.NewConstSampler(true), jaeger.NewNullReporter())
+	const (


nit: no need for var group, just const credits = 10 is fine

Good to know. Didn't think about that.

prashantv · 2018-06-01T15:25:05Z

utils_for_test.go

+	t             *testing.T
+}
+
+func newDebugTraceCounter(numDebugTraces int, t *testing.T) jaeger.Reporter {


t *testing.T should be the first argument

prashantv · 2018-06-01T15:25:18Z

utils_for_test.go

+}
+
+func (d *debugTraceCounter) Report(span *jaeger.Span) {
+	if span == nil {


is this actually possible?

Doesn't seem to be. Will remove. I miss assert(span != nil) 😢.

prashantv · 2018-06-01T15:25:24Z

utils_for_test.go

+func (d *debugTraceCounter) Close() {
+	d.Lock()
+	defer d.Unlock()
+	if d.t == nil {


will this ever happen?

~~Same assertion pattern from above. Will remove.~~

Actually, this is the case if any test case calls getTestTracer instead of getTestTracerWithCredits. The former method does not take a *testing.T argument, and we hit this if statement upon tracer.Close().

prashantv · 2018-06-01T15:25:48Z

utils_for_test.go

+		creditsPerDebugTrace = 1
+	)
+	d.Lock()
+	defer d.Unlock()


nit: can you add a blank line after the defer

isaachier · 2018-06-04T21:39:22Z

Added some comments.

isaachier · 2018-06-05T00:41:09Z

main.go

@@ -360,7 +361,27 @@ func getTracer(opts Options, out output) (opentracing.Tracer, io.Closer) {
 		closer io.Closer
 	)
 	if opts.TOpts.Jaeger && !opts.TOpts.NoJaeger {
-		tracer, closer = jaeger.NewTracer(opts.TOpts.CallerName, jaeger.NewConstSampler(true), jaeger.NewNullReporter())
+		// yab must set the `SynchronousInitialization` flag to indicate that


Comment explaining SynchronousInitialization.

isaachier · 2018-06-07T17:49:14Z

@prashantv anything else?

prashantv · 2018-06-07T18:20:44Z

.travis.yml

@@ -2,8 +2,8 @@ sudo: false
 language: go

 go:
-    - 1.8
-    - 1.9
+    - "1.9"


is this required?

The 1.10 addition is needed for the new Zap that we use. Something about type aliasing there. If you are referring to the quotes, Travis explains that without quotes, 1.10 is a float, so it becomes 1.1.

prashantv · 2018-06-07T18:21:45Z

main.go

+		// yab must set the `SynchronousInitialization` flag to indicate that
+		// the Jaeger client must fetch debug credits synchronously. In a
+		// short-lived process like yab, the Jaeger client cannot afford to
+		// postpone the credit request for later in time.


please add a note about what happens if this fails (e.g., that it will just not send spans).

the comment should make it clear that err != nil does not happen if we fail to get the credits synchronously

prashantv · 2018-06-07T18:22:26Z

main.go

+		// short-lived process like yab, the Jaeger client cannot afford to
+		// postpone the credit request for later in time.
+		var err error
+		tracer, closer, err = jaeger_config.Configuration{


can we move all of the logic (which is now no longer a simple line) into a separate function, so the comment + all the logic for synchronous initialization is in a single createJaegerTracer function

prashantv · 2018-06-07T18:23:12Z

utils_for_test.go

+
+func (d *debugTraceCounter) Close() {
+	d.Lock()
+	defer d.Unlock()


nit: blank after defer

prashantv · 2018-06-07T18:24:53Z

utils_for_test.go

 func getTestTracer(serviceName string) (opentracing.Tracer, io.Closer) {
-	return jaeger.NewTracer(serviceName, jaeger.NewConstSampler(true), jaeger.NewNullReporter())
+	const credits = 10
+	return getTestTracerWithCredits(nil, serviceName, credits)


there's only 2 callers of getTestTracer, i'd suggest changing it to take a *testing.T as the first argument, and propagating it, instead of the nil check you have above.

OK if that is the entire usage, I am totally fine. I was under the impression there were more callers.

prashantv · 2018-06-08T15:29:48Z

main.go

-	} else if len(opts.ROpts.Baggage) > 0 {
+		return createJaegerTracer(opts, out)
+	}
+	if len(opts.ROpts.Baggage) > 0 {
 		out.Fatalf("To propagate baggage, you must opt-into a tracing client, i.e., --jaeger")
 	}
 	return tracer, closer


no need for vars at the beginning, you can just do:

return opentracing.NoopTracer{}, closer

prashantv

Thanks for addressing all my comments, @yurishkuro, does this look good to you as well?

isaachier · 2018-06-08T16:27:51Z

main.go

 	if opts.TOpts.Jaeger && !opts.TOpts.NoJaeger {
-		tracer, closer = jaeger.NewTracer(opts.TOpts.CallerName, jaeger.NewConstSampler(true), jaeger.NewNullReporter())


@prashantv why was yab using a null reporter? Do you have a specific need to omit the root span?

~~I have just changed this based on @yurishkuro's advice. Unless you mind, we might as well do this all at once.~~

I was going to suggest not making this discussion a part of this PR, but move to #234.

I'm going to undo this and potentially open a new PR about this.

yurishkuro · 2018-06-08T16:40:27Z

main.go

+		// only enable the span when we have not hit the throttling
+		// threshold.
+		jaeger_config.Sampler(jaeger.NewConstSampler(false)),
+		jaeger_config.Reporter(jaeger.NewNullReporter()),


I think these two customizations are not necessary, but can be discussed separately - #234.

yurishkuro

lgtm

isaachier · 2018-06-11T16:46:03Z

@prashantv can you please merge this PR?

isaachier · 2018-06-11T17:22:39Z

Thanks!

Add Jaeger throttling config

887371b

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

isaachier force-pushed the jaeger-debug-throttling branch from 68ad722 to 887371b Compare May 23, 2018 19:58

Isaac Hier added 3 commits May 23, 2018 17:06

Update lock file

628b9e7

Fix integration test

453327c

Add throttling test

66c9b97

black-adder reviewed May 25, 2018

View reviewed changes

Use const false sampler and override when we can

a4404bc

prashantv suggested changes Jun 1, 2018

View reviewed changes

Isaac Hier added 2 commits June 4, 2018 17:59

Minor fixes, use int for credit representation

209261b

Upgrade Go versions

4e3e297

isaachier force-pushed the jaeger-debug-throttling branch from 3a98ebd to 4e3e297 Compare June 4, 2018 22:04

isaachier commented Jun 5, 2018

View reviewed changes

prashantv reviewed Jun 7, 2018

View reviewed changes

Clarify error behavior for Jaeger throttling

605c05b

prashantv reviewed Jun 8, 2018

View reviewed changes

Remove unnecessary var declarations

a290a0f

prashantv approved these changes Jun 8, 2018

View reviewed changes

isaachier commented Jun 8, 2018

View reviewed changes

isaachier force-pushed the jaeger-debug-throttling branch from 56023bb to a290a0f Compare June 8, 2018 16:33

yurishkuro reviewed Jun 8, 2018

View reviewed changes

yurishkuro approved these changes Jun 8, 2018

View reviewed changes

prashantv merged commit 539a20d into yarpc:dev Jun 11, 2018

isaachier deleted the jaeger-debug-throttling branch June 11, 2018 17:22

		if opts.TOpts.Jaeger && !opts.TOpts.NoJaeger {
		tracer, closer = jaeger.NewTracer(opts.TOpts.CallerName, jaeger.NewConstSampler(true), jaeger.NewNullReporter())

Add Jaeger throttling config #233

Add Jaeger throttling config #233

Conversation

isaachier commented May 23, 2018

CLAassistant commented May 23, 2018 • edited Loading

codecov bot commented May 24, 2018 • edited Loading

Codecov Report

black-adder left a comment

Choose a reason for hiding this comment

isaachier commented May 25, 2018

prashantv left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaachier Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaachier Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaachier commented Jun 4, 2018

Choose a reason for hiding this comment

isaachier commented Jun 7, 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

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

prashantv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaachier Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

isaachier commented Jun 11, 2018

isaachier commented Jun 11, 2018

CLAassistant commented May 23, 2018 •

edited

Loading

codecov bot commented May 24, 2018 •

edited

Loading

isaachier Jun 4, 2018 •

edited

Loading

isaachier Jun 4, 2018 •

edited

Loading

isaachier Jun 8, 2018 •

edited

Loading

yurishkuro Jun 8, 2018 •

edited

Loading