Do not strip leading zeros #398
Conversation
Fixes jaegertracing#391 Signed-off-by: Aleksei Androsov <[email protected]>
| 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.
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.
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.
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'); | |||
|
Any updates on this? |
* fix(plugin-http): ensure no leaks closes jaegertracing#397 Signed-off-by: Olivier Albertini <[email protected]> * fix: add @Flarna recommandations Signed-off-by: Olivier Albertini <[email protected]>
Fixes #391