Add Graceful Shutdown Support by jonahrosenblum · Pull Request #1321 · open-telemetry/opentelemetry-js

jonahrosenblum · 2020-07-16T19:00:14Z

Which problem is this PR solving?

Cloud Run Graceful Termination Support #1290

Short description of the changes

Add event handlers to both Trace & Meter providers to call shutdown methods when the process receives a SIGTERM.
Add unit tests to demonstrate how the shutdown methods are called and that exports are successful
Modify push controller's collect method to wait for collection to happen before export (I think this may fix an undocumented bug?)
Modify config file to allow graceful shutdown to be turned off, by default it is enabled.

packages/opentelemetry-metrics/src/MeterProvider.ts

dyladan · 2020-07-16T19:02:53Z

packages/opentelemetry-metrics/src/export/Controller.ts

-  private _collect() {
-    this._meter.collect();
+  shutdown() {
+    this._collect();


Why does shutdown call an async collect but not await it?

Could you give an example of when this could have negative side effects? I was under the impression that async functions could continue to run even if the parent function had finished.

It's not that it will have a negative side-effect, but that some users may want to manually call shutdown and wait for it to finish.

I can make shutdown async as well and add awaits so users can wait for it to finish if they call it manually.

dyladan · 2020-07-16T19:03:24Z

packages/opentelemetry-metrics/src/MeterProvider.ts

+
+  shutdown(): void {
+    if (this._config['exporter']) {
+      this._config['exporter'].shutdown();


I believe exporter shutdown takes a callback param? how will we know when its done?

Is this another case where a user may want to call shutdown manually?

yes. I can't think of a reason not to support the use-case

Metric exporter does not take a callback param currently.

dyladan · 2020-07-16T19:04:03Z

packages/opentelemetry-tracing/src/BasicTracerProvider.ts

      logger: this.logger,
      resource: this.resource,
    });
+    if (this._config['gracefulShutdown']) {


why bracket notation?

I usually program in C/C++, I am not attached to any one notation and if the project does not use this I have no issue switching. Thanks for bringing this to my attention!

typically the bracket notation in typescript means you are accessing a private property

dyladan · 2020-07-16T19:04:18Z

packages/opentelemetry-tracing/src/BasicTracerProvider.ts

    }
  }
+
+  shutdown(): void {


no callback? not waiting for the shutdown to finish?

I had implemented all of this assuming users would not be calling shutdown manually. I can add a callback parameter if you think that would be a good feature.

What do you mean by "not waiting for the shutdown to finish"? I assumed shutdown was a blocking function. I added a callback option which will be passed to the shutdown function in the processor but do I need to make this async and add an await or should that be enough?

Issue with adding the callback option in these shutdown methods: any function used as a signal handler cannot have any args so adding callbacks is a bit tricky. We can get around this by adding @ts-ignore to ignore strict function typing but this may be ill advised? I compiled it using this and the test cases worked fine. Another solution I could try would be to have a private onShutdown function which is bound to the signal handler and a public shutdown function which accepts a callback - they would have identical functionality aside from the callback arg but this may look confusing as it requires duplicated code (I wish typescript had overloaded functions!)

I am strongly against ts-ignore

I ended up not using @ts-ignore, I also added callbacks and make the meter provider shutdown blocking.

codecov · 2020-07-16T19:04:54Z

Codecov Report

Merging #1321 into master will increase coverage by 0.09%.
The diff coverage is 78.57%.

@@            Coverage Diff             @@
##           master    #1321      +/-   ##
==========================================
+ Coverage   94.12%   94.21%   +0.09%     
==========================================
  Files         145      146       +1     
  Lines        4341     4377      +36     
  Branches      886      893       +7     
==========================================
+ Hits         4086     4124      +38     
+ Misses        255      253       -2

Impacted Files	Coverage Δ
packages/opentelemetry-metrics/src/types.ts	`100.00% <ø> (ø)`
packages/opentelemetry-tracing/src/config.ts	`100.00% <ø> (ø)`
...lemetry-core/src/platform/node/ShutdownNotifier.ts	`33.33% <33.33%> (ø)`
...ges/opentelemetry-metrics/src/export/Controller.ts	`76.19% <60.00%> (+9.52%)`	⬆️
...ackages/opentelemetry-metrics/src/MeterProvider.ts	`96.55% <92.85%> (-3.45%)`	⬇️
packages/opentelemetry-metrics/src/Meter.ts	`96.00% <100.00%> (+0.08%)`	⬆️
...s/opentelemetry-tracing/src/BasicTracerProvider.ts	`81.39% <100.00%> (+4.92%)`	⬆️
...es/opentelemetry-tracing/src/MultiSpanProcessor.ts	`100.00% <0.00%> (+9.09%)`	⬆️
...elemetry-tracing/src/export/ConsoleSpanExporter.ts	`100.00% <0.00%> (+15.38%)`	⬆️
... and 2 more

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

packages/opentelemetry-metrics/src/MeterProvider.ts

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

packages/opentelemetry-metrics/src/MeterProvider.ts

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

dyladan

Thanks for the changes

dyladan

process is not supported in web environments, which means this PR will make the meter provider not web compatible

obecny

The process is not part of browser and MeterProvider.ts is meant to be used in both environment, so you can't use it like this. If you want to implement mechanism that will execute something when process is closed you should mimic the same behaviour in browser (probably event unload on window). But to do that you should create some platform dependent mechanism. Put in inside core sdk in folder platform and create some easy to share function for example

// platform/browser/ShutdownNotifier.ts
export function onGlobalShutdown(callback) {
  window.addEventListener('unload', callback);
}

// platform/node/ShutdownNotifier.ts
export function onGlobalShutdown(callback) {
  process.once('SIGTERM', callback);
}


//and then in your code

onGlobalShutdown(function(){
  this._shutdownAllMeters.bind(this)
}.bind(this));

obecny

Last concerns around async / await / promise and callback - I added comments, after this I think it will be good to go, thx

packages/opentelemetry-core/src/platform/browser/ShutdownNotifier.ts

packages/opentelemetry-core/src/platform/node/ShutdownNotifier.ts

packages/opentelemetry-metrics/src/MeterProvider.ts

obecny · 2020-08-12T18:38:57Z

packages/opentelemetry-metrics/src/export/Controller.ts

+
+  private async _collect() {
+    await this._meter.collect();
+    await this._exporter.export(


"export" function from exporters return "void" there is no reason to call "await" in front of it as it will not block anything.
IF you really want to wait for exporter to finish you should create here a promise and resolve or reject it in a callback where you added "@todo

The todo was actually there before my time, I removed this await - you make a good point about it not being async.

but you can return a promise to be resolved when exporter exports something (or rejected depends on callback) so this way the _collect will really wait until the exporter finishes - what was the original plan I believe

jonahrosenblum · 2020-08-12T21:41:34Z

@obecny Hopefully this addresses your concerns.

obecny · 2020-08-12T21:50:16Z

packages/opentelemetry-metrics/src/MeterProvider.ts

      this._config.exporter.shutdown();
    }
-    await Promise.all(
+    return Promise.all(


this is fine, but you can remove async now from the function _shutdownAllMeters as nothing is async anymore here

obecny · 2020-08-12T22:00:05Z

packages/opentelemetry-metrics/src/export/Controller.ts

-        if (result !== ExportResult.SUCCESS) {
-          // @todo: log error
-        }
+    this._exporter.export(this._meter.getBatcher().checkPointSet(), result => {


Suggested change

this._exporter.export(this._meter.getBatcher().checkPointSet(), result => {

return new Promise((resolve, reject) => {

this._exporter.export(

this._meter.getBatcher().checkPointSet(),

result => {

if (result === ExportResult.SUCCESS) {

resolve();

} else {

reject();

}

}

);

});

@obecny I implemented this while also updating the return type and leaving in the @todo comment, I think there is still error logging functionality that needs to be handled so we should likely leave that in, no?

obecny

I have added a follow up PR which fixes the remaining issues but also unifies and fixes the shutdown across all exporters, metric, spans, processors, so that the shutdown will be correctly handled in whole pipeline. It will also return promise that will resolve correctly when all tasks that depend on it will resolve including http, xmlhttprequest, grpc. Callbacks has been replaced with Promises.

jonahrosenblum#3

@open-telemetry/javascript-approvers please have a look at follow up PR ^^

dyladan

This looks good. Sorry for the long review process

jonahrosenblum · 2020-08-14T19:15:25Z

This looks good. Sorry for the long review process

No worries, thanks for all the help and feedback along the way! How many more reviewers do I need for this PR?

dyladan · 2020-08-14T19:15:59Z

One more

obecny · 2020-08-14T19:17:15Z

@dyladan have you seen #1321 (review) ?

dyladan · 2020-08-14T19:18:25Z

@dyladan have you seen #1321 (review) ?

Yes I saw it but I assumed since you opened it against jonah's master it wasn't ready for reviews and that you would open a PR here when you were ready.

obecny · 2020-08-14T19:24:29Z

@dyladan have you seen #1321 (review) ?

Yes I saw it but I assumed since you opened it against jonah's master it wasn't ready for reviews and that you would open a PR here when you were ready.

Well I can't open the PR against otel as it requires this PR, otherwise it will show changes from here and mine changes and it will be impossible to see differences.

kintel · 2020-08-14T19:32:44Z

@obecny A common pattern is to open a draft PR against otel anyway, and point out that only the last commit is subject to review. Then just rebase and force-push once this PR is merged. Not ideal, but it offers good visibility.

mayurkale22

overall LGTM.

mayurkale22 · 2020-08-14T20:53:29Z

packages/opentelemetry-metrics/src/types.ts

  /** Metric batcher. */
  batcher?: Batcher;
+
+  /** Bool for whether or not graceful shutdown is enabled */


It would be nice to highlight the pitfall of disabling the gracefulShutdown option.

I added a little more emphasis about why it's good to enable graceful shutdown.

mayurkale22 · 2020-08-14T20:55:32Z

packages/opentelemetry-tracing/test/export/InMemorySpanExporter.test.ts

-    // reset spans in memory.
-    memoryExporter.reset();
+  beforeEach(() => {
+    memoryExporter = new InMemorySpanExporter();


Just curious to know the reasoning behind this?

Only having one instantiation of the in-memory exporter caused some strange bugs in testing. This approach resolves the bug and doesn't complicate the test case too much.

mayurkale22 · 2020-08-14T22:47:07Z

packages/opentelemetry-tracing/test/export/BatchSpanProcessor.test.ts

      it('should call an async callback when shutdown is complete', done => {
        let exportedSpans = 0;
        sinon.stub(exporter, 'export').callsFake((spans, callback) => {
-          console.log('uh, export?');


Good catch!

dyladan · 2020-08-17T18:56:06Z

I think this can be merged, but I will let @obecny do it as he seems to have a plan for follow up.

Graceful shutdown support was removed via open-telemetry#1522 Remove the corresponding configuration because it is not used anymore. Refs: open-telemetry#1321

Graceful shutdown support was removed via #1522 Remove the corresponding configuration because it is not used anymore. Refs: #1321

Graceful shutdown support was removed via open-telemetry/opentelemetry-js#1522 Remove the corresponding configuration because it is not used anymore. Refs: open-telemetry/opentelemetry-js#1321

* chore(mongo): add DB_OPERATION attribute * chore(mongo): replace 'remove' with 'delete' * chore(mongo): add tests * chore(mongo): add condition in case cmd=unknown * chore(mongo): make code more readable

jonahrosenblum added 4 commits July 16, 2020 05:40

feat: graceful shutdown for tracing and metrics

871d696

Merge remote-tracking branch 'upstream/master' into master

da633ad

fix: wording in test case

de161cc

fix: typo

edd3b49

jonahrosenblum requested review from OlivierAlbertini, dyladan, legendecas, markwolff, mayurkale22, mwear, naseemkullah, obecny and vmarchaud as code owners July 16, 2020 19:00

dyladan reviewed Jul 16, 2020

View reviewed changes

jonahrosenblum and others added 5 commits July 16, 2020 19:07

fix meterprovider config to use bracket notation

16c0faf

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

fix meterprovider config to use bracket notation

f0b9ef6

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

fix: add callbacks to shutdown methods

4bd85b5

fix: merge conflict

86cc4e9

fix: merge conflict

5d93f6f

dyladan reviewed Jul 17, 2020

View reviewed changes

packages/opentelemetry-metrics/src/MeterProvider.ts Outdated Show resolved Hide resolved

packages/opentelemetry-metrics/src/MeterProvider.ts Outdated Show resolved Hide resolved

jonahrosenblum and others added 3 commits July 17, 2020 21:20

simplify meter shutdown code

9ad87e5

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

Merge remote-tracking branch 'upstream/master' into master

1d3ea53

fix: fix one-liner

0a99520

dyladan reviewed Jul 17, 2020

View reviewed changes

packages/opentelemetry-metrics/src/MeterProvider.ts Outdated Show resolved Hide resolved

jonahrosenblum and others added 2 commits July 18, 2020 00:22

private function name style fix

791156b

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

fix: naming of private member variables

7d714b6

dyladan approved these changes Jul 20, 2020

View reviewed changes

dyladan requested changes Jul 22, 2020

View reviewed changes

obecny suggested changes Jul 22, 2020

View reviewed changes

obecny reviewed Aug 12, 2020

View reviewed changes

fix: add documentation and cleanup code

bfa1a0a

obecny reviewed Aug 12, 2020

View reviewed changes

jonahrosenblum added 2 commits August 12, 2020 22:12

fix: remove async label from shutdown and cleanup test case

ec3ce12

fix: update controller collect to return promise

49e1a69

obecny approved these changes Aug 13, 2020

View reviewed changes

dyladan approved these changes Aug 14, 2020

View reviewed changes

mayurkale22 approved these changes Aug 14, 2020

View reviewed changes

fix: make downsides of disabling graceful shutdown more apparent

8afa9a1

mayurkale22 approved these changes Aug 15, 2020

View reviewed changes

jonahrosenblum and others added 2 commits August 17, 2020 15:57

Merge branch 'master' into master

32439ab

Merge branch 'master' into master

529df9b

Merge branch 'master' into master

d5c430c

obecny merged commit b884eec into open-telemetry:master Aug 17, 2020

dyladan linked an issue Aug 20, 2020 that may be closed by this pull request

Cloud Run Graceful Termination Support #1290

Closed

Flarna mentioned this pull request Sep 12, 2020

Remove process listener #1522

Merged

Flarna mentioned this pull request Nov 19, 2020

chore: remove unused tracer config gracefulShutdown #1692

Merged

dyladan pushed a commit that referenced this pull request Nov 25, 2020

chore: remove unused tracer config gracefulShutdown (#1692)

8933661

Graceful shutdown support was removed via #1522 Remove the corresponding configuration because it is not used anymore. Refs: #1321

-    this._exporter.export(this._meter.getBatcher().checkPointSet(), result => {
+    return new Promise((resolve, reject) => {
+      this._exporter.export(
+        this._meter.getBatcher().checkPointSet(),
+        result => {
+          if (result === ExportResult.SUCCESS) {
+            resolve();
+          } else {
+            reject();
+          }
+        }
+      );
+    });

Conversation

jonahrosenblum commented Jul 16, 2020

Which problem is this PR solving?

Short description of the changes

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dyladan left a comment

Choose a reason for hiding this comment

Uh oh!

dyladan left a comment

Choose a reason for hiding this comment

Uh oh!

obecny left a comment

Choose a reason for hiding this comment

Uh oh!

obecny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahrosenblum commented Aug 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov bot commented Jul 16, 2020 •

edited

Loading

obecny left a comment •

edited

Loading