use another

src/_flow/baggage_restriction_manager.js

@@ -19,14 +19,14 @@
 // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 // THE SOFTWARE.
 
-import BaggageSetter from '../baggage/baggage_setter.js'
+import Restriction from '../baggage/restriction.js'
 
 /**
  * BaggageRestrictionManager is an interface for a class that manages baggage
- * restrictions for baggage keys. The manager will return a BaggageSetter
- * for a specific baggage key which will set the baggage on the Span
- * given the baggage restrictions for that key.
+ * restrictions for baggage keys. The manager will return a Restriction
+ * for a specific baggage key which will determine whether the baggage key is
+ * allowed and any other applicable restrictions on the baggage value.
  */
 declare interface BaggageRestrictionManager {
-    getBaggageSetter(key: string): BaggageSetter;
+    getRestriction(key: string): Restriction;
 }

src/baggage/baggage_setter.js

@@ -24,55 +24,48 @@ import SpanContext from '../span_context.js';
 import Metrics from '../metrics/metrics.js'
 
 /**
- * BaggageSetter is a class that sets a valid baggage key:value and the associated
+ * BaggageSetter is a class that sets a baggage key:value and the associated
  * logs on a Span.
  */
 export default class BaggageSetter {
-    /**
-     * This flag represents whether the key is a valid baggage key. If valid
-     * the baggage key:value will be written to the span.
-     */
-    _valid: boolean;
-    _maxValueLength: number;
+    _restrictionManager: BaggageRestrictionManager;
     _metrics: Metrics;
 
-    constructor(valid: boolean, maxValueLength: number, metrics: Metrics) {
-        this._valid = valid;
-        this._maxValueLength = maxValueLength;
+    constructor(restrictionManager: BaggageRestrictionManager, metrics: Metrics) {
+        this._restrictionManager = restrictionManager;
         this._metrics = metrics;
     }
 
     /**
      * Sets the baggage key:value on the span and the corresponding logs.
-     * Whether the baggage is set on the span depends on if the key
-     * is valid.
-     * A SpanContext is returned with the new baggage key:value set
-     * if key is valid, else returns the existing SpanContext on the Span.
+     * A SpanContext is returned with the new baggage key:value set.
      *
      * @param {Span} span - The span to set the baggage on.
      * @param {string} key - The baggage key to set.
      * @param {string} value - The baggage value to set.
-     * @return {SpanContext} - The SpanContext with the baggage set.
+     * @return {SpanContext} - The SpanContext with the baggage set if applicable.
      */
     setBaggage(span: Span, key: string, value: string): SpanContext {
         let truncated = false;
-        let prevItem = span.getBaggageItem(key);
-        if (!this._valid) {
+        let prevItem = '';
+        let restriction = this._restrictionManager.getRestriction(key);
+        if (!restriction.keyAllowed) {
+            this._logFields(span, key, value, prevItem, truncated, restriction.keyAllowed);
             this._metrics.baggageUpdateFailure.increment(1);
-            this._logFields(span, key, value, prevItem, truncated);
             return span.context();
         }
-        if (value.length > this._maxValueLength) {
+        if (value.length > restriction.maxValueLength) {
             truncated = true;
-            value = value.substring(0, this._maxValueLength);
+            value = value.substring(0, restriction.maxValueLength);
             this._metrics.baggageTruncate.increment(1);
         }
-        this._logFields(span, key, value, prevItem, truncated);
+        prevItem = span.getBaggageItem(key);
+        this._logFields(span, key, value, prevItem, truncated, restriction.keyAllowed);
         this._metrics.baggageUpdateSuccess.increment(1);
         return span.context().withBaggageItem(key, value);
     }
 
-    _logFields(span: Span, key: string, value: string, prevItem: string, truncated: boolean) {
+    _logFields(span: Span, key: string, value: string, prevItem: string, truncated: boolean, valid: boolean) {
         if (!span.context().isSampled()) {
             return
         }
@@ -87,7 +80,7 @@ export default class BaggageSetter {
         if (truncated) {
             fields['truncated'] = 'true';
         }
-        if (!this._valid) {
+        if (!valid) {
             fields['invalid'] = 'true';
         }
         span.log(fields);

src/baggage/default_baggage_restriction_manager.js

@@ -19,24 +19,22 @@
 // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 // THE SOFTWARE.
 
-import BaggageSetter from '../baggage/baggage_setter.js'
-import Metrics from '../metrics/metrics.js'
+import Restriction from './restriction.js'
 
 export const DEFAULT_MAX_VALUE_LENGTH = 2048;
 
 /**
  * Creates a BaggageRestrictionManager that allows any baggage key.
  */
 export default class DefaultBaggageRestrictionManager {
-    _baggageSetter: BaggageSetter;
+    _restriction: Restriction;
 
-    constructor(metrics: Metrics, maxValueLength: ?number) {
+    constructor(maxValueLength: ?number) {
         maxValueLength = maxValueLength || DEFAULT_MAX_VALUE_LENGTH;
-        this._baggageSetter = new BaggageSetter(true, maxValueLength, metrics);
+        this._restriction = new Restriction(true, maxValueLength);
     }
 
-    getBaggageSetter(key: string): BaggageSetter {
-        return this._baggageSetter;
+    getRestriction(key: string): Restriction {
+        return this._restriction;
     }
-
 }

src/baggage/restriction.js

@@ -0,0 +1,42 @@
+// @flow
+// Copyright (c) 2017 Uber Technologies, Inc.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+/**
+ * Restriction determines whether a baggage key is allowed and contains any
+ * restrictions on the baggage value.
+ */
+export default class Restriction {
+    _keyAllowed: boolean;
+    _maxValueLength: number;
+
+    constructor(keyAllowed: boolean, maxValueLength: number) {
+        this._keyAllowed = keyAllowed;
+        this._maxValueLength = maxValueLength;
+    }
+
+    get keyAllowed(): boolean {
+        return this._keyAllowed;
+    }
+
+    get maxValueLength(): number {
+        return this._maxValueLength;
+    }
+}

src/span.js

@@ -20,10 +20,10 @@
 // THE SOFTWARE.
 
 import * as constants from './constants.js';
-import NullLogger from './logger.js';
 import SpanContext from './span_context.js';
 import * as opentracing from 'opentracing';
 import Utils from './util.js';
+import BaggageSetter from './baggage/baggage_setter';
 
 export default class Span {
     _tracer: any;
@@ -36,6 +36,7 @@ export default class Span {
     _tags: Array<Tag>;
     static _baggageHeaderCache: any;
     _references: Array<Reference>;
+    _baggageSetter: BaggageSetter;
 
     constructor(tracer: any,
                 operationName: string,
@@ -49,6 +50,7 @@ export default class Span {
         this._startTime = startTime;
         this._logger = tracer._logger;
         this._references = references;
+        this._baggageSetter = tracer._baggageSetter;
         this._logs = [];
         this._tags = [];
     }
@@ -96,15 +98,14 @@ export default class Span {
      **/
     setBaggageItem(key: string, value: string): Span {
         let normalizedKey = this._normalizeBaggageKey(key);
-        let setter = this._tracer.baggageRestrictionManager.getBaggageSetter(normalizedKey);
 
         // We create a new instance of the context here instead of just adding
         // another entry to the baggage dictionary. By doing so we keep the
         // baggage immutable so that it can be passed to children spans as is.
         // If it was mutable, we would have to make a copy of the dictionary
         // for every child span, which on average we expect to occur more
         // frequently than items being added to the baggage.
-        this._spanContext = setter.setBaggage(this, normalizedKey, value);
+        this._spanContext = this._baggageSetter.setBaggage(this, normalizedKey, value);
         return this;
     }
 

src/tracer.js

@@ -35,6 +35,7 @@ import Metrics from './metrics/metrics';
 import NoopMetricFactory from './metrics/noop/metric_factory';
 import DefaultBaggageRestrictionManager from './baggage/default_baggage_restriction_manager';
 import os from 'os';
+import BaggageSetter from './baggage/baggage_setter';
 
 export default class Tracer {
     _serviceName: string;
@@ -45,7 +46,7 @@ export default class Tracer {
     _injectors: any;
     _extractors: any;
     _metrics: any;
-    _baggageRestrictionManager: BaggageRestrictionManager;
+    _baggageSetter: BaggageSetter;
 
     /**
      * @param {String} [serviceName] - name of the current service or application.
@@ -74,8 +75,11 @@ export default class Tracer {
         this._reporter = reporter;
         this._sampler = sampler;
         this._logger = options.logger || new NullLogger();
-        this._baggageRestrictionManager = options.baggageRestrictionManager ||
-            new DefaultBaggageRestrictionManager(this._metrics);
+        if (options.baggageRestrictionManager) {
+            this._baggageSetter  = new BaggageSetter(options.baggageRestrictionManager, this._metrics);
+        } else {
+            this._baggageSetter  = new BaggageSetter(new DefaultBaggageRestrictionManager(), this._metrics);
+        }
         this._injectors = {};
         this._extractors = {};
 
@@ -100,10 +104,6 @@ export default class Tracer {
         this._reporter.setProcess(this._serviceName, Utils.convertObjectToTags(this._tags));
     }
 
-    get baggageRestrictionManager(): BaggageRestrictionManager {
-        return this._baggageRestrictionManager;
-    }
-
     _startInternalSpan(
         spanContext: SpanContext,
         operationName: string,

test/baggage/baggage_setter.js

@@ -26,6 +26,9 @@ import BaggageSetter from "../../src/baggage/baggage_setter.js";
 import Tracer from '../../src/tracer.js';
 import InMemoryReporter from '../../src/reporters/in_memory_reporter.js';
 import ConstSampler from '../../src/samplers/const_sampler.js';
+import DefaultBaggageRestrictionManager from "../../src/baggage/default_baggage_restriction_manager.js";
+import Restriction from "../../src/baggage/restriction";
+import sinon from 'sinon';
 
 describe('BaggageSetter should', () => {
     let metrics: Metrics;
@@ -59,7 +62,7 @@ describe('BaggageSetter should', () => {
             reporter,
             new ConstSampler(true),
             {
-                options: metrics,
+                metrics: metrics,
             }
         );
 
@@ -71,7 +74,11 @@ describe('BaggageSetter should', () => {
     });
 
     it ('fail for invalid baggage key', (done) => {
-        let setter = new BaggageSetter(false, 10, metrics);
+        let mgr = new DefaultBaggageRestrictionManager();
+        let stub = sinon.stub(mgr, 'getRestriction', function(key) {
+            return new Restriction(false, 0);
+        });
+        let setter = new BaggageSetter(mgr, metrics);
         let key = "key";
         let value = "value";
         let ctx = setter.setBaggage(span, key, value);
@@ -82,7 +89,7 @@ describe('BaggageSetter should', () => {
     });
 
     it ('succeed for valid baggage key', (done) => {
-        let setter = new BaggageSetter(true, 5, metrics);
+        let setter = new BaggageSetter(new DefaultBaggageRestrictionManager(5), metrics);
         let key = "key";
         let value = "0123456789";
         let expected = "01234";
@@ -96,4 +103,26 @@ describe('BaggageSetter should', () => {
         assert.equal(LocalBackend.counterValue(metrics.baggageTruncate), 1);
         done();
     });
+
+    it ('not set logs if span is not sampled', (done) => {
+        let mgr = new DefaultBaggageRestrictionManager();
+        tracer = new Tracer(
+            'test-service-name',
+            reporter,
+            new ConstSampler(false),
+            {
+                metrics: metrics,
+                baggageRestrictionManager: mgr,
+            }
+        );
+        span = tracer.startSpan('op-name');
+
+        let setter = new BaggageSetter(mgr, metrics);
+        let key = "key";
+        let value = "0123456789";
+        let ctx = setter.setBaggage(span, key, value);
+        assert.equal(ctx._baggage[key], value);
+        assert.lengthOf(span._logs, 0);
+        done();
+    });
 });

test/baggage/default_baggage_restriction_manager.js

@@ -19,17 +19,14 @@
 // THE SOFTWARE.
 
 import {assert} from 'chai';
-import Metrics from '../../src/metrics/metrics.js';
-import BaggageSetter from "../../src/baggage/baggage_setter.js";
 import DefaultBaggageRestrictionManager from "../../src/baggage/default_baggage_restriction_manager";
-import NoopMetricFactory from "../../src/metrics/noop/metric_factory";
+import Restriction from "../../src/baggage/restriction";
 
 describe('DefaultBaggageRestrictionManager should', () => {
 
-    it ('return a baggageSetter', (done) => {
-        let metrics = new Metrics(new NoopMetricFactory());
-        let mgr = new DefaultBaggageRestrictionManager(metrics);
-        assert.deepEqual(mgr.getBaggageSetter("key"), new BaggageSetter(true, 2048, metrics));
+    it ('return a Restriction', (done) => {
+        let mgr = new DefaultBaggageRestrictionManager();
+        assert.deepEqual(mgr.getRestriction("key"), new Restriction(true, 2048));
         done();
     });
 });

test/span.js

@@ -33,7 +33,6 @@ import sinon from 'sinon';
 import * as thrift from '../src/thrift.js';
 import Tracer from '../src/tracer.js';
 import Utils from '../src/util.js';
-import BaggageSetter from "../src/baggage/baggage_setter";
 
 describe('span should', () => {
     let reporter = new InMemoryReporter();
@@ -165,12 +164,11 @@ describe('span should', () => {
         let key = 'some-key';
         let value = 'some-value';
 
-        let spy = sinon.spy(span._tracer.baggageRestrictionManager, 'getBaggageSetter');
+        let spy = sinon.spy(span._baggageSetter, 'setBaggage');
         span.setBaggageItem(key, value);
         assert.equal(value, span.getBaggageItem(key));
         assert(spy.calledOnce);
-        assert(spy.calledWith('some-key'));
-        assert(spy.returned(new BaggageSetter(true, 2048, tracer._metrics)));
+        assert(spy.calledWith(span, key, value));
     });
 
     it ('inherit baggage from parent', () => {