Add BaggageRestrictionManager #217

black-adder · 2017-07-24T15:51:50Z

Subset of this: https://github.com/uber/jaeger-client-java/pull/210/files. Only adding the BaggageRestrictionManager abstract class and the DefaultBaggageRestrictionManager.

Tests

codecov-io · 2017-07-24T15:58:33Z

Codecov Report

Merging #217 into master will decrease coverage by 0.13%.
The diff coverage is 94.33%.

@@             Coverage Diff              @@
##             master     #217      +/-   ##
============================================
- Coverage     81.53%   81.39%   -0.14%     
- Complexity      491      504      +13     
============================================
  Files            79       82       +3     
  Lines          1895     1935      +40     
  Branches        229      232       +3     
============================================
+ Hits           1545     1575      +30     
- Misses          253      261       +8     
- Partials         97       99       +2

Impacted Files	Coverage Δ	Complexity Δ
...src/main/java/com/uber/jaeger/metrics/Metrics.java	`77.5% <ø> (ø)`	`11 <0> (ø)`	⬇️
...eger/baggage/DefaultBaggageRestrictionManager.java	`100% <100%> (ø)`	`3 <3> (?)`
...in/java/com/uber/jaeger/baggage/BaggageSetter.java	`100% <100%> (ø)`	`9 <9> (?)`
...ger-core/src/main/java/com/uber/jaeger/Tracer.java	`85.52% <100%> (+0.32%)`	`18 <2> (+1)`	⬆️
...aeger-core/src/main/java/com/uber/jaeger/Span.java	`80.8% <50%> (-4.1%)`	`37 <1> (-2)`
...main/java/com/uber/jaeger/baggage/Restriction.java	`66.66% <66.66%> (ø)`	`2 <2> (?)`
...ain/java/com/uber/jaeger/senders/ThriftSender.java	`73.46% <0%> (-4.09%)`	`10% <0%> (ø)`
...jaeger/reporters/protocols/ThriftUdpTransport.java	`77.96% <0%> (-3.39%)`	`14% <0%> (ø)`
...java/com/uber/jaeger/reporters/RemoteReporter.java	`87.14% <0%> (-2.86%)`	`9% <0%> (ø)`
... and 2 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 ec6ac78...877120b. Read the comment docs.

yurishkuro

Have you given any thought to my comment about minimizing memory allocations? For example, even if you need multi-dimensional answer from baggage manager, you can achieve that without returning a value object, by using a strategy pattern, i.e. returning an actor that "does something with key/value", which could be no-op & metrics, set & metrics, truncate & set & metrics. All those actors can be pre-created by the manager once.

yurishkuro · 2017-07-24T17:06:36Z

jaeger-core/src/main/java/com/uber/jaeger/Span.java

+      }
+      SanitizedBaggage baggage = this.tracer.getBaggageRestrictionManager().sanitizeBaggage(key, value);
+      if (!baggage.isValid()) {
+        this.tracer.getMetrics().baggageUpdateFailure.inc(1);


might be useful to log this to span as well

black-adder · 2017-07-24T19:02:59Z

I attempted the strategy pattern approach and it's a hacky mess. If you're concerned about minimizing memory allocations, maybe I can go back to my first ever approach where I only return (isValid, maxValueLength) which are created once (a minute) and cached.

black-adder · 2017-07-24T20:14:20Z

jaeger-core/src/main/java/com/uber/jaeger/baggage/BaggageValidity.java

+
+    String prevItem = span.getBaggageItem(key);
+    this.logFields(span, key, value, prevItem, truncated, false);
+    SpanContext context = span.context().withBaggageItem(key, value);


I have to create the new context and pass it back given the setter isn't public. Also the BaggageValidity class reeks of feature envy. I could move this file down to the same namespace as Span and make the context setter protected or I could remove the functionality in this class completely and inline it in Span.setBaggageItem()

yurishkuro · 2017-07-24T20:27:50Z

jaeger-core/src/main/java/com/uber/jaeger/Span.java

      }
+      BaggageValidity baggageValidity = this.getTracer().getBaggageRestrictionManager().isBaggageValid(key, value);


how about

s/BaggageValidity/BaggageRestriction/

s/isBaggageValid(key, value)/getBaggageRestriction(key)/

s/sanitizeBaggage/setBaggage/

move the call to getBaggageRestriction outside of synchronized since it's not dependent on the span

or s/BaggageValidity/BaggageSetter/ (back to "strategy" pattern)

vprithvi · 2017-07-24T20:43:44Z

jaeger-core/src/main/java/com/uber/jaeger/baggage/BaggageValidity.java

+  final int maxValueLength;
+
+  public SpanContext sanitizeBaggage(Span span, String key, String value) {
+    Metrics metrics = span.getTracer().getMetrics();


I'd initialize the metrics object by accepting a Metrics when creating an instance of the class. There is no necessity to retrieve it once per method call.

See also: Law of Demeter

vprithvi · 2017-07-24T20:52:52Z

jaeger-core/src/main/java/com/uber/jaeger/Span.java

-          fields.put("override", "true");
-        }
-        return this.log(fields);
+      if (key == null || value == null) {


This test doesn't need to be synchronized.

vprithvi · 2017-07-25T15:07:18Z

jaeger-core/src/main/java/com/uber/jaeger/Tracer.java

    private String serviceName;
    private Clock clock = new SystemClock();
    private Map<String, Object> tags = new HashMap<String, Object>();
    private boolean zipkinSharedRpcSpan;
    private ActiveSpanSource activeSpanSource = new ThreadLocalActiveSpanSource();
-    private BaggageRestrictionManager baggageRestrictionManager = new DefaultBaggageRestrictionManager();
+    private BaggageRestrictionManager baggageRestrictionManager =
+        new DefaultBaggageRestrictionManager(this.metrics);


This introduces a bug; the instance of metrics used by DefaultBaggageRestrictionManager would always be the one set in L453.

vprithvi · 2017-07-25T16:12:10Z

jaeger-core/src/main/java/com/uber/jaeger/Tracer.java

@@ -499,6 +508,9 @@ public Builder withZipkinSharedRpcSpan() {

    public Builder withMetrics(Metrics metrics) {
      this.metrics = metrics;
+      if (this.baggageRestrictionManager instanceof DefaultBaggageRestrictionManager) {
+        this.baggageRestrictionManager = new DefaultBaggageRestrictionManager(this.metrics);
+      }


This is bad because withMetrics now has multiple responsibilities.
It is also incorrect in that it misses the case where metrics are initialized withStatsReporter

A better way to handle this is to leave baggageRestrictionManager uninitialized, and initialize it in build with the right metrics instance.

vprithvi · 2017-07-26T15:01:37Z

jaeger-core/src/main/java/com/uber/jaeger/baggage/BaggageSetter.java

+    return context;
+  }
+
+  void logFields(Span span, String key, String value, String prevItem, boolean truncated, boolean invalid) {


This should be private.

vprithvi · 2017-07-26T15:08:00Z

jaeger-core/src/main/java/com/uber/jaeger/baggage/BaggageSetter.java

+  final Metrics metrics;
+
+  public SpanContext setBaggage(Span span, String key, String value) {
+    if (!this.isValid()) {


It's simpler to check the boolean directly instead of using the getter

vprithvi · 2017-07-26T15:08:24Z

jaeger-core/src/main/java/com/uber/jaeger/baggage/BaggageSetter.java

+  public SpanContext setBaggage(Span span, String key, String value) {
+    if (!this.isValid()) {
+      metrics.baggageUpdateFailure.inc(1);
+      this.logFields(span, key, value, null, false, true);


There is no need to use this here

vprithvi · 2017-07-26T15:09:44Z

jaeger-core/src/main/java/com/uber/jaeger/baggage/BaggageRestrictionManager.java

+
+package com.uber.jaeger.baggage;
+
+public abstract class BaggageRestrictionManager {


Could you add javadocs on what this is?

vprithvi · 2017-07-26T15:10:56Z

jaeger-core/src/main/java/com/uber/jaeger/baggage/BaggageSetter.java

+  final int maxValueLength;
+  final Metrics metrics;
+
+  public SpanContext setBaggage(Span span, String key, String value) {


Add javadoc, simply based on the method name, I'd conclude that it simply sets baggage.

vprithvi · 2017-07-26T15:13:16Z

jaeger-core/src/main/java/com/uber/jaeger/baggage/BaggageRestrictionManager.java

+package com.uber.jaeger.baggage;
+
+public abstract class BaggageRestrictionManager {
+  static final int DEFAULT_MAX_VALUE_LENGTH = 2048;


This is only used in the DefaultBaggageRestrictionManager. Do you see this being made use of elsewhere? Perhaps it can be moved to DefaultBaggageRestrictionManager

it'll be used in another manager in the follow up PR

vprithvi · 2017-07-26T15:20:00Z

jaeger-core/src/main/java/com/uber/jaeger/baggage/DefaultBaggageRestrictionManager.java

+  }
+
+  @Override
+  public BaggageSetter getBaggageSetter(String key) {


Do we need this?
Can't we use lombok's @Getter annotation?

getBaggageSetter is an interface function

discussed this offline: my suggestion doesn't work because the getter requires a key

vprithvi · 2017-07-26T15:27:26Z

jaeger-core/src/test/java/com/uber/jaeger/baggage/BaggageSetterTest.java

+        1L, metricsReporter.counters.get("jaeger.baggage-update.result=ok").longValue());
+  }
+
+  private void assertBaggageLogs(


It looks like testing concerns aren't separated between SpanTest and BaggageSetter.

Ideally, we would only do the following in SpanTest
Assert that span.setBaggageItem invokes BaggageSetter with the right arguments.
Assert that span.getBaggeItem can retrieve baggage that has been set

All of the detailed test cases that only involve setting baggage should be under the purview of BaggageSetterTest. This results in a clear separation of concerns and code deduplication.

yurishkuro · 2017-07-27T15:18:42Z

jaeger-core/src/main/java/com/uber/jaeger/baggage/BaggageSetter.java

+public class BaggageSetter {
+  final boolean valid;
+  final int maxValueLength;
+  final Metrics metrics;


to make fields visible to subclasses, they should be protected. Did you also need package visibility?

given that these fields are final, there should be a constructor setting them. If they are only set by subclasses, then the constructor can be protected (but then this class should be abstract).

the concept of valid is not clear, please document

@value(staticConstructor = "of") generates a private constructor and a public factory for this class.

yurishkuro · 2017-07-27T15:19:10Z

jaeger-core/src/main/java/com/uber/jaeger/baggage/BaggageSetter.java

+    return context;
+  }
+
+  private void logFields(Span span, String key, String value, String prevItem, boolean truncated, boolean invalid) {


avoid negatives - s/invalid/valid/

yurishkuro · 2017-07-27T15:20:18Z

jaeger-core/src/main/java/com/uber/jaeger/baggage/BaggageSetter.java

+  public SpanContext setBaggage(Span span, String key, String value) {
+    if (!valid) {
+      metrics.baggageUpdateFailure.inc(1);
+      logFields(span, key, value, null, false, true);


if you move prevItem and truncated declarations to the top, this could be made a lot more readable: logFields(span, key, value, prevValue, truncated, valid);

yurishkuro · 2017-07-27T15:24:15Z

jaeger-core/src/main/java/com/uber/jaeger/baggage/BaggageSetter.java

+        fields.put("truncated", "true");
+      }
+      if (invalid) {
+        fields.put("invalid", "true");


why are you logging these as strings instead of booleans?

oibe · 2017-07-27T20:19:33Z

jaeger-core/src/main/java/com/uber/jaeger/baggage/BaggageSetter.java

+   * This flag represents whether the key is a valid baggage key. If valid
+   * the baggage key:value will be written to the span.
+   */
+  private final boolean valid;


If something is not a valid baggage key then why create a BaggageSetter for it?

I assume it would create a Noop setter. This way the validity does not need to be exposed outside of baggage manager into the Span

yurishkuro · 2017-07-27T21:53:29Z

jaeger-core/src/main/java/com/uber/jaeger/baggage/DefaultBaggageRestrictionManager.java

+
+  @Override
+  public BaggageSetter getBaggageSetter(String key) {
+    BaggageSetter setter = baggageSetters.get(key);


unsynchronized map

…e SpanSetter since this wont be able to scale with the number of random baggage people can set

black-adder · 2017-08-08T21:19:35Z

Rewrote to fit into new API.

yurishkuro

not clear how this will support per-service restrictions

yurishkuro · 2017-08-10T17:36:20Z

jaeger-core/src/main/java/com/uber/jaeger/baggage/BaggageSetter.java

+  /**
+   * Sets the baggage key:value on the {@link Span} and the corresponding
+   * logs. Whether the baggage is set on the span depends on if the key
+   * is valid.


s/is valid/is allowed to be set by this service/

yurishkuro · 2017-08-10T17:36:41Z

jaeger-core/src/main/java/com/uber/jaeger/Span.java

-        }
-        return this.log(fields);
-      }
+      this.context = getTracer().setBaggage(this, key, value);


why getTracer instead of just tracer?

yurishkuro · 2017-08-10T17:37:13Z

jaeger-core/src/main/java/com/uber/jaeger/baggage/BaggageRestrictionManager.java

+ * BaggageRestrictionManager is an interface for a class that manages baggage
+ * restrictions for baggage keys. The manager will return a {@link Restriction}
+ * for a specific baggage key which will determine whether the baggage key is
+ * allowed and any other applicable restrictions on the baggage value.


s/is allowed/is allowed for the current service/

yurishkuro · 2017-08-10T17:37:31Z

jaeger-core/src/main/java/com/uber/jaeger/baggage/BaggageRestrictionManager.java

+public interface BaggageRestrictionManager {
+  int DEFAULT_MAX_VALUE_LENGTH = 2048;
+
+  Restriction getRestriction(String key);


shouldn't we pass the service name?

black-adder · 2017-08-10T17:42:39Z

I'm not sure how the whole client will support the per service use case. Maybe we should have a plan for that before making the baggagerestrictionmanager battle ready?

yurishkuro · 2017-08-10T17:45:51Z

the service name will be associated with the span, not the tracer. But I see how in this version you filter on the service name at the time of querying the backend, so that's ok for now. But it will be an API change in the future, so it might make sense to just pass the service name from the tracer to baggage restriction manager, even if it ignores it for now.

black-adder · 2017-08-10T18:48:40Z

just pass the service name from the tracer to baggage restriction manager (BRM)

I'm not sure if that's future proof. My assumption is that in this dystopian future with no rules, people will bring up a tracer without a service name which means we can't initialize the BRM with a service. This means that we have to change getRestriction(key) to getRestriction(service, key) and then have BRM asynchronously fill its cache with restrictions for the service.

Looks like we'd need a batch endpoint to agent to retrieve restrictions for multiple services
First call to getRestriction will always fail (unless we have a failClosed flag)

Add BaggageRestrictionManager

ea69588

yurishkuro reviewed Jul 24, 2017

View reviewed changes

feature envy

6379066

black-adder commented Jul 24, 2017

View reviewed changes

yurishkuro reviewed Jul 24, 2017

View reviewed changes

vprithvi reviewed Jul 24, 2017

View reviewed changes

black-adder added 2 commits July 24, 2017 23:27

comments

e29d837

too many spaces

11c6d86

vprithvi reviewed Jul 25, 2017

View reviewed changes

meh

f113e55

vprithvi reviewed Jul 25, 2017

View reviewed changes

black-adder added 2 commits July 25, 2017 12:46

wow

df9b653

fix variable name

4af5607

vprithvi reviewed Jul 26, 2017

View reviewed changes

private

b644bb6

vprithvi suggested changes Jul 26, 2017

View reviewed changes

oops

72c1b84

black-adder force-pushed the Add_BaggageRestrictionManager branch from af37bfb to 72c1b84 Compare July 26, 2017 18:28

yurishkuro reviewed Jul 27, 2017

View reviewed changes

black-adder added 2 commits July 27, 2017 13:47

make setter more reasonable

b24d7de

lil clean up

4d5d98b

vprithvi approved these changes Jul 27, 2017

View reviewed changes

oibe reviewed Jul 27, 2017

View reviewed changes

yurishkuro reviewed Jul 27, 2017

View reviewed changes

revert some of the previous changes that added the key directly to th…

495a656

…e SpanSetter since this wont be able to scale with the number of random baggage people can set

black-adder force-pushed the Add_BaggageRestrictionManager branch from 2aeba8f to 495a656 Compare July 28, 2017 14:50

new API

72cbdc5

black-adder force-pushed the Add_BaggageRestrictionManager branch from fdfa73f to 72cbdc5 Compare August 8, 2017 21:13

yurishkuro reviewed Aug 10, 2017

View reviewed changes

address comments

45d2b58

yurishkuro approved these changes Aug 10, 2017

View reviewed changes

Merge branch 'master' into Add_BaggageRestrictionManager

877120b

black-adder merged commit a44f07f into jaegertracing:master Aug 10, 2017

black-adder deleted the Add_BaggageRestrictionManager branch August 10, 2017 21:13

pavolloffay mentioned this pull request May 10, 2019

Automatically generate changelog #629

Closed

		}
		BaggageValidity baggageValidity = this.getTracer().getBaggageRestrictionManager().isBaggageValid(key, value);


		package com.uber.jaeger.baggage;

		public abstract class BaggageRestrictionManager {

Add BaggageRestrictionManager #217

Add BaggageRestrictionManager #217

Conversation

black-adder commented Jul 24, 2017 • edited Loading

codecov-io commented Jul 24, 2017 • edited Loading

Codecov Report

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

black-adder commented Jul 24, 2017

Choose a reason for hiding this comment

yurishkuro Jul 24, 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

Choose a reason for hiding this comment

vprithvi Jul 25, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

black-adder commented Aug 8, 2017

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

black-adder commented Aug 10, 2017

yurishkuro commented Aug 10, 2017

black-adder commented Aug 10, 2017

black-adder commented Jul 24, 2017 •

edited

Loading

codecov-io commented Jul 24, 2017 •

edited

Loading

yurishkuro Jul 24, 2017 •

edited

Loading

vprithvi Jul 25, 2017 •

edited

Loading