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.

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

Add Graceful Shutdown Support #1321

Merged
merged 69 commits into from
Aug 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
69 commits
Select commit Hold shift + click to select a range
871d696
feat: graceful shutdown for tracing and metrics
jonahrosenblum Jul 16, 2020
da633ad
Merge remote-tracking branch 'upstream/master' into master
jonahrosenblum Jul 16, 2020
de161cc
fix: wording in test case
jonahrosenblum Jul 16, 2020
edd3b49
fix: typo
jonahrosenblum Jul 16, 2020
16c0faf
fix meterprovider config to use bracket notation
jonahrosenblum Jul 16, 2020
f0b9ef6
fix meterprovider config to use bracket notation
jonahrosenblum Jul 16, 2020
4bd85b5
fix: add callbacks to shutdown methods
jonahrosenblum Jul 17, 2020
86cc4e9
fix: merge conflict
jonahrosenblum Jul 17, 2020
5d93f6f
fix: merge conflict
jonahrosenblum Jul 17, 2020
9ad87e5
simplify meter shutdown code
jonahrosenblum Jul 17, 2020
1d3ea53
Merge remote-tracking branch 'upstream/master' into master
jonahrosenblum Jul 17, 2020
0a99520
fix: fix one-liner
jonahrosenblum Jul 17, 2020
791156b
private function name style fix
jonahrosenblum Jul 18, 2020
7d714b6
fix: naming of private member variables
jonahrosenblum Jul 18, 2020
b106b50
fix: graceful shutdown now works in browser
jonahrosenblum Jul 22, 2020
ba5be87
Merge remote-tracking branch 'upstream/master' into master
jonahrosenblum Jul 22, 2020
064c8e8
fix: window event listener will trigger once
jonahrosenblum Jul 22, 2020
54a82c5
fix: modify global shutdown helper functions
jonahrosenblum Jul 23, 2020
8fd53c8
fix: remove callback from remove listener args
jonahrosenblum Jul 23, 2020
b5af380
fix: change global shutdown function names and simplify functionality
jonahrosenblum Jul 23, 2020
e159546
fix: add rest of function refactoring and simplification
jonahrosenblum Jul 23, 2020
c0d3f5f
fix: remove unintended code snippet
jonahrosenblum Jul 23, 2020
9ceba0e
fix: refactor naming of listener cleanup function and fix sandbox issue
jonahrosenblum Jul 24, 2020
078802d
fix: make global shutdown cleanup local
jonahrosenblum Jul 24, 2020
adedac1
Merge remote-tracking branch 'upstream/master' into master
jonahrosenblum Jul 24, 2020
44a7275
fix: change interval of MeterProvider collection to ensure it does no…
jonahrosenblum Aug 3, 2020
a1bfb12
fix: resolve merge conflict
jonahrosenblum Aug 3, 2020
3dd51d2
Merge remote-tracking branch 'upstream/master' into master
jonahrosenblum Aug 3, 2020
d541837
chore: removing _cleanupGlobalShutdownListeners
obecny Aug 3, 2020
0163241
fix: remove unnecesary trace provider member function
jonahrosenblum Aug 4, 2020
95bb8bf
Merge remote-tracking branch 'upstream/master' into master
jonahrosenblum Aug 4, 2020
ef84611
Merge branch 'master' of https:/open-telemetry/openteleme…
jonahrosenblum Aug 5, 2020
6f4de1e
Removing default span attributes (#1342)
aravinsiva Jul 27, 2020
5597957
feat: add baggage support to the opentracing shim (#918)
rubenvp8510 Jul 27, 2020
7c09806
Add nodejs sdk package (#1187)
dyladan Jul 27, 2020
b956e9c
feat: add OTEL_LOG_LEVEL env var (#974)
Jul 27, 2020
ab0de32
Proto update to latest to support arrays and maps (#1339)
obecny Jul 27, 2020
c4ffad7
chore: 0.10.0 release proposal (#1345)
dyladan Jul 27, 2020
32db1a9
fix: add missing grpc-js index (#1358)
dyladan Jul 28, 2020
db4745e
chore: 0.10.1 release proposal (#1359)
dyladan Jul 28, 2020
0a0a487
feat(api/context-base): change compile target to es5 (#1368)
markwolff Jul 31, 2020
802c5ad
Feat: Make ID generator configurable (#1331)
EdZou Jul 31, 2020
e6d49b4
fix: require grpc-js instead of grpc in grpc-js example (#1364)
reggiemcdonald Jul 31, 2020
5727e6e
chore(deps): update all non-major dependencies (#1371)
renovate-bot Jul 31, 2020
451c99c
chore: bump metapackage dependencies (#1383)
dyladan Aug 3, 2020
9336bfa
chore: 0.10.2 proposal (#1382)
dyladan Aug 3, 2020
bf46c1b
fix: remove unnecesary trace provider member function
jonahrosenblum Aug 4, 2020
62966b5
refactor(metrics): distinguish different aggregator types (#1325)
legendecas Aug 4, 2020
d07163a
Propagate b3 parentspanid and debug flag (#1346)
srjames90 Aug 4, 2020
f0323e6
feat: Export MinMaxLastSumCountAggregator metrics to the collector as…
davidwitten Aug 4, 2020
4d92d39
feat: Collector Metric Exporter for the Web (#1308)
davidwitten Aug 5, 2020
70bee58
Fix issues in TypeScript getting started example code (#1374)
mickdekkers Aug 5, 2020
ef046b9
chore: deploy canary releases (#1384)
dyladan Aug 6, 2020
5b14fb5
fix: addressing failed merge
jonahrosenblum Aug 7, 2020
73edc9a
Merge branch 'master' of https:/jonahrosenblum/openteleme…
jonahrosenblum Aug 7, 2020
12b18fd
fix: protos pull
jonahrosenblum Aug 7, 2020
b0fdffd
fix: address marius' feedback
jonahrosenblum Aug 11, 2020
81f7dee
chore: merge with pr_1321
obecny Aug 11, 2020
93719b9
chore: deleting removeAllListeners from prometheus, fixing tests, cle…
obecny Aug 11, 2020
a0d309d
Merge branch 'master' into pr_1321_fix
obecny Aug 11, 2020
a28cfb0
Merge branch 'master' into master
obecny Aug 11, 2020
9c21dbb
Merge pull request #2 from obecny/pr_1321_fix
jonahrosenblum Aug 11, 2020
bfa1a0a
fix: add documentation and cleanup code
jonahrosenblum Aug 12, 2020
ec3ce12
fix: remove async label from shutdown and cleanup test case
jonahrosenblum Aug 12, 2020
49e1a69
fix: update controller collect to return promise
jonahrosenblum Aug 12, 2020
8afa9a1
fix: make downsides of disabling graceful shutdown more apparent
jonahrosenblum Aug 15, 2020
32439ab
Merge branch 'master' into master
jonahrosenblum Aug 17, 2020
529df9b
Merge branch 'master' into master
dyladan Aug 17, 2020
d5c430c
Merge branch 'master' into master
obecny Aug 17, 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* Copyright The OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/**
* Adds an event listener to trigger a callback when an unload event in the window is detected
*/
export function notifyOnGlobalShutdown(cb: () => void): () => void {
jonahrosenblum marked this conversation as resolved.
Show resolved Hide resolved
window.addEventListener('unload', cb, { once: true });
return function removeCallbackFromGlobalShutdown() {
window.removeEventListener('unload', cb, false);
};
}

/**
* Warning: meant for internal use only! Closes the current window, triggering the unload event
*/
export function _invokeGlobalShutdown() {
jonahrosenblum marked this conversation as resolved.
Show resolved Hide resolved
window.close();
}
1 change: 1 addition & 0 deletions packages/opentelemetry-core/src/platform/browser/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,4 @@ export * from './RandomIdGenerator';
export * from './performance';
export * from './sdk-info';
export * from './timer-util';
export * from './ShutdownNotifier';
32 changes: 32 additions & 0 deletions packages/opentelemetry-core/src/platform/node/ShutdownNotifier.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* Copyright The OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/**
* Adds an event listener to trigger a callback when a SIGTERM is detected in the process
*/
export function notifyOnGlobalShutdown(cb: () => void): () => void {
jonahrosenblum marked this conversation as resolved.
Show resolved Hide resolved
process.once('SIGTERM', cb);
return function removeCallbackFromGlobalShutdown() {
process.removeListener('SIGTERM', cb);
};
}

/**
* Warning: meant for internal use only! Sends a SIGTERM to the current process
*/
export function _invokeGlobalShutdown() {
process.kill(process.pid, 'SIGTERM');
}
1 change: 1 addition & 0 deletions packages/opentelemetry-core/src/platform/node/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,4 @@ export * from './RandomIdGenerator';
export * from './performance';
export * from './sdk-info';
export * from './timer-util';
export * from './ShutdownNotifier';
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
*/

import { HrTime, ObserverResult } from '@opentelemetry/api';
import {
notifyOnGlobalShutdown,
_invokeGlobalShutdown,
} from '@opentelemetry/core';
import {
CounterMetric,
SumAggregator,
Expand All @@ -32,6 +36,7 @@ const mockedTimeMS = 1586347902211000;

describe('PrometheusExporter', () => {
let toPoint: () => Point<Sum>;
let removeEvent: Function | undefined;
before(() => {
toPoint = SumAggregator.prototype.toPoint;
SumAggregator.prototype.toPoint = function (): Point<Sum> {
Expand Down Expand Up @@ -185,16 +190,27 @@ describe('PrometheusExporter', () => {

describe('export', () => {
let exporter: PrometheusExporter;
let meterProvider: MeterProvider;
let meter: Meter;

beforeEach(done => {
exporter = new PrometheusExporter();
meter = new MeterProvider().getMeter('test-prometheus');
meterProvider = new MeterProvider({
interval: Math.pow(2, 31) - 1,
gracefulShutdown: true,
});
meter = meterProvider.getMeter('test-prometheus', '1', {
exporter: exporter,
});
exporter.startServer(done);
});

afterEach(done => {
exporter.shutdown(done);
if (removeEvent) {
removeEvent();
removeEvent = undefined;
}
});

it('should export a count aggregation', done => {
Expand Down Expand Up @@ -320,6 +336,70 @@ describe('PrometheusExporter', () => {
});
});

it('should export multiple labels on graceful 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);

removeEvent = notifyOnGlobalShutdown(() => {
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 ${mockedTimeMS}`,
`counter{counterKey1="labelValue2"} 20 ${mockedTimeMS}`,
`counter{counterKey1="labelValue3"} 30 ${mockedTimeMS}`,
'',
]);

done();
});
})
.on('error', errorHandler(done));
});
_invokeGlobalShutdown();
});

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(() => {
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 ${mockedTimeMS}`,
`counter{counterKey1="labelValue2"} 20 ${mockedTimeMS}`,
`counter{counterKey1="labelValue3"} 30 ${mockedTimeMS}`,
'',
]);

done();
});
})
.on('error', errorHandler(done));
});
});

it('should export a comment if no metrics are registered', done => {
exporter.export([], () => {
http
Expand Down
7 changes: 6 additions & 1 deletion packages/opentelemetry-metrics/src/Meter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ export class Meter implements api.Meter {
private readonly _batcher: Batcher;
private readonly _resource: Resource;
private readonly _instrumentationLibrary: InstrumentationLibrary;
private readonly _controller: PushController;

/**
* Constructs a new Meter instance.
Expand All @@ -55,7 +56,7 @@ export class Meter implements api.Meter {
// start the push controller
const exporter = config.exporter || new NoopExporter();
const interval = config.interval;
new PushController(this, exporter, interval);
this._controller = new PushController(this, exporter, interval);
}

/**
Expand Down Expand Up @@ -309,6 +310,10 @@ export class Meter implements api.Meter {
return this._batcher;
}

async shutdown(): Promise<void> {
await this._controller.shutdown();
}

/**
* Registers metric to register.
* @param name The name of the metric.
Expand Down
27 changes: 26 additions & 1 deletion packages/opentelemetry-metrics/src/MeterProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import { ConsoleLogger } from '@opentelemetry/core';
import { ConsoleLogger, notifyOnGlobalShutdown } from '@opentelemetry/core';
import * as api from '@opentelemetry/api';
import { Resource } from '@opentelemetry/resources';
import { Meter } from '.';
Expand All @@ -26,6 +26,7 @@ import { DEFAULT_CONFIG, MeterConfig } from './types';
export class MeterProvider implements api.MeterProvider {
private readonly _config: MeterConfig;
private readonly _meters: Map<string, Meter> = new Map();
private _cleanNotifyOnGlobalShutdown: Function | undefined;
readonly resource: Resource;
readonly logger: api.Logger;

Expand All @@ -36,6 +37,11 @@ export class MeterProvider implements api.MeterProvider {
logger: this.logger,
resource: this.resource,
});
if (this._config.gracefulShutdown) {
this._cleanNotifyOnGlobalShutdown = notifyOnGlobalShutdown(
this._shutdownAllMeters.bind(this)
);
}
}

/**
Expand All @@ -54,4 +60,23 @@ export class MeterProvider implements api.MeterProvider {

return this._meters.get(key)!;
}

shutdown(cb: () => void = () => {}): void {
this._shutdownAllMeters().then(() => {
setTimeout(cb, 0);
});
if (this._cleanNotifyOnGlobalShutdown) {
this._cleanNotifyOnGlobalShutdown();
this._cleanNotifyOnGlobalShutdown = undefined;
}
}

private _shutdownAllMeters() {
if (this._config.exporter) {
this._config.exporter.shutdown();
Copy link

Choose a reason for hiding this comment

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

Is this._config.exporter.shutdown() actually blocking, or is this a missing callback or missing async decoration?
It may be out of scope to fix, but an issue could be created to look into this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think exporter shutdown calls are async. Regardless I'm not sure if it matters if this is blocking?

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 the exporters don't do any batching so it should be ok to have the shutdown be sync. On the other hand, it may be worth adding to the API just to allow the flexibility in the future.

Copy link

Choose a reason for hiding this comment

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

Technically, the exporters could batch here: Say you have 1K unflushed spans, and the backend API supports 100 spans per batch call with a 500 ms limit between calls. -> shutdown would take 5 seconds.

I'm not sure it matters either. Perhaps something to potentially address in a future API design sprint.

}
return Promise.all(
Copy link
Member

Choose a reason for hiding this comment

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

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

Array.from(this._meters, ([_, meter]) => meter.shutdown())
);
}
}
24 changes: 18 additions & 6 deletions packages/opentelemetry-metrics/src/export/Controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,24 @@ export class PushController extends Controller {
unrefTimer(this._timer);
}

private _collect() {
this._meter.collect();
this._exporter.export(this._meter.getBatcher().checkPointSet(), result => {
if (result !== ExportResult.SUCCESS) {
// @todo: log error
}
async shutdown(): Promise<void> {
await this._collect();
}

private async _collect(): Promise<void> {
await this._meter.collect();
return new Promise((resolve, reject) => {
this._exporter.export(
this._meter.getBatcher().checkPointSet(),
result => {
if (result === ExportResult.SUCCESS) {
resolve();
} else {
// @todo log error
reject();
}
}
);
});
}
}
4 changes: 4 additions & 0 deletions packages/opentelemetry-metrics/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,15 @@ export interface MeterConfig {

/** Metric batcher. */
batcher?: Batcher;

/** Bool for whether or not graceful shutdown is enabled. If disabled metrics will not be exported when SIGTERM is recieved */
gracefulShutdown?: boolean;
}

/** Default Meter configuration. */
export const DEFAULT_CONFIG = {
logLevel: getEnv().OTEL_LOG_LEVEL,
gracefulShutdown: true,
};

/** The default metric creation options value. */
Expand Down
Loading