-
Notifications
You must be signed in to change notification settings - Fork 0
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
Apply updates suggested in PR comment #1
Changes from all commits
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 |
---|---|---|
@@ -1,7 +1,7 @@ | ||
'use strict'; | ||
|
||
const benchmark = require('./benchmark'); | ||
const opentelemetry = require('@opentelemetry/core'); | ||
const opentelemetry = require('../packages/opentelemetry-core'); | ||
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. Loading by filepath removes dependency management issues and ensures you're testing the current checked out branch |
||
|
||
const setups = [ | ||
{ | ||
|
@@ -26,7 +26,7 @@ const setups = [ | |
for (const setup of setups) { | ||
console.log(`Beginning ${setup.name} Benchmark...`); | ||
const propagator = setup.propagator; | ||
const suite = benchmark() | ||
const suite = benchmark(100) | ||
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. formatters are fast so we can run them more times |
||
.add('#Inject', function () { | ||
propagator.inject({ | ||
traceId: 'd4cda95b652f4a1592b449d5929fda1b', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,9 @@ | ||
'use strict'; | ||
|
||
const benchmark = require('./benchmark'); | ||
const opentelemetry = require('@opentelemetry/core'); | ||
const { BasicTracerRegistry, BatchSpanProcessor, InMemorySpanExporter, SimpleSpanProcessor } = require('@opentelemetry/tracing'); | ||
const { NodeTracerRegistry } = require('@opentelemetry/node'); | ||
const opentelemetry = require('../packages/opentelemetry-core'); | ||
const { BasicTracerRegistry, BatchSpanProcessor, InMemorySpanExporter, SimpleSpanProcessor } = require('../packages/opentelemetry-tracing'); | ||
|
||
const exporter = new InMemorySpanExporter(); | ||
const logger = new opentelemetry.NoopLogger(); | ||
|
||
const setups = [ | ||
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 think these setups make more sense since Basic and Node TracerRegistries return the same Tracer class so their performance should be identical. |
||
|
@@ -14,16 +12,23 @@ const setups = [ | |
registry: new BasicTracerRegistry({ logger }) | ||
}, | ||
{ | ||
name: 'NodeTracerRegistry', | ||
registry: new NodeTracerRegistry({ logger }) | ||
name: 'BasicTracerRegistry with SimpleSpanProcessor', | ||
registry: getRegistry(new SimpleSpanProcessor(new InMemorySpanExporter())) | ||
}, | ||
{ | ||
name: 'BasicTracerRegistry with BatchSpanProcessor', | ||
registry: getRegistry(new BatchSpanProcessor(new InMemorySpanExporter())) | ||
}, | ||
{ | ||
name: 'NoopTracerRegistry', | ||
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. Adding a noop as a baseline |
||
registry: opentelemetry.getTracerRegistry() | ||
} | ||
]; | ||
|
||
for (const setup of setups) { | ||
console.log(`Beginning ${setup.name} Benchmark...`); | ||
const registry = setup.registry; | ||
const tracer = registry.getTracer("benchmark"); | ||
const suite = benchmark() | ||
const tracer = setup.registry.getTracer("benchmark"); | ||
const suite = benchmark(20) | ||
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. The tracer tests run slower so I did fewer runs. feel free to tweak it |
||
.add('#startSpan', function () { | ||
const span = tracer.startSpan('op'); | ||
span.end(); | ||
|
@@ -52,25 +57,14 @@ for (const setup of setups) { | |
span.setAttribute('attr-key-' + j, 'attr-value-' + j); | ||
} | ||
span.end(); | ||
}) | ||
.add('#startSpan with SimpleSpanProcessor', function () { | ||
const simpleSpanProcessor = new SimpleSpanProcessor(exporter); | ||
|
||
registry.addSpanProcessor(simpleSpanProcessor); | ||
const span = tracer.startSpan('op'); | ||
span.end(); | ||
|
||
simpleSpanProcessor.shutdown(); | ||
}) | ||
.add('#startSpan with BatchSpanProcessor', function () { | ||
const batchSpanProcessor = new BatchSpanProcessor(exporter); | ||
|
||
registry.addSpanProcessor(batchSpanProcessor); | ||
const span = tracer.startSpan('op'); | ||
span.end(); | ||
batchSpanProcessor.shutdown(); | ||
}); | ||
|
||
// run async | ||
suite.run({ async: false }); | ||
} | ||
function getRegistry(processor) { | ||
const registry = new BasicTracerRegistry({ logger }); | ||
registry.addSpanProcessor(processor); | ||
return registry; | ||
} | ||
|
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.
This fixes the issue oliver was talking about