Add BaggageRestrictionManager and BaggageSetter #142

black-adder · 2017-07-28T21:23:46Z

subset of: #131

coveralls · 2017-07-28T21:40:39Z

Coverage decreased (-0.05%) to 98.76% when pulling c1da07b on baggage_stuffs into 178dddf on master.

oibe · 2017-07-31T17:18:49Z

src/baggage/baggage_setter.js

+    setBaggage(span: Span, key: string, value: string): SpanContext {
+        let truncated = false;
+        let prevItem = span.getBaggageItem(key);
+        if (!this._valid) {


I find this use of this._valid confusing. Where is this._valid set? what makes baggage invalid?

The collectors will have a list of valid baggage keys and their maximum size. The clients will retrieve these restrictions once a minute. Anytime the client writes baggage, it'll be validated against these rules. If invalid, the baggage is logged in the span but is not propagated.

The remote fetching of baggage rules will come in a future PR. Right now, the DefaultBaggageRestrictionManager will return a BaggageSetter that allows all baggage. ie all baggage is valid.

it's because a better way would be to have different implementations for the Setter

oibe · 2017-07-31T17:24:05Z

src/baggage/default_baggage_restriction_manager.js

+
+    constructor(metrics: Metrics, maxValueLength: ?number) {
+        maxValueLength = maxValueLength || DEFAULT_MAX_VALUE_LENGTH;
+        this._baggageSetter = new BaggageSetter(true, maxValueLength, metrics);


nit: Me and you go back and forth on this a lot, but personally I would prefer.
let valid = true;
this._baggageSetter = new BaggageSetter(valid, maxValueLength, metrics);

As of now when I read this code I have to jump to BaggageSetter to know what true means, whereas everything else has a variable name. Anyway really a nit.

oibe · 2017-07-31T17:38:47Z

Some small nits, and also coverage dropped a bit, but otherwise looks good to me.

coveralls · 2017-08-07T17:37:51Z

Coverage decreased (-0.04%) to 98.769% when pulling 2534873 on baggage_stuffs into 178dddf on master.

coveralls · 2017-08-08T15:04:20Z

Coverage increased (+0.04%) to 99.239% when pulling 9958178 on baggage_stuffs into f29839b on master.

oibe · 2017-08-08T21:19:29Z

src/tracer.js

@@ -70,6 +75,11 @@ export default class Tracer {
        this._reporter = reporter;
        this._sampler = sampler;
        this._logger = options.logger || new NullLogger();
+        if (options.baggageRestrictionManager) {


this could be simpler written as:
this._baggageSetter = new BaggageSetter(
options.baggageRestrictionManager || new DefaultBaggageRestrictionManager(),
this._metrics
);

oibe · 2017-08-08T21:22:34Z

test/baggage/baggage_setter.js

+
+    it ('fail for invalid baggage key', (done) => {
+        let mgr = new DefaultBaggageRestrictionManager();
+        let stub = sinon.stub(mgr, 'getRestriction', function(key) {


I don't think you need "let stub", since the variable is never used.

oibe · 2017-08-08T21:25:22Z

test/baggage/baggage_setter.js

+        let setter = new BaggageSetter(mgr, metrics);
+        let key = "key";
+        let value = "value";
+        let ctx = setter.setBaggage(span, key, value);


nit: s/ctx/spanContext/

oibe · 2017-08-08T21:28:57Z

test/baggage/default_baggage_restriction_manager.js

+
+    it ('return a Restriction', (done) => {
+        let mgr = new DefaultBaggageRestrictionManager();
+        assert.deepEqual(mgr.getRestriction("key"), new Restriction(true, 2048));


If the default restrictions change this test will change.
Perhaps: assert.deepEqual(mgr.getRestriction("key"), mgr._restriction);

I believe for things like this we would want it to break because what you're suggesting is tantamount to this.var == this.getVar.

I remember there being a informative Google Testing Blog entry about this but can't find it.

oibe · 2017-08-08T21:29:31Z

test/span.js

@@ -149,7 +139,7 @@ describe('span should', () => {
        assert.equal(span._logs[0].fields[0].value, event);
    });

-    it('add logs with paylaod', () => {
+    it('add logs with paylod', () => {


s/paylod/payload/

oibe · 2017-08-08T21:35:25Z

After you address the comments I above I would say "LGTM"

coveralls · 2017-08-08T22:11:43Z

Coverage increased (+0.03%) to 99.237% when pulling 26a8e71 on baggage_stuffs into f29839b on master.

tiffon

Great work. Had a few questions and suggestions.

tiffon · 2017-08-11T19:37:59Z

src/baggage/baggage_setter.js

+        let prevItem = '';
+        let restriction = this._restrictionManager.getRestriction(key);
+        if (!restriction.keyAllowed) {
+            this._logFields(span, key, value, prevItem, truncated, restriction.keyAllowed);


I don't have much context for this, if the key is not allowed, should the value be logged? I'm just thinking one reason for a restriction (bearing in mind I'm not familiar with the feature) might be to protect sensitive information.

I don't think sensitive info is a concern - we cannot protect against it in case the user calls the general span.log() method, so why worry about protecting here.

tiffon · 2017-08-11T19:47:55Z

src/baggage/baggage_setter.js

+        }
+        if (value.length > restriction.maxValueLength) {
+            truncated = true;
+            value = value.substring(0, restriction.maxValueLength);


It's generally discourage to reassign function parameters, although in this case it's not specifically a problem. As a practice, I generally avoid it.

http://airbnb.io/javascript/#functions--reassign-params
https://spin.atomicobject.com/2011/04/10/javascript-don-t-reassign-your-function-arguments/

seems like a stale comment, since value is a local var now

tiffon · 2017-08-11T19:52:41Z

src/baggage/baggage_setter.js

+        let fields: { [key: string]: string } = {
+            'event': 'baggage',
+            'key': key,
+            'value': value,


Single quotes around 'event', etc., shouldn't be necessary.

tiffon · 2017-08-11T19:53:03Z

src/baggage/baggage_setter.js

+            'value': value,
+        };
+        if (prevItem) {
+            fields['override'] = 'true';


I believe this can be written as fields.override = 'true';

tiffon · 2017-08-11T19:54:38Z

src/baggage/default_baggage_restriction_manager.js

+    _restriction: Restriction;
+
+    constructor(maxValueLength: ?number) {
+        maxValueLength = maxValueLength || DEFAULT_MAX_VALUE_LENGTH;


See suggestion above about reassigning parameters.

tiffon · 2017-08-11T21:15:24Z

test/baggage/baggage_setter.js

+        log.fields.forEach((kv) => {
+           fields[kv.key] = kv.value;
+        });
+        assert.equal(fields['event'], 'baggage');


Just curious, why not use fields.event?

tiffon · 2017-08-11T21:22:27Z

test/baggage/baggage_setter.js

+        tracer.close();
+    });
+
+    it ('fail for invalid baggage key', (done) => {


Re done, is this an async test?

tiffon · 2017-08-11T21:22:56Z

test/baggage/baggage_setter.js

+            new ConstSampler(true),
+            {
+                metrics: metrics,
+            }


With the babel transformations, does { metrics } work?

perhaps, but certainly makes the code more cryptic

tiffon · 2017-08-11T21:26:05Z

test/baggage/baggage_setter.js

+        done();
+    });
+
+    it ('succeed for valid baggage key', (done) => {


Test case description doesn't mention truncation is also being verified.

tiffon · 2017-08-11T21:27:03Z

test/baggage/default_baggage_restriction_manager.js

+    it ('return a Restriction', (done) => {
+        let mgr = new DefaultBaggageRestrictionManager();
+        assert.deepEqual(mgr.getRestriction("key"), new Restriction(true, 2048));
+        done();


See question above about async test case.

coveralls · 2017-08-15T21:08:01Z

Coverage increased (+0.04%) to 99.238% when pulling fda00c4 on baggage_stuffs into f29839b on master.

yurishkuro

lgtm

yurishkuro · 2017-08-18T04:19:30Z

src/_flow/baggage_restriction_manager.js

+ * allowed and any other applicable restrictions on the baggage value.
+ */
+declare interface BaggageRestrictionManager {
+    getRestriction(key: string): Restriction;


do you want to add the service name here, similar to Go client?

yurishkuro · 2017-08-18T04:21:16Z

src/baggage/baggage_setter.js

+        let prevItem = '';
+        let restriction = this._restrictionManager.getRestriction(key);
+        if (!restriction.keyAllowed) {
+            this._logFields(span, key, value, prevItem, truncated, restriction.keyAllowed);


I don't think sensitive info is a concern - we cannot protect against it in case the user calls the general span.log() method, so why worry about protecting here.

yurishkuro · 2017-08-18T04:22:24Z

src/baggage/baggage_setter.js

+        }
+        if (value.length > restriction.maxValueLength) {
+            truncated = true;
+            value = value.substring(0, restriction.maxValueLength);


seems like a stale comment, since value is a local var now

yurishkuro · 2017-08-18T04:24:51Z

src/span.js

@@ -36,6 +36,7 @@ export default class Span {
    _tags: Array<Tag>;
    static _baggageHeaderCache: any;


+1, I did't even know JS had a Map. But I'd leave it out of this diff, it sounds like a global refactoring

yurishkuro · 2017-08-18T04:34:56Z

src/span.js

        // frequently than items being added to the baggage.
-        this._spanContext = this._spanContext.withBaggageItem(normalizedKey, value);
+        this._spanContext = this._baggageSetter.setBaggage(this, normalizedKey, value);


we need to get rid of normalizing baggage keys altogether, but it's a larger change across all clients (https://github.com/uber/jaeger/issues/346)

yurishkuro · 2017-08-18T04:35:52Z

src/tracer.js

@@ -70,6 +75,8 @@ export default class Tracer {
        this._reporter = reporter;
        this._sampler = sampler;
        this._logger = options.logger || new NullLogger();
+        this._baggageSetter  = new BaggageSetter(options.baggageRestrictionManager || new DefaultBaggageRestrictionManager(),


maybe add \n after new BaggageSetter( for better readability

yurishkuro · 2017-08-18T04:36:40Z

test/baggage/baggage_setter.js

+            new ConstSampler(true),
+            {
+                metrics: metrics,
+            }


perhaps, but certainly makes the code more cryptic

coveralls · 2017-08-21T16:24:44Z

Coverage increased (+0.04%) to 99.239% when pulling a8f5a44 on baggage_stuffs into f29839b on master.

oibe reviewed Jul 31, 2017

View reviewed changes

black-adder added 2 commits August 8, 2017 10:33

Add BaggageRestricionManager and BaggageSetter

67d7108

use another

9958178

black-adder force-pushed the baggage_stuffs branch from 2534873 to 9958178 Compare August 8, 2017 14:33

oibe reviewed Aug 8, 2017

View reviewed changes

code review

26a8e71

black-adder requested a review from tiffon August 11, 2017 19:04

tiffon reviewed Aug 11, 2017

View reviewed changes

fixes

fda00c4

yurishkuro approved these changes Aug 18, 2017

View reviewed changes

code review

a8f5a44

black-adder merged commit 5bd0763 into master Aug 21, 2017

yurishkuro deleted the baggage_stuffs branch September 27, 2017 17:34

Iuriy-Budnikov pushed a commit to agile-pm/jaeger-client-node that referenced this pull request Sep 25, 2021

chore: update dependency @types/node (jaegertracing#142)

d332c75

		@@ -36,6 +36,7 @@ export default class Span {
		_tags: Array<Tag>;
		static _baggageHeaderCache: any;

Add BaggageRestrictionManager and BaggageSetter #142

Add BaggageRestrictionManager and BaggageSetter #142

Conversation

black-adder commented Jul 28, 2017

coveralls commented Jul 28, 2017

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

oibe commented Jul 31, 2017

coveralls commented Aug 7, 2017

coveralls commented Aug 8, 2017

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

oibe commented Aug 8, 2017

coveralls commented Aug 8, 2017

tiffon 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

coveralls commented Aug 15, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 21, 2017