feat: make prometheus config preventServerStart optional #1375

legendecas · 2020-08-03T03:28:50Z

Which problem is this PR solving?

There are a couple of reasons to leave the PrometheusExporterConfig.startServer false by default. First, it is async so we have another call that a user can do asynchronously so they can ensure it actually started. Second, it grabs a port on the host computer. This type of thing is best left explicit so the user is not surprised.

However, by most situations (and in examples), the startServer config was mostly set to true. And it is quite easy to forget to set the config and failed to find the Prometheus working as expected. In this situation, it can be very helpful to start the server by default and provide an option to prevent the server from starting (grabbing a port from the host).

Short description of the changes

the config startServer is replaced by preventServerStart.

codecov · 2020-08-03T03:33:17Z

Codecov Report

Merging #1375 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1375   +/-   ##
=======================================
  Coverage   93.18%   93.18%           
=======================================
  Files         156      156           
  Lines        4854     4854           
  Branches      986      986           
=======================================
  Hits         4523     4523           
  Misses        331      331

Impacted Files	Coverage Δ
...etry-exporter-prometheus/src/PrometheusExporter.ts	`90.16% <100.00%> (ø)`

packages/opentelemetry-exporter-prometheus/src/prometheus.ts

…-option

dyladan · 2020-08-05T02:56:15Z

packages/opentelemetry-exporter-prometheus/src/export/types.ts

   */
-  startServer?: boolean;
+  startServer: boolean;


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

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

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

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.

packages/opentelemetry-exporter-prometheus/src/prometheus.ts

obecny

I would argue that boolean parameter should be ever required. By default anything what is boolean and is not defined is simply false, we should not change it, In js if user doesn't define config we will have "an object undefined exception", I don't see any reason why we should have it. And just writing everywhere

startServer: false

doesn't look very intuitive why would you even want that

obecny · 2020-08-07T01:10:09Z

packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts

@@ -68,17 +68,13 @@ describe('PrometheusExporter', () => {
        }
      );
    });
-
-    it('should not start the server by default', () => {


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 ?

obecny · 2020-08-07T01:12:08Z

packages/opentelemetry-exporter-prometheus/src/prometheus.ts

@@ -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) {


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.

@legendecas any update here?

dyladan · 2020-08-07T13:22:04Z

By default anything what is boolean and is not defined is simply false

Maybe a better option is to rename startServer to preventServerStart or something?

dyladan · 2020-09-23T16:24:54Z

I would argue that boolean parameter should be ever required. By default anything what is boolean and is not defined is simply false, we should not change it, In js if user doesn't define config we will have "an object undefined exception", I don't see any reason why we should have it. And just writing everywhere
startServer: false
doesn't look very intuitive why would you even want that

@legendecas what do you think about inverting the name to preventStartServer or similar?

naseemkullah · 2020-09-24T01:35:51Z

By default anything what is boolean and is not defined is simply false

Maybe a better option is to rename startServer to preventServerStart or something?

serverless? 😄

legendecas · 2020-09-24T05:29:25Z

@dyladan what do you think about inverting the name to preventStartServer or similar?

I'm good with that, will do a quick update late today. But I'm wondering what the use case will be if we set preventStartServer: true?

dyladan · 2020-09-24T14:00:08Z

@legendecas we could just remove the option entirely, but then there is no way for the user to stop the exporter from grabbing a port. The usecase is admittedly entirely theoretical, but one could imagine a user that wants to defer server start until some precondition is met like waiting for a port to become available.

# Conflicts: # packages/opentelemetry-exporter-prometheus/src/prometheus.ts # packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts

legendecas · 2020-09-24T17:36:00Z

@dyladan @obecny @naseemkullah @mayurkale22 the startServer option has been replaced by preventServerStart, which is false by default -- i.e. starting server by default. PTAL :)

obecny

lgtm

obecny · 2020-09-24T17:53:34Z

is this still a breaking change now @dyladan ?

…elemetry#1375) * fix(instrumentation-fs): fix instrumentation of `fs/promises` * Revert "fix(instrumentation-fs): fix instrumentation of `fs/promises`" This reverts commit 90d9f0d68960ef647bbe5b7347ad6c97f936651e. * fix(instrumentation-fs): fix instrumentation of `fs/promises` * chore(instrumentation-fs): fix lint error * chore(instrumentation-fs): hard-code `R_OK` value for node 14 * chore(instrumentation-fs): fix supported version for `fs/promises` * chore(instrumentation-fs): incorporate changes from open-telemetry#1332 * chore(instrumentation-fs): consolidate common test utilities --------- Co-authored-by: Haddas Bronfman <[email protected]>

feat: make prometheus config startServer mandatory

2c62be3

legendecas requested review from dyladan, markwolff, mayurkale22, mwear, naseemkullah, obecny, OlivierAlbertini and vmarchaud as code owners August 3, 2020 03:28

fix: degrade log level on startServer is false

66dd10b

dyladan reviewed Aug 4, 2020

View reviewed changes

packages/opentelemetry-exporter-prometheus/src/prometheus.ts Outdated Show resolved Hide resolved

legendecas added 2 commits August 5, 2020 10:48

fix: remove the log on not starting server

cb03cd2

Merge remote-tracking branch 'upstream/master' into prom-start-server…

cc77819

…-option

dyladan reviewed Aug 5, 2020

View reviewed changes

packages/opentelemetry-exporter-prometheus/src/prometheus.ts Outdated Show resolved Hide resolved

fix: callbacks shall be called asynchronously unconditionally

6387935

dyladan approved these changes Aug 5, 2020

View reviewed changes

naseemkullah approved these changes Aug 5, 2020

View reviewed changes

Merge branch 'master' into prom-start-server-option

e75b4b6

mayurkale22 approved these changes Aug 6, 2020

View reviewed changes

dyladan added the breaking label Aug 6, 2020

obecny reviewed Aug 7, 2020

View reviewed changes

legendecas added 2 commits September 25, 2020 01:08

Merge branch 'master' into prom-start-server-option

0bdea70

# Conflicts: # packages/opentelemetry-exporter-prometheus/src/prometheus.ts # packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts

fixup!: update the options with preventServerStart

c565286

obecny approved these changes Sep 24, 2020

View reviewed changes

obecny changed the title ~~feat: make prometheus config startServer mandatory~~ feat: make prometheus config preventServerStart optional Sep 24, 2020

Merge branch 'master' into prom-start-server-option

9e3d50d

obecny merged commit 24ffd16 into open-telemetry:master Sep 29, 2020

legendecas deleted the prom-start-server-option branch October 1, 2020 17:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: make prometheus config preventServerStart optional #1375

feat: make prometheus config preventServerStart optional #1375

legendecas commented Aug 3, 2020 •

edited

Loading

codecov bot commented Aug 3, 2020 •

edited

Loading

dyladan Aug 5, 2020

legendecas Aug 5, 2020

naseemkullah Sep 1, 2020

dyladan Sep 2, 2020

obecny left a comment

obecny Aug 7, 2020

obecny Aug 7, 2020

dyladan Sep 23, 2020

dyladan commented Aug 7, 2020

dyladan commented Sep 23, 2020

naseemkullah commented Sep 24, 2020

legendecas commented Sep 24, 2020

dyladan commented Sep 24, 2020

legendecas commented Sep 24, 2020 •

edited

Loading

obecny left a comment

obecny commented Sep 24, 2020

feat: make prometheus config preventServerStart optional #1375

feat: make prometheus config preventServerStart optional #1375

Conversation

legendecas commented Aug 3, 2020 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Aug 3, 2020 • edited Loading

Codecov Report

dyladan Aug 5, 2020

Choose a reason for hiding this comment

legendecas Aug 5, 2020

Choose a reason for hiding this comment

naseemkullah Sep 1, 2020

Choose a reason for hiding this comment

dyladan Sep 2, 2020

Choose a reason for hiding this comment

obecny left a comment

Choose a reason for hiding this comment

obecny Aug 7, 2020

Choose a reason for hiding this comment

obecny Aug 7, 2020

Choose a reason for hiding this comment

dyladan Sep 23, 2020

Choose a reason for hiding this comment

dyladan commented Aug 7, 2020

dyladan commented Sep 23, 2020

naseemkullah commented Sep 24, 2020

legendecas commented Sep 24, 2020

dyladan commented Sep 24, 2020

legendecas commented Sep 24, 2020 • edited Loading

obecny left a comment

Choose a reason for hiding this comment

obecny commented Sep 24, 2020

legendecas commented Aug 3, 2020 •

edited

Loading

codecov bot commented Aug 3, 2020 •

edited

Loading

legendecas commented Sep 24, 2020 •

edited

Loading