Skip to content
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

Simplify and speed up trace context parsing #708

Merged

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Jan 17, 2020

Which problem is this PR solving?

  • Speeds up the trace context parser significantly

Short description of the changes

  • Remove duplicate validity checks
  • Condense validity check and parsing into a single step

Benchmark

On my machine I achieved a ~2X speedup

➜  testbed node index.js

  2 tests completed.

  old x 1,835,685 ops/sec ±0.46% (93 runs sampled)
  new x 3,499,510 ops/sec ±0.32% (96 runs sampled)

Based on this benchmark code:

const Benchmark = require('benchmark');
const pretty = require('beautify-benchmark');

const VALID_TRACE_PARENT_REGEX_NEW = /^00-([\da-f]{32})-([\da-f]{16})-([\da-f]{2})$/;
const VALID_TRACE_PARENT_REGEX = /^[\da-f]{2}-[\da-f]{32}-[\da-f]{16}-[\da-f]{2}$/;
const VALID_TRACEID_REGEX = /^[0-9a-f]{32}$/i;
const VALID_SPANID_REGEX = /^[0-9a-f]{16}$/i;
const INVALID_ID_REGEX = /^0+$/i;
const VERSION = '00';

const suite = new Benchmark.Suite()
    .on('cycle', event => pretty.add(event.target))
    .on('error', event => { throw event.target.error; })
    .on('complete', function () { pretty.log(); });

suite
    .add("old", () => parseTraceParent("00-d4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-00"))
    .add("new", () => newParseTraceParent("00-d4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-00"));

suite.run();

function newParseTraceParent(traceParent) {
    const match = traceParent.match(VALID_TRACE_PARENT_REGEX_NEW);
    if (
        !match ||
        match[1] === '00000000000000000000000000000000' ||
        match[2] === '0000000000000000'
    ) {
        return null;
    }

    return { traceId: match[1], spanId: match[2], traceFlags: parseInt(match[3], 16) };
}

function parseTraceParent(traceParent) {
    const match = traceParent.match(VALID_TRACE_PARENT_REGEX);
    if (!match) return null;
    const parts = traceParent.split('-');
    const [version, traceId, spanId, option] = parts;
    const traceFlags = parseInt(option, 16);

    if (
        !isValidVersion(version) ||
        !isValidTraceId(traceId) ||
        !isValidSpanId(spanId)
    ) {
        return null;
    }

    return { traceId, spanId, traceFlags };
}

function isValidVersion(version) {
    return version === VERSION;
}

function isValidTraceId(traceId) {
    return VALID_TRACEID_REGEX.test(traceId) && !INVALID_ID_REGEX.test(traceId);
}

function isValidSpanId(spanId) {
    return VALID_SPANID_REGEX.test(spanId) && !INVALID_ID_REGEX.test(spanId);
}

@codecov-io
Copy link

codecov-io commented Jan 17, 2020

Codecov Report

Merging #708 into master will decrease coverage by 1.79%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master     #708     +/-   ##
=========================================
- Coverage   92.69%   90.89%   -1.8%     
=========================================
  Files         228      225      -3     
  Lines       10411    10305    -106     
  Branches      939      957     +18     
=========================================
- Hits         9650     9367    -283     
- Misses        761      938    +177
Impacted Files Coverage Δ
packages/opentelemetry-core/test/utils/url.test.ts 50% <0%> (-50%) ⬇️
...pentelemetry-core/test/internal/validators.test.ts 50% <0%> (-50%) ⬇️
...elemetry-core/test/trace/spancontext-utils.test.ts 55.55% <0%> (-44.45%) ⬇️
...lemetry-core/test/trace/ProbabilitySampler.test.ts 56.52% <0%> (-43.48%) ⬇️
...s/opentelemetry-core/test/trace/NoopTracer.test.ts 60% <0%> (-40%) ⬇️
...s/opentelemetry-core/test/context/B3Format.test.ts 63.39% <0%> (-36.61%) ⬇️
...ges/opentelemetry-core/test/trace/NoopSpan.test.ts 63.63% <0%> (-36.37%) ⬇️
...s/opentelemetry-core/test/trace/tracestate.test.ts 65.06% <0%> (-34.94%) ⬇️
...ntelemetry-core/test/trace/NoRecordingSpan.test.ts 71.42% <0%> (-28.58%) ⬇️
...ackages/opentelemetry-core/src/platform/node/id.ts 71.42% <0%> (-28.58%) ⬇️
... and 41 more

Copy link
Member

@OlivierAlbertini OlivierAlbertini left a comment

Choose a reason for hiding this comment

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

Good job!

@mayurkale22 mayurkale22 added the enhancement New feature or request label Jan 18, 2020
Copy link
Member

@mayurkale22 mayurkale22 left a comment

Choose a reason for hiding this comment

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

Nice 💯

@mayurkale22 mayurkale22 added the Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Jan 20, 2020
@mayurkale22 mayurkale22 added this to the Alpha v0.3.3 milestone Jan 20, 2020
@dyladan dyladan merged commit 8496fb2 into open-telemetry:master Jan 22, 2020
@dyladan dyladan deleted the trace-context-fast branch January 22, 2020 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants