-
Notifications
You must be signed in to change notification settings - Fork 131
Conversation
Fixes jaegertracing#391 Signed-off-by: Aleksei Androsov <doochik@ya.ru>
const safeTraceIdStr = (padding + this._traceIdStr).slice(-traceIdExactLength); | ||
this._traceId = Utils.newBufferFromHex(safeTraceIdStr); | ||
} | ||
this._traceId = Utils.newBufferFromHex(this._traceIdStr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I like this part. We have backwards compatibility requirement to parse strings with stripped zeros (potentially coming from other clients). That means _traceidStr
could be such trimmed ID. If we convert it directly to byte buffer, the buffer could be of random length. I liked the code above that made sure the buffer is always of precisely known length.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean case where I manually set trace.context().traceIdStr = 'abc'
?
In other cases SpanContext.withStringIds()
guarantees valid traceIdStr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. But wouldn't it be better to perform the padding in the constructor, instead of SpanContext.withStringIds()
?
Codecov Report
@@ Coverage Diff @@
## master #398 +/- ##
==========================================
- Coverage 98.72% 98.72% -0.01%
==========================================
Files 50 50
Lines 2035 2032 -3
Branches 383 383
==========================================
- Hits 2009 2006 -3
Misses 26 26
Continue to review full report at Codecov.
|
@@ -51,10 +51,10 @@ describe('tracer should', () => { | |||
}; | |||
|
|||
let mycontext = mytracer.extract(opentracing.FORMAT_HTTP_HEADERS, headers); | |||
assert.equal(mycontext.toString(), headers[ck]); | |||
assert.equal(mycontext.toString(), '000000000000000a:000000000000000b:000000000000000c:d'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Any updates on this? |
* fix(plugin-http): ensure no leaks closes jaegertracing#397 Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca> * fix: add @Flarna recommandations Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
Fixes #391