Integrated Benchmarks #4795

danstarns · 2024-06-15T13:11:51Z

This PR adds benchmarks with and without otel usage inside common node.js runtimes like HTTP and Express.

performance: high latency when using otel libs inside common endpoints #4741

What is added?

I have added a dir in the monorepo ./integrated-benchmarks. They contain a fork of otel-js-server-benchmarks, they use a combination of Crystal, Nix, bombardier to spawn Node.js servers with and without basic OpenTelemetry usage.

For each commit the benchmarks will run in CI, on pull requests the results are outputted to the console, and on main they are committed into the benchmarks.md file.

Anatomy of a benchmark

Each benchmark is a node.js web server on port 8000. Each server has a single endpoint /hello that will return a simple JSON response of { message: "Hello World" }. The benchmarks contain versions of this endpoint with and without OpenTelemetry span creation.

Comparison

To see the impact of OpenTelemetry JS in the tested runtimes, I have added "base case" benchmarks annotated without the -otel abbreviation.

For example the http benchmark:

import { createServer } from 'http';

const server = createServer((req, res) => {
  if (req.method === 'GET' && req.url === '/hello') {
    res.writeHead(200, { 'Content-Type': 'application/json' });
    res.end(JSON.stringify({ message: 'Hello World' }));
  } else {
    res.writeHead(404, { 'Content-Type': 'text/plain' });
    res.end('Not Found');
  }
});

server.listen(8000);

Now to see the impact of adding OpenTelemetry to the benchmarks I added basic usage in the annotated -otel benchmarks such as the http-otel:

import { createServer } from 'http';

+ import opentelemetry from '@opentelemetry/api';
+ import { OTLPTraceExporter } from '@opentelemetry/exporter-trace-otlp-http';
+ import { Resource } from '@opentelemetry/resources';
+ import {
+  BasicTracerProvider,
+  BatchSpanProcessor,
+ } from '@opentelemetry/sdk-trace-base';
+ import { SEMRESATTRS_SERVICE_NAME } from '@opentelemetry/semantic-conventions';

+ const provider = new BasicTracerProvider({
+  resource: new Resource({
+    [SEMRESATTRS_SERVICE_NAME]: 'basic-service',
+  }),
+ });

+ const exporter = new OTLPTraceExporter({});
+ provider.addSpanProcessor(new BatchSpanProcessor(exporter));
+ provider.register();

const server = createServer((req, res) => {
  if (req.method === 'GET' && req.url === '/hello') {
+    const tracer = opentelemetry.trace.getTracer('hello-tracer');
+    const span = tracer.startSpan('hello');
+    span.setAttribute('value', 'world');
+    span.end();

    res.writeHead(200, { 'Content-Type': 'application/json' });
    res.end(JSON.stringify({ message: 'Hello World' }));
  } else {
    res.writeHead(404, { 'Content-Type': 'text/plain' });
    res.end('Not Found');
  }
});

server.listen(8000);

Why was it added?

Based on the conversation discussed in the related issue:

performance: high latency when using otel libs inside common endpoints #4741

The current benchmarks: https://open-telemetry.github.io/opentelemetry-js/benchmarks/ don't give a good reflection of how processing, exporting, and creating spans in a web server affect the performance.

Not only this, but the benchmarks that we already created show that even after doing all the necessary things such as adding the BatchProcessor, OpenTelemetry JS still adds significant overhead to a Node.js application.

Given all the reasons in the linked issue, and mentioned here, I have added these benchmarks into the source code of OpenTelemetry as they provide a transparent insight into performance plus allow the community to iterate on performance improvements.

danstarns · 2024-06-15T19:18:07Z

tsconfig.json

+      "integrated-benchmarks/http",
+      "integrated-benchmarks/http-otel",
+      "integrated-benchmarks/express",
+      "integrated-benchmarks/express-otel"


@pichlermarc do you know why these get removed on npm i are they meant to be somewhere else?

I think these may also need to be added to the npm workspace and and then they should stay even when npm installing. Same as below.

danstarns · 2024-06-15T19:18:30Z

tsconfig.json

+    },
+    {
+      "path": "integrated-benchmarks/http"
+    },
+    {
+      "path": "integrated-benchmarks/http-otel"
+    },
+    {
+      "path": "integrated-benchmarks/express"
+    },
+    {
+      "path": "integrated-benchmarks/express-otel"


danstarns · 2024-06-15T19:18:59Z

integrated-benchmarks/shell.nix

+    nodejs
+    nodePackages.npm
+    rustc
+    cargo
+    go
+    crystal


Nix Shell needs these runtimes to perform the benchmarks.

danstarns · 2024-06-15T19:20:20Z

.github/workflows/integrated-benchmarks.yml

+          key: ${{ runner.os }}-node-${{ hashFiles('**/package-lock.json') }}
+      - name: Run benchmarks
+        run: cd ./integrated-benchmarks && nix-shell --quiet --run ./run.cr
+      - name: Push readme


Open discussion where should the benchmarks go? Is committing them to the readme ok for now?

Benchmark data is currenty commited to the gh-pages branch at https://github.com/open-telemetry/opentelemetry-js/blob/gh-pages/benchmarks/data.js and then show up at https://opentelemetry.io/docs/languages/js/benchmarks/

danstarns · 2024-06-15T19:20:52Z

.github/workflows/integrated-benchmarks.yml

@@ -0,0 +1,47 @@
+name: Run Integrated Benchmarks
+
+on:


Open discussion on when these benchmarks should be invoked.

danstarns · 2024-06-15T19:23:00Z

integrated-benchmarks/README.md

+cd ./integrated-benchmarks && nix-shell --quiet --run ./run.cr
+```
+
+> Its best to use the pipeline to run the benchmarks, as it will ensure that the benchmarks are run in the same environment as CI. Running locally may give different results and consume all your resources.


The committed initial benchmarks in benchmarks.md are from running locally. Ideally, we should let the main branch commit the benchmarks and use the main machine as the source of truth.

On merge of this PR the benchmarks file would be updated and should reflect more reliable data.

danstarns · 2024-06-15T19:25:02Z

integrated-benchmarks/benchmarks.md

@@ -0,0 +1,10 @@
+<!-- README.md is generated from README.ecr, do not edit -->


ditto - local machine initial data - let CI commit it in.

Or check pipelines output @pichlermarc could you enable the runners for this PR?

danstarns · 2024-06-15T19:25:33Z

integrated-benchmarks/express-otel/package.json

+    "start": "node ./build/src/index.js",
+    "compile": "tsc --build",
+    "clean": "tsc --build --clean",
+    "lint": "eslint . --ext .ts",
+    "lint:fix": "eslint . --ext .ts --fix",
+    "version": "node ../../scripts/version-update.js",
+    "precompile": "cross-var lerna run version --scope $npm_package_name --include-dependencies",
+    "prewatch": "npm run precompile",
+    "peer-api-check": "node ../../scripts/peer-api-check.js",
+    "align-api-deps": "node ../../scripts/align-api-deps.js"


What of these do we need if we never publish the benchmarks to npm?

danstarns

Use ./integrated-benchmarks/README.md as the source of truth for this PR's supporting documentation.

pichlermarc

I think this is a great Idea and definitely helpful.

I'm worried about long-term maintainability though with the additional tooling that many contributors are not too familiar with.

pichlermarc · 2024-06-18T11:37:10Z

integrated-benchmarks/express-otel/src/index.ts

+  var app = express();
+
+  app.use('/hello', async (req, res) => {
+    const tracer = opentelemetry.trace.getTracer('hello-tracer');


A performance-minded user would instantiate one tracer and hold on to it to avoid expensive lookup operations on each request.

pichlermarc · 2024-06-18T11:43:21Z

.github/workflows/integrated-benchmarks.yml

+          key: ${{ runner.os }}-node-${{ hashFiles('**/package-lock.json') }}
+      - name: Run benchmarks
+        run: cd ./integrated-benchmarks && nix-shell --quiet --run ./run.cr
+      - name: Push readme


Benchmark data is currenty commited to the gh-pages branch at https://github.com/open-telemetry/opentelemetry-js/blob/gh-pages/benchmarks/data.js and then show up at https://opentelemetry.io/docs/languages/js/benchmarks/

pichlermarc · 2024-06-18T11:44:37Z

.github/workflows/integrated-benchmarks.yml

+
+jobs:
+  integrated-benchmarks:
+    runs-on: ubuntu-latest


we do have bare-metal runners for benchmarks, see https://github.com/open-telemetry/opentelemetry-js/blob/main/.github/workflows/benchmark.yml

pichlermarc · 2024-06-18T12:00:43Z

.github/workflows/integrated-benchmarks.yml

+      - uses: cachix/install-nix-action@v24
+        with:
+          nix_path: nixpkgs=channel:nixos-unstable
+      - uses: actions/cache@v3
+        with:
+          path: |
+            ~/.ivy2/cache
+            ~/.sbt
+          key: ${{ runner.os }}-sbt-${{ hashFiles('**/build.sbt') }}
+      - uses: actions/cache@v3
+        with:
+          path: |
+            ~/.cargo/bin/
+            ~/.cargo/registry/index/
+            ~/.cargo/registry/cache/
+            ~/.cargo/git/db/
+          key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}


Hmm, would it be possible to get away with less tooling? Most of our contributors may not be familiar with these tools. I'm worried about long-term maintainability of this if we don't have enough people familiar with it.

pichlermarc · 2024-06-18T14:22:59Z

tsconfig.json

+      "integrated-benchmarks/http",
+      "integrated-benchmarks/http-otel",
+      "integrated-benchmarks/express",
+      "integrated-benchmarks/express-otel"


I think these may also need to be added to the npm workspace and and then they should stay even when npm installing. Same as below.

pichlermarc · 2024-06-18T14:24:02Z

integrated-benchmarks/express-otel/package.json

+  "peerDependencies": {
+    "@opentelemetry/api": ">=1.0.0 <1.10.0"
+  },


since this does not implement the API we don't need the peer-dep.

pichlermarc · 2024-06-18T14:24:32Z

integrated-benchmarks/express-otel/package.json

+    "access": "restricted"
+  },
+  "devDependencies": {
+    "@opentelemetry/api": ">=1.0.0 <1.10.0",


Same here, does not implement the API so we can directly depend on the latest version.

Suggested change

"@opentelemetry/api": ">=1.0.0 <1.10.0",

"@opentelemetry/api": "1.9.0",

pichlermarc · 2024-06-18T14:28:09Z

integrated-benchmarks/express-otel/package.json

+    "express": "4.19.2",
+    "cross-var": "1.1.0",
+    "lerna": "6.6.2",
+    "ts-mocha": "10.0.0",


I think mocha is unused in this package.

github-actions · 2024-08-19T06:37:59Z

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions · 2024-09-09T06:39:55Z

This PR was closed because it has been stale for 14 days with no activity.

feat: add init integrated benchmarks

b5ac660

danstarns requested a review from a team June 15, 2024 13:11

package: remove conflict

4b61cf6

danstarns marked this pull request as draft June 15, 2024 14:59

danstarns added 6 commits June 15, 2024 17:54

docs: add more benchmark docs

fe0a021

feat: make benchmarks runnable with docs

2a3bec6

fix: eslint path

d237e19

refactor: change to BatchSpanProcessor

263177d

feat: add express benchmark

d4d1e2f

feat: add express benchmarks

3b91b92

danstarns marked this pull request as ready for review June 15, 2024 18:40

danstarns added 6 commits June 15, 2024 19:41

fix: add lock back

f080596

fix: space in output

06fb9b7

refactor: remove not needed .md

7e3de82

refactor: remove npmignore

e6267a3

refactor: remove unused readme

acdcd16

docs: update pkg description

9b5f5aa

danstarns commented Jun 15, 2024

View reviewed changes

danstarns mentioned this pull request Jun 15, 2024

performance: high latency when using otel libs inside common endpoints #4741

Closed

pichlermarc reviewed Jun 18, 2024

View reviewed changes

github-actions bot added the stale label Aug 19, 2024

github-actions bot closed this Sep 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrated Benchmarks #4795

Integrated Benchmarks #4795

danstarns commented Jun 15, 2024 •

edited

Loading

danstarns Jun 15, 2024

pichlermarc Jun 18, 2024

danstarns Jun 15, 2024

danstarns Jun 15, 2024

danstarns Jun 15, 2024

pichlermarc Jun 18, 2024

danstarns Jun 15, 2024

danstarns Jun 15, 2024 •

edited

Loading

danstarns Jun 15, 2024

danstarns Jun 15, 2024

danstarns left a comment

pichlermarc left a comment

pichlermarc Jun 18, 2024

pichlermarc Jun 18, 2024

pichlermarc Jun 18, 2024

pichlermarc Jun 18, 2024

pichlermarc Jun 18, 2024

pichlermarc Jun 18, 2024

pichlermarc Jun 18, 2024

pichlermarc Jun 18, 2024

github-actions bot commented Aug 19, 2024

github-actions bot commented Sep 9, 2024

		@@ -0,0 +1,10 @@
		<!-- README.md is generated from README.ecr, do not edit -->

	"@opentelemetry/api": ">=1.0.0 <1.10.0",
	"@opentelemetry/api": "1.9.0",

Integrated Benchmarks #4795

Integrated Benchmarks #4795

Conversation

danstarns commented Jun 15, 2024 • edited Loading

What is added?

Anatomy of a benchmark

Comparison

Why was it added?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danstarns Jun 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danstarns left a comment

Choose a reason for hiding this comment

pichlermarc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 19, 2024

github-actions bot commented Sep 9, 2024

danstarns commented Jun 15, 2024 •

edited

Loading

danstarns Jun 15, 2024 •

edited

Loading