-
Notifications
You must be signed in to change notification settings - Fork 1.1k
perf(sdk-trace-base): use Uint8Array for browser RandomIdGenerator #6209
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
b3791ae
ee2d94d
d7fe798
be94956
462afae
a0b0a12
b8a4740
ee3f9ea
e04cec4
16ca8c7
57e44ed
359171b
8f9588e
b3ad1c7
57076cf
d7cadc9
65d7875
449ec34
1b2ff44
06e9418
c2d34a4
6f0629b
4c703e2
46a2879
e0649b2
6cb1e9b
ebc8bbe
3499c8f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,36 +16,56 @@ | |
|
|
||
| import { IdGenerator } from '../../IdGenerator'; | ||
|
|
||
| const SPAN_ID_BYTES = 8; | ||
| const TRACE_ID_BYTES = 16; | ||
| const SPAN_ID_BYTES = 8; | ||
|
|
||
| const TRACE_BUFFER = new Uint8Array(TRACE_ID_BYTES); | ||
| const SPAN_BUFFER = new Uint8Array(SPAN_ID_BYTES); | ||
|
|
||
| // Byte-to-hex lookup is faster than toString(16) in browsers | ||
| const HEX: string[] = Array.from({ length: 256 }, (_, i) => | ||
| i.toString(16).padStart(2, '0') | ||
| ); | ||
|
|
||
| /** | ||
| * Fills buffer with random bytes, ensuring at least one is non-zero | ||
| * per W3C Trace Context spec. | ||
| */ | ||
| function randomFill(buf: Uint8Array): void { | ||
| for (let i = 0; i < buf.length; i++) { | ||
| buf[i] = (Math.random() * 256) >>> 0; | ||
| } | ||
| // Ensure non-zero | ||
| for (let i = 0; i < buf.length; i++) { | ||
| if (buf[i] > 0) return; | ||
| } | ||
| buf[buf.length - 1] = 1; | ||
|
Comment on lines
+35
to
+42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: did you consider using
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without running a test i can't say for sure but I'd say its very unlikely the crypto module is fast enough. The "cryptographically strong" requirement of that module trades off speed and its pretty significant in a lot of cases. We only have need for statistical randomness (uniform distribution) here. The distinction is that it might be possible to guess which trace id is generated in some cases (through things like timing attacks), but that's fine in this case.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I did try crypto: 10x slower than the original 🫠 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, apparently it's slower than your solution... this was unexpected 😄 https://jsben.ch/ZR73M
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We were actually using crypto module in the past and removed it for performance reasons https://github.com/open-telemetry/opentelemetry-js/pull/1349/changes
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just added a benchmark test. |
||
| } | ||
|
|
||
| function toHex(buf: Uint8Array): string { | ||
| let hex = ''; | ||
| for (let i = 0; i < buf.length; i++) { | ||
| hex += HEX[buf[i]]; | ||
| } | ||
| return hex; | ||
| } | ||
|
|
||
| export class RandomIdGenerator implements IdGenerator { | ||
| /** | ||
| * Returns a random 16-byte trace ID formatted/encoded as a 32 lowercase hex | ||
| * characters corresponding to 128 bits. | ||
| */ | ||
| generateTraceId = getIdGenerator(TRACE_ID_BYTES); | ||
| generateTraceId(): string { | ||
| randomFill(TRACE_BUFFER); | ||
| return toHex(TRACE_BUFFER); | ||
| } | ||
|
|
||
| /** | ||
| * Returns a random 8-byte span ID formatted/encoded as a 16 lowercase hex | ||
| * characters corresponding to 64 bits. | ||
| */ | ||
| generateSpanId = getIdGenerator(SPAN_ID_BYTES); | ||
| } | ||
|
|
||
| const SHARED_CHAR_CODES_ARRAY = Array(32); | ||
| function getIdGenerator(bytes: number): () => string { | ||
| return function generateId() { | ||
| for (let i = 0; i < bytes * 2; i++) { | ||
| SHARED_CHAR_CODES_ARRAY[i] = Math.floor(Math.random() * 16) + 48; | ||
| // valid hex characters in the range 48-57 and 97-102 | ||
| if (SHARED_CHAR_CODES_ARRAY[i] >= 58) { | ||
| SHARED_CHAR_CODES_ARRAY[i] += 39; | ||
| } | ||
| } | ||
| return String.fromCharCode.apply( | ||
| null, | ||
| SHARED_CHAR_CODES_ARRAY.slice(0, bytes * 2) | ||
| ); | ||
| }; | ||
| generateSpanId(): string { | ||
| randomFill(SPAN_BUFFER); | ||
| return toHex(SPAN_BUFFER); | ||
| } | ||
| } | ||
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.
nit: as a fallback a '1' is written in the last position to ensure the buf does not contain all zeroes. Maybe accessing a random position of the array and setting it to '1' if not already set would allow is to skip a second iteration.
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'm not a mathematician but I'd worry a bit that that biases the distribution away from values with
0s.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.
Me neither but what you said makes a lot of sense. The I guess a flag in the loop that fills the buf would be enough
Something like
Uh oh!
There was an error while loading. Please reload this page.
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.
@david-luna the
allZerocheck in the loop is about 20% slower than the proposed function.