-
Notifications
You must be signed in to change notification settings - Fork 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 all 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
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| 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++) { | ||
|
Contributor
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. 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.
Contributor
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'm not a mathematician but I'd worry a bit that that biases the distribution away from values with
Contributor
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. 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 let allZero = true;
for (let i = 0; i < buf.length; i++) {
buf[i] = (Math.random() * 256) >>> 0;
allZero = allZero && buf[i] === 0;
}
if (allZero) {
buf[buf.length - 1] = 1;
}
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. @david-luna the |
||
| 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); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| /* | ||
| * Copyright The OpenTelemetry Authors | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| import * as Benchmark from 'benchmark'; | ||
| import { RandomIdGenerator } from '../../src/platform'; | ||
|
|
||
| const idGenerator = new RandomIdGenerator(); | ||
|
|
||
| describe('RandomIdGenerator benchmark (browser)', function () { | ||
| this.timeout(60000); | ||
|
|
||
| it('generateTraceId (browser)', done => { | ||
| const suite = new Benchmark.Suite(); | ||
| suite | ||
| .add('generateTraceId (browser)', () => idGenerator.generateTraceId()) | ||
| .on('cycle', (event: Benchmark.Event) => | ||
| console.log(String(event.target)) | ||
| ) | ||
| .on('complete', () => done()) | ||
| .run({ async: true }); | ||
| }); | ||
|
|
||
| it('generateSpanId (browser)', done => { | ||
| const suite = new Benchmark.Suite(); | ||
| suite | ||
| .add('generateSpanId (browser)', () => idGenerator.generateSpanId()) | ||
| .on('cycle', (event: Benchmark.Event) => | ||
| console.log(String(event.target)) | ||
| ) | ||
| .on('complete', () => done()) | ||
| .run({ async: true }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| /* | ||
| * Copyright The OpenTelemetry Authors | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| import * as Benchmark from 'benchmark'; | ||
| import { BasicTracerProvider } from '../../src'; | ||
|
|
||
| const tracerProvider = new BasicTracerProvider(); | ||
| const tracer = tracerProvider.getTracer('test'); | ||
|
|
||
| describe('Span benchmark (browser)', function () { | ||
| this.timeout(60000); | ||
|
|
||
| it('create spans (10 attributes) (browser)', done => { | ||
| const suite = new Benchmark.Suite(); | ||
| suite | ||
| .add('create spans (10 attributes) (browser)', () => { | ||
| const span = tracer.startSpan('span'); | ||
| span.setAttribute('aaaaaaaaaaaaaaaaaaaa', 'aaaaaaaaaaaaaaaaaaaa'); | ||
| span.setAttribute('bbbbbbbbbbbbbbbbbbbb', 'aaaaaaaaaaaaaaaaaaaa'); | ||
| span.setAttribute('cccccccccccccccccccc', 'aaaaaaaaaaaaaaaaaaaa'); | ||
| span.setAttribute('dddddddddddddddddddd', 'aaaaaaaaaaaaaaaaaaaa'); | ||
| span.setAttribute('eeeeeeeeeeeeeeeeeeee', 'aaaaaaaaaaaaaaaaaaaa'); | ||
| span.setAttribute('ffffffffffffffffffff', 'aaaaaaaaaaaaaaaaaaaa'); | ||
| span.setAttribute('gggggggggggggggggggg', 'aaaaaaaaaaaaaaaaaaaa'); | ||
| span.setAttribute('hhhhhhhhhhhhhhhhhhhh', 'aaaaaaaaaaaaaaaaaaaa'); | ||
| span.setAttribute('iiiiiiiiiiiiiiiiiiii', 'aaaaaaaaaaaaaaaaaaaa'); | ||
| span.setAttribute('jjjjjjjjjjjjjjjjjjjj', 'aaaaaaaaaaaaaaaaaaaa'); | ||
| span.end(); | ||
| }) | ||
| .on('cycle', (event: Benchmark.Event) => | ||
| console.log(String(event.target)) | ||
| ) | ||
| .on('complete', () => done()) | ||
| .run({ async: true }); | ||
| }); | ||
| }); |
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 we keep this file? This is a benchmark running node using browser code. I think quoting some benchmark code (or perhaps a https://jsben.ch or similar link) showing browser numbers in the PR discussion would be good, but not sure of the value of having the benchmark file persisting in the repo.
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.
Great point. Let me try to add a browser bench test.
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.
@trentm I moved some files around so the platform chosen in package.json is used for common tests, including the new benchmark test for the randomIdGenerator. It expanded the scope quite a bit but now reports numbers for node and browser. I updated the description above with node, browser (main), and browser (this branch) results.
If this is too many changes for this PR I can back them out and make a new branch for the test folder.
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.
@trentm I added browser benchmark tests that do not write to disk. I just saw @pichlermarc's note about being judicious with the amount of benchmarks published.
I defer to you whether or not they should be published (in a follow-up)
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.
Great, thanks. Sounds good to me. My main concern had been about the (lack of) value of having those metrics included in the published benchmarks.
Marc had mentioned that he has a PR coming at some point to reduce the set of metrics that will be included in the published set (by just changing the benchmark to not output results to the benchmark.txt file that is slurped up by the benchmark.yml CI, I believe).