From 2c62be3479ffdf6389454527f20e7f63fb6fbe76 Mon Sep 17 00:00:00 2001 From: legendecas Date: Mon, 3 Aug 2020 11:21:15 +0800 Subject: [PATCH 1/5] feat: make prometheus config startServer mandatory --- .../src/export/types.ts | 3 +-- .../src/prometheus.ts | 7 +++++-- .../test/prometheus.test.ts | 21 +++++++++++-------- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/packages/opentelemetry-exporter-prometheus/src/export/types.ts b/packages/opentelemetry-exporter-prometheus/src/export/types.ts index d4e320ecdb..693d0536fd 100644 --- a/packages/opentelemetry-exporter-prometheus/src/export/types.ts +++ b/packages/opentelemetry-exporter-prometheus/src/export/types.ts @@ -44,9 +44,8 @@ export interface ExporterConfig { /** * Define if the Prometheus exporter server will be started - * @default false */ - startServer?: boolean; + startServer: boolean; /** Standard logging interface */ logger?: api.Logger; diff --git a/packages/opentelemetry-exporter-prometheus/src/prometheus.ts b/packages/opentelemetry-exporter-prometheus/src/prometheus.ts index af0c551dba..6a5d7d098e 100644 --- a/packages/opentelemetry-exporter-prometheus/src/prometheus.ts +++ b/packages/opentelemetry-exporter-prometheus/src/prometheus.ts @@ -59,7 +59,7 @@ export class PrometheusExporter implements MetricExporter { * @param config Exporter configuration * @param callback Callback to be called after a server was started */ - constructor(config: ExporterConfig = {}, callback?: () => void) { + constructor(config: ExporterConfig, callback?: () => void) { this._logger = config.logger || new NoopLogger(); this._port = config.port || PrometheusExporter.DEFAULT_OPTIONS.port; this._prefix = config.prefix || PrometheusExporter.DEFAULT_OPTIONS.prefix; @@ -69,9 +69,12 @@ export class PrometheusExporter implements MetricExporter { config.endpoint || PrometheusExporter.DEFAULT_OPTIONS.endpoint ).replace(/^([^/])/, '/$1'); - if (config.startServer || PrometheusExporter.DEFAULT_OPTIONS.startServer) { + if (config.startServer) { this.startServer(callback); } else if (callback) { + this._logger.info( + 'Prometheus exporter server is not started. The server can be started with the first construction parameter of PrometheusExporter to be `true`.' + ); callback(); } } diff --git a/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts b/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts index ba499de3f5..4425005507 100644 --- a/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts +++ b/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts @@ -44,7 +44,7 @@ describe('PrometheusExporter', () => { }); describe('constructor', () => { it('should construct an exporter', () => { - const exporter = new PrometheusExporter(); + const exporter = new PrometheusExporter({ startServer: false }); assert.ok(typeof exporter.startServer === 'function'); assert.ok(typeof exporter.shutdown === 'function'); }); @@ -67,17 +67,13 @@ describe('PrometheusExporter', () => { } ); }); - - it('should not start the server by default', () => { - const exporter = new PrometheusExporter(); - assert.ok(exporter['_server']!.listening === false); - }); }); describe('server', () => { it('it should start on startServer() and call the callback', done => { const exporter = new PrometheusExporter({ port: 9722, + startServer: false, }); exporter.startServer(() => { exporter.shutdown(() => { @@ -89,7 +85,7 @@ describe('PrometheusExporter', () => { it('it should listen on the default port and default endpoint', done => { const port = PrometheusExporter.DEFAULT_OPTIONS.port; const endpoint = PrometheusExporter.DEFAULT_OPTIONS.endpoint; - const exporter = new PrometheusExporter(); + const exporter = new PrometheusExporter({ startServer: false }); exporter.startServer(() => { const url = `http://localhost:${port}${endpoint}`; @@ -109,6 +105,7 @@ describe('PrometheusExporter', () => { const exporter = new PrometheusExporter({ port, endpoint, + startServer: false, }); exporter.startServer(() => { @@ -129,6 +126,7 @@ describe('PrometheusExporter', () => { const exporter = new PrometheusExporter({ port, endpoint, + startServer: false, }); exporter.startServer(() => { @@ -139,6 +137,7 @@ describe('PrometheusExporter', () => { const exporter2 = new PrometheusExporter({ port, endpoint: `/${endpoint}`, + startServer: false, }); exporter2.startServer(() => { @@ -161,6 +160,7 @@ describe('PrometheusExporter', () => { const exporter = new PrometheusExporter({ port, endpoint, + startServer: false, }); exporter.startServer(() => { const url = `http://localhost:${port}/invalid`; @@ -175,7 +175,7 @@ describe('PrometheusExporter', () => { }); it('should call a provided callback regardless of if the server is running', done => { - const exporter = new PrometheusExporter(); + const exporter = new PrometheusExporter({ startServer: false }); exporter.shutdown(() => { return done(); }); @@ -187,7 +187,7 @@ describe('PrometheusExporter', () => { let meter: Meter; beforeEach(done => { - exporter = new PrometheusExporter(); + exporter = new PrometheusExporter({ startServer: false }); meter = new MeterProvider().getMeter('test-prometheus'); exporter.startServer(done); }); @@ -441,6 +441,7 @@ describe('PrometheusExporter', () => { it('should use a configured name prefix', done => { exporter = new PrometheusExporter({ prefix: 'test_prefix', + startServer: false, }); exporter.startServer(async () => { @@ -470,6 +471,7 @@ describe('PrometheusExporter', () => { it('should use a configured port', done => { exporter = new PrometheusExporter({ port: 8080, + startServer: false, }); exporter.startServer(async () => { @@ -499,6 +501,7 @@ describe('PrometheusExporter', () => { it('should use a configured endpoint', done => { exporter = new PrometheusExporter({ endpoint: '/test', + startServer: false, }); exporter.startServer(async () => { From 66dd10b5e2b5a2f24cac306416257763098508d1 Mon Sep 17 00:00:00 2001 From: legendecas Date: Mon, 3 Aug 2020 11:47:05 +0800 Subject: [PATCH 2/5] fix: degrade log level on startServer is false --- .../opentelemetry-exporter-prometheus/src/prometheus.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/opentelemetry-exporter-prometheus/src/prometheus.ts b/packages/opentelemetry-exporter-prometheus/src/prometheus.ts index 6a5d7d098e..123ded4ec3 100644 --- a/packages/opentelemetry-exporter-prometheus/src/prometheus.ts +++ b/packages/opentelemetry-exporter-prometheus/src/prometheus.ts @@ -71,11 +71,11 @@ export class PrometheusExporter implements MetricExporter { if (config.startServer) { this.startServer(callback); - } else if (callback) { - this._logger.info( + } else { + this._logger.debug( 'Prometheus exporter server is not started. The server can be started with the first construction parameter of PrometheusExporter to be `true`.' ); - callback(); + callback?.(); } } From cb03cd26c92c8d05a12033b18160b283dec4ce92 Mon Sep 17 00:00:00 2001 From: legendecas Date: Wed, 5 Aug 2020 10:48:51 +0800 Subject: [PATCH 3/5] fix: remove the log on not starting server --- packages/opentelemetry-exporter-prometheus/src/prometheus.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/opentelemetry-exporter-prometheus/src/prometheus.ts b/packages/opentelemetry-exporter-prometheus/src/prometheus.ts index 123ded4ec3..8d71ca7ea7 100644 --- a/packages/opentelemetry-exporter-prometheus/src/prometheus.ts +++ b/packages/opentelemetry-exporter-prometheus/src/prometheus.ts @@ -37,7 +37,6 @@ import { ExporterConfig } from './export/types'; export class PrometheusExporter implements MetricExporter { static readonly DEFAULT_OPTIONS = { port: 9464, - startServer: false, endpoint: '/metrics', prefix: '', }; @@ -72,9 +71,6 @@ export class PrometheusExporter implements MetricExporter { if (config.startServer) { this.startServer(callback); } else { - this._logger.debug( - 'Prometheus exporter server is not started. The server can be started with the first construction parameter of PrometheusExporter to be `true`.' - ); callback?.(); } } From 63879357e18a863187ec912c2b6640bd5825bf31 Mon Sep 17 00:00:00 2001 From: legendecas Date: Wed, 5 Aug 2020 12:31:39 +0800 Subject: [PATCH 4/5] fix: callbacks shall be called asynchronously unconditionally --- packages/opentelemetry-exporter-prometheus/src/prometheus.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/opentelemetry-exporter-prometheus/src/prometheus.ts b/packages/opentelemetry-exporter-prometheus/src/prometheus.ts index 8d71ca7ea7..d045613fab 100644 --- a/packages/opentelemetry-exporter-prometheus/src/prometheus.ts +++ b/packages/opentelemetry-exporter-prometheus/src/prometheus.ts @@ -70,8 +70,8 @@ export class PrometheusExporter implements MetricExporter { if (config.startServer) { this.startServer(callback); - } else { - callback?.(); + } else if (callback) { + process.nextTick(callback); } } From c56528695f618943279b91fee010e146d23a2fce Mon Sep 17 00:00:00 2001 From: legendecas Date: Fri, 25 Sep 2020 01:29:02 +0800 Subject: [PATCH 5/5] fixup!: update the options with `preventServerStart` --- .../src/PrometheusExporter.ts | 3 +- .../src/export/types.ts | 5 +- .../test/PrometheusExporter.test.ts | 348 ++++++++---------- 3 files changed, 163 insertions(+), 193 deletions(-) diff --git a/packages/opentelemetry-exporter-prometheus/src/PrometheusExporter.ts b/packages/opentelemetry-exporter-prometheus/src/PrometheusExporter.ts index 0812fb7419..f8c9c85370 100644 --- a/packages/opentelemetry-exporter-prometheus/src/PrometheusExporter.ts +++ b/packages/opentelemetry-exporter-prometheus/src/PrometheusExporter.ts @@ -26,7 +26,6 @@ import { PrometheusLabelsBatcher } from './PrometheusLabelsBatcher'; export class PrometheusExporter implements MetricExporter { static readonly DEFAULT_OPTIONS = { port: 9464, - startServer: false, endpoint: '/metrics', prefix: '', }; @@ -59,7 +58,7 @@ export class PrometheusExporter implements MetricExporter { config.endpoint || PrometheusExporter.DEFAULT_OPTIONS.endpoint ).replace(/^([^/])/, '/$1'); - if (config.startServer || PrometheusExporter.DEFAULT_OPTIONS.startServer) { + if (config.preventServerStart !== true) { this.startServer().then(callback); } else if (callback) { callback(); diff --git a/packages/opentelemetry-exporter-prometheus/src/export/types.ts b/packages/opentelemetry-exporter-prometheus/src/export/types.ts index 693d0536fd..99574d13c6 100644 --- a/packages/opentelemetry-exporter-prometheus/src/export/types.ts +++ b/packages/opentelemetry-exporter-prometheus/src/export/types.ts @@ -43,9 +43,10 @@ export interface ExporterConfig { port?: number; /** - * Define if the Prometheus exporter server will be started + * Prevent the Prometheus exporter server from starting + * @default false */ - startServer: boolean; + preventServerStart?: boolean; /** Standard logging interface */ logger?: api.Logger; diff --git a/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts b/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts index 34322c8396..8936da63ab 100644 --- a/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts +++ b/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts @@ -32,29 +32,30 @@ describe('PrometheusExporter', () => { mockAggregator(MinMaxLastSumCountAggregator); describe('constructor', () => { - it('should construct an exporter', () => { - const exporter = new PrometheusExporter({ startServer: false }); + it('should construct an exporter', done => { + const exporter = new PrometheusExporter(); assert.ok(typeof exporter.startServer === 'function'); assert.ok(typeof exporter.shutdown === 'function'); + exporter.shutdown().then(done); }); - it('should start the server if startServer is passed as an option', done => { + it('should start the server by default and call the callback', done => { const port = PrometheusExporter.DEFAULT_OPTIONS.port; const endpoint = PrometheusExporter.DEFAULT_OPTIONS.endpoint; - const exporter = new PrometheusExporter( - { - startServer: true, - }, - () => { - 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}`; + http.get(url, (res: any) => { + assert.strictEqual(res.statusCode, 200); + exporter.shutdown().then(() => { + return done(); }); - } - ); + }); + }); + }); + + it('should not start the server if preventServerStart is passed as an option', () => { + const exporter = new PrometheusExporter({ preventServerStart: true }); + assert.ok(exporter['_server'].listening === false); }); }); @@ -62,7 +63,7 @@ describe('PrometheusExporter', () => { it('it should start on startServer() and call the callback', done => { const exporter = new PrometheusExporter({ port: 9722, - startServer: false, + preventServerStart: true, }); exporter.startServer().then(() => { exporter.shutdown().then(() => { @@ -74,9 +75,7 @@ describe('PrometheusExporter', () => { it('it should listen on the default port and default endpoint', done => { const port = PrometheusExporter.DEFAULT_OPTIONS.port; const endpoint = PrometheusExporter.DEFAULT_OPTIONS.endpoint; - const exporter = new PrometheusExporter({ startServer: false }); - - exporter.startServer().then(() => { + const exporter = new PrometheusExporter({}, () => { const url = `http://localhost:${port}${endpoint}`; http.get(url, (res: any) => { assert.strictEqual(res.statusCode, 200); @@ -91,80 +90,81 @@ describe('PrometheusExporter', () => { const port = 9991; const endpoint = '/metric'; - const exporter = new PrometheusExporter({ - port, - endpoint, - startServer: false, - }); - - exporter.startServer().then(() => { - 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}`; + http.get(url, (res: any) => { + assert.strictEqual(res.statusCode, 200); + exporter.shutdown().then(() => { + return done(); + }); }); - }); - }); + } + ); }); it('it should not require endpoints to start with a slash', done => { const port = 9991; const endpoint = 'metric'; - const exporter = new PrometheusExporter({ - port, - endpoint, - startServer: false, - }); - - exporter.startServer().then(() => { - 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}`, - startServer: false, - }); - - exporter2.startServer().then(() => { - 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, + }, + () => { + 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(); + }); + }); + } + ); }); }); - }); - }); + } + ); }); it('it should return a HTTP status 404 if the endpoint does not match', done => { const port = 9912; const endpoint = '/metrics'; - const exporter = new PrometheusExporter({ - port, - endpoint, - startServer: false, - }); - exporter.startServer().then(() => { - const url = `http://localhost:${port}/invalid`; + 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(); + http.get(url, (res: any) => { + assert.strictEqual(res.statusCode, 404); + exporter.shutdown().then(() => { + return done(); + }); }); - }); - }); + } + ); }); - it('should call a provided callback regardless of if the server is running', done => { - const exporter = new PrometheusExporter(); + it('should call a provided callback on shutdown regardless of if the server is running', done => { + const exporter = new PrometheusExporter({ preventServerStart: true }); exporter.shutdown().then(() => { return done(); }); @@ -177,14 +177,15 @@ describe('PrometheusExporter', () => { let meter: Meter; beforeEach(done => { - exporter = new PrometheusExporter(); - meterProvider = new MeterProvider({ - interval: Math.pow(2, 31) - 1, - }); - meter = meterProvider.getMeter('test-prometheus', '1', { - exporter: exporter, + exporter = new PrometheusExporter({}, () => { + meterProvider = new MeterProvider({ + interval: Math.pow(2, 31) - 1, + }); + meter = meterProvider.getMeter('test-prometheus', '1', { + exporter, + }); + done(); }); - exporter.startServer().then(done); }); afterEach(done => { @@ -341,37 +342,6 @@ describe('PrometheusExporter', () => { }); }); - it('should export multiple labels on manual shutdown', done => { - const counter = meter.createCounter('counter', { - description: 'a test description', - }) as CounterMetric; - - counter.bind({ counterKey1: 'labelValue1' }).add(10); - counter.bind({ counterKey1: 'labelValue2' }).add(20); - counter.bind({ counterKey1: 'labelValue3' }).add(30); - meterProvider.shutdown().then(() => { - http - .get('http://localhost:9464/metrics', res => { - res.on('data', chunk => { - const body = chunk.toString(); - const lines = body.split('\n'); - - assert.deepStrictEqual(lines, [ - '# HELP counter a test description', - '# TYPE counter counter', - `counter{counterKey1="labelValue1"} 10 ${mockedHrTimeMs}`, - `counter{counterKey1="labelValue2"} 20 ${mockedHrTimeMs}`, - `counter{counterKey1="labelValue3"} 30 ${mockedHrTimeMs}`, - '', - ]); - - done(); - }); - }) - .on('error', errorHandler(done)); - }); - }); - it('should export a comment if no metrics are registered', done => { exporter.export([], () => { http @@ -605,93 +575,93 @@ describe('PrometheusExporter', () => { }); it('should use a configured name prefix', done => { - exporter = new PrometheusExporter({ - prefix: 'test_prefix', - startServer: false, - }); - - exporter.startServer().then(async () => { - await meter.collect(); - exporter!.export(meter.getBatcher().checkPointSet(), () => { - http - .get('http://localhost:9464/metrics', res => { - res.on('data', chunk => { - const body = chunk.toString(); - const lines = body.split('\n'); + exporter = new PrometheusExporter( + { + prefix: 'test_prefix', + }, + async () => { + await meter.collect(); + exporter!.export(meter.getBatcher().checkPointSet(), () => { + http + .get('http://localhost:9464/metrics', res => { + res.on('data', chunk => { + const body = chunk.toString(); + const lines = body.split('\n'); - assert.deepStrictEqual(lines, [ - '# HELP test_prefix_counter description missing', - '# TYPE test_prefix_counter counter', - `test_prefix_counter{key1="labelValue1"} 10 ${mockedHrTimeMs}`, - '', - ]); + assert.deepStrictEqual(lines, [ + '# HELP test_prefix_counter description missing', + '# TYPE test_prefix_counter counter', + `test_prefix_counter{key1="labelValue1"} 10 ${mockedHrTimeMs}`, + '', + ]); - done(); - }); - }) - .on('error', errorHandler(done)); - }); - }); + done(); + }); + }) + .on('error', errorHandler(done)); + }); + } + ); }); it('should use a configured port', done => { - exporter = new PrometheusExporter({ - port: 8080, - startServer: false, - }); - - exporter.startServer().then(async () => { - await meter.collect(); - exporter!.export(meter.getBatcher().checkPointSet(), () => { - http - .get('http://localhost:8080/metrics', res => { - res.on('data', chunk => { - const body = chunk.toString(); - const lines = body.split('\n'); + exporter = new PrometheusExporter( + { + port: 8080, + }, + async () => { + await meter.collect(); + exporter!.export(meter.getBatcher().checkPointSet(), () => { + http + .get('http://localhost:8080/metrics', res => { + res.on('data', chunk => { + const body = chunk.toString(); + const lines = body.split('\n'); - assert.deepStrictEqual(lines, [ - '# HELP counter description missing', - '# TYPE counter counter', - `counter{key1="labelValue1"} 10 ${mockedHrTimeMs}`, - '', - ]); + assert.deepStrictEqual(lines, [ + '# HELP counter description missing', + '# TYPE counter counter', + `counter{key1="labelValue1"} 10 ${mockedHrTimeMs}`, + '', + ]); - done(); - }); - }) - .on('error', errorHandler(done)); - }); - }); + done(); + }); + }) + .on('error', errorHandler(done)); + }); + } + ); }); it('should use a configured endpoint', done => { - exporter = new PrometheusExporter({ - endpoint: '/test', - startServer: false, - }); - - exporter.startServer().then(async () => { - await meter.collect(); - exporter!.export(meter.getBatcher().checkPointSet(), () => { - http - .get('http://localhost:9464/test', res => { - res.on('data', chunk => { - const body = chunk.toString(); - const lines = body.split('\n'); + exporter = new PrometheusExporter( + { + endpoint: '/test', + }, + async () => { + await meter.collect(); + exporter!.export(meter.getBatcher().checkPointSet(), () => { + http + .get('http://localhost:9464/test', res => { + res.on('data', chunk => { + const body = chunk.toString(); + const lines = body.split('\n'); - assert.deepStrictEqual(lines, [ - '# HELP counter description missing', - '# TYPE counter counter', - `counter{key1="labelValue1"} 10 ${mockedHrTimeMs}`, - '', - ]); + assert.deepStrictEqual(lines, [ + '# HELP counter description missing', + '# TYPE counter counter', + `counter{key1="labelValue1"} 10 ${mockedHrTimeMs}`, + '', + ]); - done(); - }); - }) - .on('error', errorHandler(done)); - }); - }); + done(); + }); + }) + .on('error', errorHandler(done)); + }); + } + ); }); }); });