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

feat: make prometheus config preventServerStart optional #1375

Merged
merged 9 commits into from
Sep 29, 2020
Merged

feat: make prometheus config preventServerStart optional #1375

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2c62be3
feat: make prometheus config startServer mandatory
legendecas Aug 3, 2020
66dd10b
fix: degrade log level on startServer is false
legendecas Aug 3, 2020
cb03cd2
fix: remove the log on not starting server
legendecas Aug 5, 2020
cc77819
Merge remote-tracking branch 'upstream/master' into prom-start-server…
legendecas Aug 5, 2020
6387935
fix: callbacks shall be called asynchronously unconditionally
legendecas Aug 5, 2020
e75b4b6
Merge branch 'master' into prom-start-server-option
legendecas Aug 6, 2020
0bdea70
Merge branch 'master' into prom-start-server-option
legendecas Sep 24, 2020
c565286
fixup!: update the options with `preventServerStart`
legendecas Sep 24, 2020
9e3d50d
Merge branch 'master' into prom-start-server-option
obecny Sep 29, 2020
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
3 changes: 1 addition & 2 deletions packages/opentelemetry-exporter-prometheus/src/export/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,8 @@ export interface ExporterConfig {

/**
* Define if the Prometheus exporter server will be started
* @default false
*/
startServer?: boolean;
startServer: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you make this configuration option required? Should it not just default to true?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason startServer defaults to false have been stated at the description of the PR "it grabs a port on the host computer. This type of thing is best left explicit so the user is not surprised" -- which is copy-pasted from previous Gitter chat history I considered as a fair reason (unfortunately Gitter doesn't provide chat history links).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am leaning towards default to true, prometheus is meant to be scraped in most cases. The default port of 9464 is meant for this as per https://github.com/prometheus/prometheus/wiki/Default-port-allocations

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the gitter chat @legendecas referenced was actually me :)

Think I see this one both ways. It is unfortunate that we have to grab a port, but the exporter really is useless without it so I would say it's probably ok to just do it.


/** Standard logging interface */
logger?: api.Logger;
Expand Down
7 changes: 3 additions & 4 deletions packages/opentelemetry-exporter-prometheus/src/prometheus.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: '',
};
Expand All @@ -59,7 +58,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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should not be changed, if in js user will not define any config we don't want the error "object is undefined" to be thrown.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@legendecas any update here?

this._logger = config.logger || new NoopLogger();
this._port = config.port || PrometheusExporter.DEFAULT_OPTIONS.port;
this._prefix = config.prefix || PrometheusExporter.DEFAULT_OPTIONS.prefix;
Expand All @@ -69,10 +68,10 @@ 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) {
callback();
process.nextTick(callback);
}
}

Expand Down
21 changes: 12 additions & 9 deletions packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,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');
});
Expand All @@ -68,17 +68,13 @@ describe('PrometheusExporter', () => {
}
);
});

it('should not start the server by default', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test is still relevant , if user doesn't provide anything (when using in pure js for example) the server should not start right ?

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(() => {
Expand All @@ -90,7 +86,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}`;
Expand All @@ -110,6 +106,7 @@ describe('PrometheusExporter', () => {
const exporter = new PrometheusExporter({
port,
endpoint,
startServer: false,
});

exporter.startServer(() => {
Expand All @@ -130,6 +127,7 @@ describe('PrometheusExporter', () => {
const exporter = new PrometheusExporter({
port,
endpoint,
startServer: false,
});

exporter.startServer(() => {
Expand All @@ -140,6 +138,7 @@ describe('PrometheusExporter', () => {
const exporter2 = new PrometheusExporter({
port,
endpoint: `/${endpoint}`,
startServer: false,
});

exporter2.startServer(() => {
Expand All @@ -162,6 +161,7 @@ describe('PrometheusExporter', () => {
const exporter = new PrometheusExporter({
port,
endpoint,
startServer: false,
});
exporter.startServer(() => {
const url = `http://localhost:${port}/invalid`;
Expand All @@ -176,7 +176,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();
});
Expand All @@ -188,7 +188,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);
});
Expand Down Expand Up @@ -442,6 +442,7 @@ describe('PrometheusExporter', () => {
it('should use a configured name prefix', done => {
exporter = new PrometheusExporter({
prefix: 'test_prefix',
startServer: false,
});

exporter.startServer(async () => {
Expand Down Expand Up @@ -471,6 +472,7 @@ describe('PrometheusExporter', () => {
it('should use a configured port', done => {
exporter = new PrometheusExporter({
port: 8080,
startServer: false,
});

exporter.startServer(async () => {
Expand Down Expand Up @@ -500,6 +502,7 @@ describe('PrometheusExporter', () => {
it('should use a configured endpoint', done => {
exporter = new PrometheusExporter({
endpoint: '/test',
startServer: false,
});

exporter.startServer(async () => {
Expand Down