Add remoteBaggageRestrictionManager #182

black-adder · 2017-08-01T23:18:43Z

Tests

black-adder · 2017-08-01T23:26:16Z

tracer_options.go

+
+func (tracerOptions) BaggageRestrictionsConfig(cfg *BaggageRestrictionsConfig) TracerOption {
+	return func(tracer *Tracer) {
+		tracer.baggageRestrictionsConfig = cfg


I wanted to copy: https://github.com/uber/jaeger-client-go/blob/master/tracer_options.go#L57 but what happens if metrics doesn't exist before that option is run?

why expose Config to the tracer instead of the actual baggage manager?

coveralls · 2017-08-01T23:55:39Z

Coverage decreased (-0.04%) to 85.355% when pulling 28607fc on Add_RemoteBaggageRestrictionManager into 08628f9 on master.

yurishkuro · 2017-08-02T04:11:00Z

This design affects all languages, and the API visibility issues are similar. Can the following work?

jaeger/
- interface BaggageSetter { SetBaggage() }
- span checks if baggageSetter == null then it stores the baggage manually, no additional logic, otherwise calls baggageSetter.SetBaggage(self, key value)
internal/baggage
- struct DefaultBaggageSetter
- deals with metrics and logging
- may want to rename baggageSetter to baggageActor (whatever)
- contains pointer to BaggageRestrictionManager that it uses for the rules
  - default BRM is - allow all
  - RemoteBaggageRestrictionManager pulls the rules from the agent
config
- BaggageManagerConfig
- creates BaggageRestrictionManager with RemoteBaggageRestrictionManager

This way the two API additions are BaggageSetter interface and BaggageManagerConfig struct. Everything else is either private or stashed under internal/. There don't seem to be any circular dependencies.

We could also go a bit further and move RemoteBRM under internal/baggage/remote. This way if people are not using config package, and don't want the RemoteBRM, they don't even need to compile that package into their app. Right now we're forcing everyone to compile almost everything from the client, we should really be doing a better job at isolation.

black-adder · 2017-08-02T17:53:55Z

What we can do instead:

jaeger/
- interface BaggageRestrictionManager { GetRestriction(key) (bool, int) }
- private baggageSetter initialized by tracer that has ptr to BaggageRestrictionManager (baggageSetter handles metrics and logs)
internal/baggage
- struct Default BRM - allow all
- struct Remote BRM pulls the rules from the agent
config
- BaggageManagerConfig
- creates BaggageRestrictionManager with RemoteBaggageRestrictionManager

We can take your approach in other languages, just not in go.

yurishkuro · 2017-08-02T20:41:55Z

interface BaggageRestrictionManager { GetRestriction(key) (bool, int) }

well my goal was to come up with a unified API across all languages. (bool, int) won't work in Java

black-adder · 2017-08-02T20:45:01Z

my bad

interface BaggageRestrictionManager { GetRestriction(key) Restriction }

type Restriction struct {
    valid bool
    maxValueLength int
    whatever in the future we want to add
}

and restriction is generated once in the DBRM and once a minute in RBRM

yurishkuro · 2017-08-02T21:07:18Z

So one drawback I see in your approach is "(baggageSetter handles metrics and logs)" - meaning there isn't any way to actually NOT log baggage without introducing more options to the Tracer (and increasing its surface). In my proposal the side-effects of setting baggage are encapsulated into another struct that can be replaced if needed, and Tracer is only exposed to its SetBaggage interface. Although when you mentioned circular dependency on the call, I see the potential for it here since the setter might need access to private Span API - :-( ...

black-adder · 2017-08-02T22:41:18Z

I addressed some of the concerns and moved as much of the code over to the internal directory. I'll address the design decisions inline

black-adder · 2017-08-02T22:44:38Z

tracer.go

@@ -109,8 +111,10 @@ func NewTracer(
 	zipkinPropagator := &zipkinPropagator{tracer: t}
 	t.addCodec(ZipkinSpanFormat, zipkinPropagator, zipkinPropagator)

-	if t.baggageRestrictionManager == nil {
-		t.baggageRestrictionManager = newDefaultBaggageRestrictionManager(&t.metrics, 0)
+	if t.baggageRestrictionManager != nil {


if the manager isn't passed in, the baggageSetter defaults to use the DefaultManager.

black-adder · 2017-08-02T22:46:12Z

baggage_setter.go

-type defaultBaggageSetter struct {
-	maxValueLength int
-	metrics        *Metrics
+type baggageSetter struct {


I opted to just make this a struct and not an interface. Given it's private anyway, if we ever need to change it to a interface it can be easily done.

black-adder · 2017-08-02T22:47:24Z

internal/baggage/restriction_manager.go

-type baggageRestrictionManager interface {
-	// setBaggage sets the baggage key:value on a span and the associated logs.
-	setBaggage(span *Span, key, value string)
+type Restriction struct {


I tried to make this immutable by adding the getters. This way we only need to generate this once and return it to the setter

black-adder · 2017-08-02T22:48:41Z

internal/baggage/restriction_manager.go

-// defaultBaggageRestrictionManager allows any baggage key.
-type defaultBaggageRestrictionManager struct {
-	setter baggageSetter
+func NewRestriction(valid bool, maxValueLength int) *Restriction {


not a fan of this, pollutes namespace and if we ever need to add extra restrictions, can't easily update the signature without a breaking change.

coveralls · 2017-08-02T23:12:51Z

Coverage decreased (-4.2%) to 81.231% when pulling 17e0565 on Add_RemoteBaggageRestrictionManager into 08628f9 on master.

oibe · 2017-08-03T17:03:30Z

internal/baggage/remote/restriction_manager_test.go

@@ -0,0 +1,216 @@
+// Copyright (c) 2017 Uber Technologies, Inc.


Why is this whole file commented out?

i wanted to iron out the API before writing the tests

yurishkuro

approach looks ok. Surprised there are no changes to Span in this diff - who calls tracer.setBaggage?

yurishkuro · 2017-08-03T18:07:01Z

baggage_setter.go

-}
-
-func logFields(span *Span, key, value, prevItem string, truncated, invalid bool) {
+func logFields(span *Span, key, value, prevItem string, truncated, valid bool) {


maybe make it a member function, to avoid polluting jaeger namespace?

yurishkuro · 2017-08-03T18:12:38Z

internal/baggage/restriction_manager.go

-	// setBaggage sets the baggage key:value on a span and the associated logs.
-	setBaggage(span *Span, key, value string)
+type Restriction struct {
+	valid          bool


s/valid/keyAllowed/ ?

black-adder · 2017-08-03T20:44:58Z

span calls tracer.setBaggage, but that was already done in a previous PR. Information hiding ftw

coveralls · 2017-08-04T16:18:30Z

Coverage decreased (-0.3%) to 85.057% when pulling ecc0680 on Add_RemoteBaggageRestrictionManager into 08628f9 on master.

coveralls · 2017-08-04T19:19:55Z

Coverage increased (+0.2%) to 85.573% when pulling fe802ec on Add_RemoteBaggageRestrictionManager into 08628f9 on master.

yurishkuro · 2017-08-05T00:08:53Z

baggage_setter.go

 	}
 }

-func (s *defaultBaggageSetter) setBaggage(span *Span, key, value string) {
+func (s baggageSetter) setBaggage(span *Span, key, value string) {


why do you pass the receiver by value?

https://gist.github.com/yurishkuro/b135e6750df0f95aa86c35a77cc736d4

yurishkuro

should be (mostly) ok to merge. Main comments are about using pointer-receiver (fix now) and the timeout of initial load (address in different PR).

yurishkuro · 2017-08-06T19:40:17Z

baggage_setter.go

 	}
 }

-func (s *defaultBaggageSetter) setBaggage(span *Span, key, value string) {
+func (s baggageSetter) setBaggage(span *Span, key, value string) {


Is there expectation that the caller holds a lock on the span?

yurishkuro · 2017-08-06T19:49:25Z

internal/baggage/remote/options.go

+const (
+	defaultMaxValueLength  = 2048
+	defaultRefreshInterval = time.Minute
+	defaultServerURL       = "http://localhost:5778/baggageRestrictions"


I'm not suggesting we change this now, but there are a couple problems with this URL config property that perhaps we should keep in mind:

if the deployment requires accessing the agent via different name than localhost we now have several config variables the user needs to update

while updating those variables, the user needs to know the exact URLs like /baggageRestrictions when all they needed was to update the agent's host name

Maybe we should have a TODO somewhere for the next major release

ill fix in next PR

yurishkuro · 2017-08-06T19:51:26Z

internal/baggage/remote/options.go

+var Options options
+
+type options struct {
+	failClosed      bool


shouldn't this follow the same naming pattern as the option? DenyBaggageOnInitializationFailure

yurishkuro · 2017-08-06T19:58:54Z

internal/baggage/remote/restriction_manager.go

+		invalidRestriction: baggage.NewRestriction(false, 0),
+		validRestriction:   baggage.NewRestriction(true, defaultMaxValueLength),
+	}
+	if err := m.updateRestrictions(); err != nil {


this call happens on the critical path of application startup. What kind of timeout do we have for it?

ill fix in next PR

yurishkuro · 2017-08-06T20:02:44Z

internal/baggage/remote/restriction_manager.go

+	return nil
+}
+
+func (m *RestrictionManager) generateRestrictions(restrictions []*thrift.BaggageRestriction) map[string]*baggage.Restriction {


this function is not "generating" restrictions. "parse" or "convert".

yurishkuro · 2017-08-06T20:10:22Z

tracer.go

@@ -283,6 +287,9 @@ func (t *Tracer) Extract(
 func (t *Tracer) Close() error {
 	t.reporter.Close()
 	t.sampler.Close()
+	if mgr, ok := t.baggageRestrictionManager.(io.Closer); ok {


note that your remote manager is not an io.Closer : func (m *RestrictionManager) Close() { (no error returned)

Should add var _ io.Closer = new(RemoteBaggageRestrictionManager) in its test file.

coveralls · 2017-08-07T13:58:10Z

Coverage increased (+0.2%) to 85.578% when pulling 5cc274e on Add_RemoteBaggageRestrictionManager into 08628f9 on master.

coveralls · 2017-08-07T14:01:18Z

Coverage increased (+0.2%) to 85.578% when pulling 7ed2a48 on Add_RemoteBaggageRestrictionManager into 08628f9 on master.

black-adder added 3 commits August 1, 2017 19:18

Add remoteBaggageRestrictionManager

2758681

imports

5c59f48

actually initialize it

28607fc

black-adder commented Aug 1, 2017

View reviewed changes

black-adder mentioned this pull request Aug 2, 2017

[WIP] baggage manager new API #183

Closed

black-adder added 2 commits August 2, 2017 18:34

point of no return

4e37e2d

heyo

17e0565

black-adder changed the title ~~Add remoteBaggageRestrictionManager~~ [WIP] Add remoteBaggageRestrictionManager Aug 2, 2017

black-adder commented Aug 2, 2017

View reviewed changes

oibe reviewed Aug 3, 2017

View reviewed changes

yurishkuro reviewed Aug 3, 2017

View reviewed changes

add tests again

ecc0680

black-adder changed the title ~~[WIP] Add remoteBaggageRestrictionManager~~ Add remoteBaggageRestrictionManager Aug 4, 2017

code coverage

fe802ec

yurishkuro reviewed Aug 5, 2017

View reviewed changes

yurishkuro approved these changes Aug 6, 2017

View reviewed changes

black-adder added 2 commits August 7, 2017 09:51

address review

5cc274e

hang my head in shame

7ed2a48

black-adder merged commit c8f7cd8 into master Aug 7, 2017

black-adder deleted the Add_RemoteBaggageRestrictionManager branch August 7, 2017 14:05

black-adder mentioned this pull request Aug 7, 2017

Clean up baggage restriction manager #186

Merged

yurishkuro mentioned this pull request Nov 14, 2017

Propose release for version 2.10.0 #226

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add remoteBaggageRestrictionManager #182

Add remoteBaggageRestrictionManager #182

black-adder commented Aug 1, 2017 •

edited

Loading

black-adder Aug 1, 2017

yurishkuro Aug 2, 2017

coveralls commented Aug 1, 2017

yurishkuro commented Aug 2, 2017

black-adder commented Aug 2, 2017 •

edited

Loading

yurishkuro commented Aug 2, 2017

black-adder commented Aug 2, 2017

yurishkuro commented Aug 2, 2017

black-adder commented Aug 2, 2017 •

edited

Loading

black-adder Aug 2, 2017

black-adder Aug 2, 2017

black-adder Aug 2, 2017

black-adder Aug 2, 2017

coveralls commented Aug 2, 2017

oibe Aug 3, 2017

black-adder Aug 3, 2017

yurishkuro left a comment

yurishkuro Aug 3, 2017

yurishkuro Aug 3, 2017

black-adder commented Aug 3, 2017

coveralls commented Aug 4, 2017

coveralls commented Aug 4, 2017

yurishkuro Aug 5, 2017

yurishkuro left a comment

yurishkuro Aug 6, 2017

yurishkuro Aug 6, 2017

black-adder Aug 7, 2017

yurishkuro Aug 6, 2017

yurishkuro Aug 6, 2017

black-adder Aug 7, 2017

yurishkuro Aug 6, 2017

yurishkuro Aug 6, 2017

coveralls commented Aug 7, 2017

coveralls commented Aug 7, 2017

		@@ -0,0 +1,216 @@
		// Copyright (c) 2017 Uber Technologies, Inc.

Add remoteBaggageRestrictionManager #182

Add remoteBaggageRestrictionManager #182

Conversation

black-adder commented Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 1, 2017

yurishkuro commented Aug 2, 2017

black-adder commented Aug 2, 2017 • edited Loading

yurishkuro commented Aug 2, 2017

black-adder commented Aug 2, 2017

yurishkuro commented Aug 2, 2017

black-adder commented Aug 2, 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

coveralls commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a 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 3, 2017

coveralls commented Aug 4, 2017

coveralls commented Aug 4, 2017

Choose a reason for hiding this comment

yurishkuro 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

coveralls commented Aug 7, 2017

coveralls commented Aug 7, 2017

black-adder commented Aug 1, 2017 •

edited

Loading

black-adder commented Aug 2, 2017 •

edited

Loading

black-adder commented Aug 2, 2017 •

edited

Loading