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.

Sign up for GitHub

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

Jump to bottom

Apply updates suggested in PR comment #1

Merged
merged 1 commit into from
Jan 21, 2020
Merged

Apply updates suggested in PR comment #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions benchmark/benchmark.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,9 @@ const Benchmark = require('benchmark');
const benchmarks = require('beautify-benchmark');

Benchmark.options.maxTime = 0;
// @todo : Change it to between 50-100 or keep it random.
Benchmark.options.minSamples = 10;

module.exports = () => {
module.exports = (minSamples) => {
Benchmark.options.minSamples = minSamples;
const suite = new Benchmark.Suite();

return suite
Expand Down
4 changes: 2 additions & 2 deletions benchmark/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@
const execSync = require('child_process').execSync;
const exec = cmd => execSync(cmd, { stdio: [0, 1, 2] });

exec('node benchmark/tracer');
exec('node benchmark/propagator');
exec('node benchmark/tracer.js');
Copy link
Author

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

exec('node benchmark/propagator.js');
4 changes: 2 additions & 2 deletions benchmark/propagator.js
View file
Open in desktop
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');
Copy link
Author

Choose a reason for hiding this comment

The 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 = [
{
Expand All @@ -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)
Copy link
Author

Choose a reason for hiding this comment

The 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',
Expand Down
46 changes: 20 additions & 26 deletions benchmark/tracer.js
View file
Open in desktop
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 = [
Copy link
Author

Choose a reason for hiding this comment

The 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.

Expand All @@ -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',
Copy link
Author

Choose a reason for hiding this comment

The 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)
Copy link
Author

Choose a reason for hiding this comment

The 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();
Expand Down Expand Up @@ -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;
}