From aec2ed1d3ace03f9c4bb3de81f7100bfeef45542 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Thu, 25 Jan 2024 15:52:10 +0800 Subject: [PATCH] refactor(exporter-prometheus): promisify prometheus tests (#4431) * refactor(exporter-prometheus): promisify prometheus tests * fix: lint --------- Co-authored-by: Marc Pichler --- doc/metrics.md | 8 +- experimental/CHANGELOG.md | 3 + .../src/PrometheusExporter.ts | 8 +- .../test/PrometheusExporter.test.ts | 387 +++++++----------- .../packages/opentelemetry-sdk-node/README.md | 2 +- 5 files changed, 154 insertions(+), 254 deletions(-) diff --git a/doc/metrics.md b/doc/metrics.md index 3f48775e49f..1eb873a11d6 100644 --- a/doc/metrics.md +++ b/doc/metrics.md @@ -59,7 +59,7 @@ const { getNodeAutoInstrumentations, } = require("@opentelemetry/auto-instrumentations-node"); -const prometheusExporter = new PrometheusExporter({ startServer: true }); +const prometheusExporter = new PrometheusExporter(); const sdk = new opentelemetry.NodeSDK({ // Optional - If omitted, the metrics SDK will not be initialized @@ -147,7 +147,7 @@ const { getNodeAutoInstrumentations, } = require("@opentelemetry/auto-instrumentations-node"); -const prometheusExporter = new PrometheusExporter({ startServer: true }); +const prometheusExporter = new PrometheusExporter(); const sdk = new opentelemetry.NodeSDK({ // Optional - If omitted, the metrics SDK will not be initialized @@ -499,8 +499,8 @@ to use the Prometheus exporter `PrometheusExporter` which is included in the const { PrometheusExporter } = require('@opentelemetry/exporter-prometheus'); const { MeterProvider } = require('@opentelemetry/sdk-metrics'); -// Add your port and startServer to the Prometheus options -const options = { port: 9464, startServer: true }; +// Add your port to the Prometheus options +const options = { port: 9464 }; const exporter = new PrometheusExporter(options); // Creates MeterProvider and installs the exporter as a MetricReader diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index 5f80d2e3e99..4135754501f 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -16,6 +16,7 @@ All notable changes to experimental packages in this project will be documented ### :bug: (Bug Fix) +* fix(exporter-prometheus): avoid invoking callback synchronously [#4431](https://github.com/open-telemetry/opentelemetry-js/pull/4431) @legendecas * fix(exporter-logs-otlp-grpc): set User-Agent header [#4398](https://github.com/open-telemetry/opentelemetry-js/pull/4398) @Vunovati * fix(exporter-logs-otlp-http): set User-Agent header [#4398](https://github.com/open-telemetry/opentelemetry-js/pull/4398) @Vunovati * fix(exporter-logs-otlp-proto): set User-Agent header [#4398](https://github.com/open-telemetry/opentelemetry-js/pull/4398) @Vunovati @@ -24,6 +25,8 @@ All notable changes to experimental packages in this project will be documented ### :house: (Internal) +* refactor(exporter-prometheus): promisify prometheus tests [#4431](https://github.com/open-telemetry/opentelemetry-js/pull/4431) @legendecas + ## 0.47.0 ### :boom: Breaking Change diff --git a/experimental/packages/opentelemetry-exporter-prometheus/src/PrometheusExporter.ts b/experimental/packages/opentelemetry-exporter-prometheus/src/PrometheusExporter.ts index 42e4fc41e88..bd763de3f0c 100644 --- a/experimental/packages/opentelemetry-exporter-prometheus/src/PrometheusExporter.ts +++ b/experimental/packages/opentelemetry-exporter-prometheus/src/PrometheusExporter.ts @@ -44,6 +44,7 @@ export class PrometheusExporter extends MetricReader { private readonly _prefix?: string; private readonly _appendTimestamp: boolean; private _serializer: PrometheusSerializer; + private _startServerPromise: Promise | undefined; // This will be required when histogram is implemented. Leaving here so it is not forgotten // Histogram cannot have a attribute named 'le' @@ -95,7 +96,8 @@ export class PrometheusExporter extends MetricReader { callback(err); }); } else if (callback) { - callback(); + // Do not invoke callback immediately to avoid zalgo problem. + queueMicrotask(callback); } } @@ -142,7 +144,7 @@ export class PrometheusExporter extends MetricReader { * Starts the Prometheus export server */ startServer(): Promise { - return new Promise((resolve, reject) => { + this._startServerPromise ??= new Promise((resolve, reject) => { this._server.once('error', reject); this._server.listen( { @@ -157,6 +159,8 @@ export class PrometheusExporter extends MetricReader { } ); }); + + return this._startServerPromise; } /** diff --git a/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts b/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts index d5122406778..66bab19ef8f 100644 --- a/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts +++ b/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts @@ -53,46 +53,37 @@ describe('PrometheusExporter', () => { }); describe('constructor', () => { - it('should construct an exporter', done => { + it('should construct an exporter', async () => { const exporter = new PrometheusExporter(); assert.ok(typeof exporter.startServer === 'function'); assert.ok(typeof exporter.shutdown === 'function'); - exporter.shutdown().then(done); + await exporter.shutdown(); }); - it('should start the server by default and call the callback', done => { + it('should start the server by default and call the callback', async () => { const port = PrometheusExporter.DEFAULT_OPTIONS.port; const endpoint = PrometheusExporter.DEFAULT_OPTIONS.endpoint; - const exporter = new PrometheusExporter({}, () => { - const url = `http://localhost:${port}${endpoint}`; - http.get(url, (res: any) => { - assert.strictEqual(res.statusCode, 200); - exporter.shutdown().then(() => { - return done(); - }); - }); - }); + const exporter = new PrometheusExporter(); + await exporter.startServer(); + const url = `http://localhost:${port}${endpoint}`; + await request(url); + await exporter.shutdown(); }); - it('should pass server error to callback when port is already in use', done => { - const firstExporter = new PrometheusExporter({}, error => { - if (error) { - // This should not happen as the port should not be already in use when the test starts. - done(error); - } - }); - const secondExporter = new PrometheusExporter({}, error => { - firstExporter - .shutdown() - .then(() => secondExporter.shutdown()) - .then(() => - done( - error - ? undefined - : 'Second exporter should respond with EADDRINUSE but did not pass it to callback' - ) - ); - }); + it('should pass server error to callback when port is already in use', async () => { + const firstExporter = new PrometheusExporter(); + await firstExporter.startServer(); + + const secondExporter = new PrometheusExporter(); + await assert.rejects( + secondExporter.startServer(), + { code: 'EADDRINUSE' }, + 'Second exporter should respond with EADDRINUSE but did not pass it to callback' + ); + + await Promise.all( + [firstExporter, secondExporter].map(it => it.shutdown()) + ); }); it('should not start the server if preventServerStart is passed as an option', () => { @@ -102,51 +93,35 @@ describe('PrometheusExporter', () => { }); describe('server', () => { - it('should start on startServer() and call the callback', done => { + it('should start on startServer() and call the callback', async () => { const exporter = new PrometheusExporter({ port: 9722, preventServerStart: true, }); - exporter.startServer().then(() => { - exporter.shutdown().then(() => { - return done(); - }); - }); + await exporter.startServer(); + await exporter.shutdown(); }); - it('should listen on the default port and default endpoint', done => { + it('should listen on the default port and default endpoint', async () => { const port = PrometheusExporter.DEFAULT_OPTIONS.port; const endpoint = PrometheusExporter.DEFAULT_OPTIONS.endpoint; - const exporter = new PrometheusExporter({}, () => { - const url = `http://localhost:${port}${endpoint}`; - http.get(url, (res: any) => { - assert.strictEqual(res.statusCode, 200); - exporter.shutdown().then(() => { - return done(); - }); - }); - }); + const exporter = new PrometheusExporter(); + const url = `http://localhost:${port}${endpoint}`; + await request(url); + await exporter.shutdown(); }); - it('should listen on a custom port and endpoint if provided', done => { + it('should listen on a custom port and endpoint if provided', async () => { const port = 9991; const endpoint = '/metric'; - const exporter = new PrometheusExporter( - { - port, - endpoint, - }, - () => { - const url = `http://localhost:${port}${endpoint}`; - http.get(url, (res: any) => { - assert.strictEqual(res.statusCode, 200); - exporter.shutdown().then(() => { - return done(); - }); - }); - } - ); + const exporter = new PrometheusExporter({ + port, + endpoint, + }); + const url = `http://localhost:${port}${endpoint}`; + await request(url); + await exporter.shutdown(); }); it('should unref the server to allow graceful termination', () => { @@ -169,67 +144,36 @@ describe('PrometheusExporter', () => { assert.strictEqual(exporter['_port'], 1234); }); - it('should not require endpoints to start with a slash', done => { + it('should not require endpoints to start with a slash', async () => { const port = 9991; const endpoint = 'metric'; + const url = `http://localhost:${port}/metric`; - const exporter = new PrometheusExporter( - { - port, - endpoint, - }, - () => { - const url = `http://localhost:${port}/metric`; - http.get(url, (res: any) => { - assert.strictEqual(res.statusCode, 200); - exporter.shutdown().then(() => { - const exporter2 = new PrometheusExporter( - { - port, - endpoint: `/${endpoint}`, - }, - () => { - const url = `http://localhost:${port}/metric`; - http.get(url, (res: any) => { - assert.strictEqual(res.statusCode, 200); - exporter2.stopServer().then(() => { - return done(); - }); - }); - } - ); - }); - }); - } - ); + const exporter = new PrometheusExporter({ + port, + endpoint, + }); + await exporter.startServer(); + await request(url); + await exporter.stopServer(); }); - it('should return a HTTP status 404 if the endpoint does not match', done => { + it('should return a HTTP status 404 if the endpoint does not match', async () => { const port = 9912; const endpoint = '/metrics'; - const exporter = new PrometheusExporter( - { - port, - endpoint, - }, - () => { - const url = `http://localhost:${port}/invalid`; - - http.get(url, (res: any) => { - assert.strictEqual(res.statusCode, 404); - exporter.shutdown().then(() => { - return done(); - }); - }); - } - ); + const exporter = new PrometheusExporter({ + port, + endpoint, + }); + const url = `http://localhost:${port}/invalid`; + + await assert.rejects(request(url), { statusCode: 404 }); + await exporter.shutdown(); }); - it('should call a provided callback on shutdown regardless of if the server is running', done => { + it('should call a provided callback on shutdown regardless of if the server is running', async () => { const exporter = new PrometheusExporter({ preventServerStart: true }); - exporter.shutdown().then(() => { - return done(); - }); + await exporter.shutdown(); }); it('should able to call getMetricsRequestHandler function to generate response with metrics', async () => { @@ -259,18 +203,17 @@ describe('PrometheusExporter', () => { let meterProvider: MeterProvider; let meter: Meter; - beforeEach(done => { - exporter = new PrometheusExporter({}, () => { - meterProvider = new MeterProvider({ - readers: [exporter], - }); - meter = meterProvider.getMeter('test-prometheus', '1'); - done(); + beforeEach(async () => { + exporter = new PrometheusExporter(); + meterProvider = new MeterProvider({ + readers: [exporter], }); + meter = meterProvider.getMeter('test-prometheus', '1'); + await exporter.startServer(); }); - afterEach(done => { - exporter.shutdown().then(done); + afterEach(async () => { + await exporter.shutdown(); }); it('should export a count aggregation', async () => { @@ -347,7 +290,7 @@ describe('PrometheusExporter', () => { ]); }); - it('should export multiple attributes on manual shutdown', done => { + it('should export multiple attributes on manual shutdown', async () => { const counter = meter.createCounter('counter_total', { description: 'a test description', }); @@ -355,16 +298,10 @@ describe('PrometheusExporter', () => { counter.add(10, { counterKey1: 'attributeValue1' }); counter.add(20, { counterKey1: 'attributeValue2' }); counter.add(30, { counterKey1: 'attributeValue3' }); - meterProvider.shutdown().then(() => { - // exporter has been shut down along with meter provider. - http - .get('http://localhost:9464/metrics', res => { - errorHandler(done)(new Error('unreachable')); - }) - .on('error', (err: any) => { - assert.equal(err.code, 'ECONNREFUSED'); - done(); - }); + await meterProvider.shutdown(); + // exporter has been shut down along with meter provider. + await assert.rejects(request('http://localhost:9464/metrics'), { + code: 'ECONNREFUSED', }); }); @@ -547,124 +484,84 @@ describe('PrometheusExporter', () => { await exporter.shutdown(); }); - it('should use a configured name prefix', done => { - exporter = new PrometheusExporter( - { - prefix: 'test_prefix', - }, - async () => { - setup(exporter); - http - .get('http://localhost:9464/metrics', res => { - res.on('data', chunk => { - const body = chunk.toString(); - const lines = body.split('\n'); - - assert.deepStrictEqual(lines, [ - ...serializedDefaultResourceLines, - '# HELP test_prefix_counter_total description missing', - '# TYPE test_prefix_counter_total counter', - 'test_prefix_counter_total{key1="attributeValue1"} 10', - '', - ]); - - done(); - }); - }) - .on('error', errorHandler(done)); - } - ); + it('should use a configured name prefix', async () => { + exporter = new PrometheusExporter({ + prefix: 'test_prefix', + }); + setup(exporter); + const body = await request('http://localhost:9464/metrics'); + const lines = body.split('\n'); + + assert.deepStrictEqual(lines, [ + ...serializedDefaultResourceLines, + '# HELP test_prefix_counter_total description missing', + '# TYPE test_prefix_counter_total counter', + 'test_prefix_counter_total{key1="attributeValue1"} 10', + '', + ]); }); - it('should use a configured port', done => { - exporter = new PrometheusExporter( - { - port: 8080, - }, - async () => { - setup(exporter); - http - .get('http://localhost:8080/metrics', res => { - res.on('data', chunk => { - const body = chunk.toString(); - const lines = body.split('\n'); - - assert.deepStrictEqual(lines, [ - ...serializedDefaultResourceLines, - '# HELP counter_total description missing', - '# TYPE counter_total counter', - 'counter_total{key1="attributeValue1"} 10', - '', - ]); - - done(); - }); - }) - .on('error', errorHandler(done)); - } - ); + it('should use a configured port', async () => { + exporter = new PrometheusExporter({ + port: 8080, + }); + + setup(exporter); + const body = await request('http://localhost:8080/metrics'); + const lines = body.split('\n'); + + assert.deepStrictEqual(lines, [ + ...serializedDefaultResourceLines, + '# HELP counter_total description missing', + '# TYPE counter_total counter', + 'counter_total{key1="attributeValue1"} 10', + '', + ]); }); - it('should use a configured endpoint', done => { - exporter = new PrometheusExporter( - { - endpoint: '/test', - }, - async () => { - setup(exporter); - http - .get('http://localhost:9464/test', res => { - res.on('data', chunk => { - const body = chunk.toString(); - const lines = body.split('\n'); - - assert.deepStrictEqual(lines, [ - ...serializedDefaultResourceLines, - '# HELP counter_total description missing', - '# TYPE counter_total counter', - 'counter_total{key1="attributeValue1"} 10', - '', - ]); - - done(); - }); - }) - .on('error', errorHandler(done)); - } - ); + it('should use a configured endpoint', async () => { + exporter = new PrometheusExporter({ + endpoint: '/test', + }); + + setup(exporter); + const body = await request('http://localhost:9464/test'); + const lines = body.split('\n'); + + assert.deepStrictEqual(lines, [ + ...serializedDefaultResourceLines, + '# HELP counter_total description missing', + '# TYPE counter_total counter', + 'counter_total{key1="attributeValue1"} 10', + '', + ]); }); - it('should export a metric with timestamp', done => { - exporter = new PrometheusExporter( - { - appendTimestamp: true, - }, - async () => { - setup(exporter); - http - .get('http://localhost:9464/metrics', res => { - res.on('data', chunk => { - const body = chunk.toString(); - const lines = body.split('\n'); - - assert.deepStrictEqual(lines, [ - ...serializedDefaultResourceLines, - '# HELP counter_total description missing', - '# TYPE counter_total counter', - `counter_total{key1="attributeValue1"} 10 ${mockedHrTimeMs}`, - '', - ]); - - done(); - }); - }) - .on('error', errorHandler(done)); - } - ); + it('should export a metric with timestamp', async () => { + exporter = new PrometheusExporter({ + appendTimestamp: true, + }); + setup(exporter); + const body = await request('http://localhost:9464/metrics'); + const lines = body.split('\n'); + + assert.deepStrictEqual(lines, [ + ...serializedDefaultResourceLines, + '# HELP counter_total description missing', + '# TYPE counter_total counter', + `counter_total{key1="attributeValue1"} 10 ${mockedHrTimeMs}`, + '', + ]); }); }); }); +class RequestStatusError extends Error { + constructor(public statusCode: number | undefined) { + super('request failed with non-200 code'); + } +} + function request(url: string): Promise { return new Promise((resolve, reject) => { http @@ -677,7 +574,7 @@ function request(url: string): Promise { }); res.on('end', () => { if (res.statusCode !== 200) { - reject(new Error('request failed with non-200 code')); + reject(new RequestStatusError(res.statusCode)); return; } resolve(result); @@ -686,7 +583,3 @@ function request(url: string): Promise { .on('error', reject); }); } - -function errorHandler(done: Mocha.Done): (err: Error) => void { - return err => done(err); -} diff --git a/experimental/packages/opentelemetry-sdk-node/README.md b/experimental/packages/opentelemetry-sdk-node/README.md index a7da74decee..ac828b371cd 100644 --- a/experimental/packages/opentelemetry-sdk-node/README.md +++ b/experimental/packages/opentelemetry-sdk-node/README.md @@ -47,7 +47,7 @@ const { } = require("@opentelemetry/auto-instrumentations-node"); const jaegerExporter = new JaegerExporter(); -const prometheusExporter = new PrometheusExporter({ startServer: true }); +const prometheusExporter = new PrometheusExporter(); const sdk = new opentelemetry.NodeSDK({ // Optional - if omitted, the tracing SDK will be initialized from environment variables