Skip to content
This repository was archived by the owner on Sep 13, 2022. It is now read-only.

Do not strip leading zeros#398

Closed
doochik wants to merge 4 commits intojaegertracing:masterfrom
doochik:issue-391
Closed

Do not strip leading zeros#398
doochik wants to merge 4 commits intojaegertracing:masterfrom
doochik:issue-391

Conversation

@doochik
Copy link
Copy Markdown
Contributor

@doochik doochik commented Sep 17, 2019

Fixes #391

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);
Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown

codecov bot commented Sep 29, 2019

Codecov Report

Merging #398 (65e0fce) into master (6a6e3ea) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
src/span_context.js 95.48% <100.00%> (-0.17%) ⬇️
src/util.js 95.23% <100.00%> (+0.11%) ⬆️

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 6a6e3ea...65e0fce. Read the comment docs.


let mycontext = mytracer.extract(opentracing.FORMAT_HTTP_HEADERS, headers);
assert.equal(mycontext.toString(), headers[ck]);
assert.equal(mycontext.toString(), '000000000000000a:000000000000000b:000000000000000c:d');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@doochik doochik closed this Nov 15, 2019
@yurishkuro yurishkuro reopened this Jan 29, 2021
@brandon-leapyear
Copy link
Copy Markdown

Any updates on this?

Iuriy-Budnikov pushed a commit to agile-pm/jaeger-client-node that referenced this pull request Sep 25, 2021
* 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]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Do not strip leading zeros

3 participants