Skip to content

Make metrics less error prone #810

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

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Make metrics less error prone #810

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b289f9b
Update to matrix-appservice-bridge 4.0.1
Half-Shot Jul 13, 2022
331d487
Update metrics to be functional
Half-Shot Jul 13, 2022
0b887e6
Support stopping the bridge gracefully
Half-Shot Jul 13, 2022
bbd3187
correct yarn.lock
Half-Shot Jul 13, 2022
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
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
"escape-string-regexp": "^4.0.0",
"js-yaml": "^3.14.0",
"marked": "^1.2.2",
"matrix-appservice-bridge": "^3.1.0",
"matrix-appservice-bridge": "^4.0.1",
"matrix-discord-parser": "0.1.5",
"mime": "^2.4.6",
"node-html-parser": "^1.2.19",
Expand Down
4 changes: 3 additions & 1 deletion src/bot.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,9 @@ export class DiscordBot {
}
}

public async stop(): Promise<void> {
public stop(): void {
this.presenceHandler.Stop();
this._bot?.destroy();
this._bot = undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick suggestion:

if (this._bot) {
    this._bot.destroy();
    this._bot = undefined; 
}

since there's a branch buried in the ?. anyways.

}

Expand Down
17 changes: 13 additions & 4 deletions src/discordas.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
Copyright 2017 - 2019 matrix-appservice-discord
Copyright 2017 - 2019 matrix-appservice-discord, 2022 (C) The Matrix.org Foundation C.I.C.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe let's just make a single change to update all of these at once 😆


Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand All @@ -13,7 +13,7 @@ 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.
*/
import { Appservice, IAppserviceRegistration, LogService, MatrixClient } from "matrix-bot-sdk";
import { Appservice, IAppserviceRegistration, LogService } from "matrix-bot-sdk";
import * as yaml from "js-yaml";
import * as fs from "fs";
import { DiscordBridgeConfig } from "./config";
Expand Down Expand Up @@ -148,7 +148,7 @@ async function run(): Promise<void> {
if (config.database.roomStorePath || config.database.userStorePath) {
log.error("The keys 'roomStorePath' and/or 'userStorePath' is still defined in the config. " +
"Please see docs/bridge-migrations.md on " +
"https://github.com/Half-Shot/matrix-appservice-discord/");
"https://github.com/matrix-org/matrix-appservice-discord/");
throw Error("Bridge has legacy configuration options and is unable to start");
}
const registration = yaml.safeLoad(fs.readFileSync(registrationPath, "utf8")) as IAppserviceRegistration;
Expand Down Expand Up @@ -181,6 +181,16 @@ async function run(): Promise<void> {
const roomhandler = discordbot.RoomHandler;
const eventProcessor = discordbot.MxEventProcessor;


process.once("SIGINT", () => {
Copy link
Member

Choose a reason for hiding this comment

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

This should trigger on SIGTERM too.

log.info("Got signal to stop application..");
appservice.stop();
MetricPeg.get.stop();
discordbot.stop();
process.exit(0);
});


// 2020-12-07: If this fails to build in TypeScript with
// "Namespace 'serveStatic' has no exported member 'RequestHandlerConstructor'.",
// remove @types/express-serve-static-core and @types/serve-static from yarn.lock
Expand Down Expand Up @@ -225,7 +235,6 @@ async function run(): Promise<void> {

await appservice.begin();
log.info(`Started listening on port ${port}`);

}

run().catch((err) => {
Expand Down
62 changes: 38 additions & 24 deletions src/metrics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,16 @@ limitations under the License.
/* eslint-disable max-classes-per-file, @typescript-eslint/no-empty-function */

import { Gauge, Counter, Histogram, collectDefaultMetrics, register } from "prom-client";
import { Log } from "./log";
import { Appservice,
IMetricContext,
METRIC_MATRIX_CLIENT_FAILED_FUNCTION_CALL,
METRIC_MATRIX_CLIENT_SUCCESSFUL_FUNCTION_CALL,
FunctionCallContext,
METRIC_MATRIX_CLIENT_FUNCTION_CALL} from "matrix-bot-sdk";
METRIC_MATRIX_CLIENT_FUNCTION_CALL,
} from "matrix-bot-sdk";
import { DiscordBridgeConfigMetrics } from "./config";
import * as http from "http";

const log = new Log("BridgeMetrics");
const REQUEST_EXPIRE_TIME_MS = 30000;

export interface IBridgeMetrics {
Expand All @@ -37,6 +36,7 @@ export interface IBridgeMetrics {
storeCall(method: string, cached: boolean);
setRemoteMonthlyActiveUsers(rmau: number);
setBridgeBlocked(isBlocked: boolean);
stop();
}

export class DummyBridgeMetrics implements IBridgeMetrics {
Expand All @@ -47,6 +47,7 @@ export class DummyBridgeMetrics implements IBridgeMetrics {
public storeCall() {}
public setRemoteMonthlyActiveUsers() {}
public setBridgeBlocked() {}
public stop() {}
}

export class MetricPeg {
Expand All @@ -63,36 +64,44 @@ export class MetricPeg {

export class PrometheusBridgeMetrics implements IBridgeMetrics {
private matrixCallCounter: Counter<string>;
private matrixFailedCallCounter: Counter<string>;
private remoteCallCounter: Counter<string>;
private storeCallCounter: Counter<string>;
private presenceGauge: Gauge<string>;
private remoteRequest: Histogram<string>;
private matrixRequest: Histogram<string>;
private requestsInFlight: Map<string, number>;
private matrixRequestStatus: Map<string, "success"|"failed">;
private requestsInFlight = new Map<string, number>();
private httpServer: http.Server;
private remoteMonthlyActiveUsers: Gauge<string>;
private bridgeBlocked: Gauge<string>;

public init(as: Appservice, config: DiscordBridgeConfigMetrics) {
collectDefaultMetrics();
// TODO: Bind this for every user.

this.httpServer = http.createServer((req, res) => {
if (req.method !== "GET" || req.url !== "/metrics") {
res.writeHead(404, "Not found");
res.end();
return;
}
// eslint-disable-next-line @typescript-eslint/naming-convention
res.writeHead(200, "OK", {"Content-Type": register.contentType});
res.write(register.metrics());
res.end();
});

this.matrixCallCounter = new Counter({
help: "Count of matrix API calls made",
labelNames: ["method", "result"],
name: "matrix_api_calls",
});
this.matrixFailedCallCounter = new Counter({
help: "Count of failed matrix API calls made",
labelNames: ["method", "result"],
name: "matrix_api_calls_failed",
});
register.registerMetric(this.matrixCallCounter);
register.registerMetric(this.matrixFailedCallCounter);

this.remoteCallCounter = new Counter({
help: "Count of remote API calls made",
Expand Down Expand Up @@ -136,7 +145,6 @@ export class PrometheusBridgeMetrics implements IBridgeMetrics {
}
});
}, REQUEST_EXPIRE_TIME_MS);
this.httpServer.listen(config.port, config.host);

this.remoteMonthlyActiveUsers = new Gauge({
help: "Current count of remote users active this month",
Expand All @@ -159,6 +167,7 @@ export class PrometheusBridgeMetrics implements IBridgeMetrics {
onStartMetric: this.sdkStartMetric.bind(this),
});

this.httpServer.listen(config.port, config.host);
return this;
}

Expand Down Expand Up @@ -196,35 +205,40 @@ export class PrometheusBridgeMetrics implements IBridgeMetrics {
this.bridgeBlocked.set(isBlocked ? 1 : 0);
}

private sdkStartMetric(metricName: string, context: IMetricContext) {
// We don't use this yet.
}

private sdkEndMetric(metricName: string, context: FunctionCallContext, timeMs: number) {
if (metricName !== METRIC_MATRIX_CLIENT_FUNCTION_CALL) {
return; // We don't handle any other type yet.
}
const successFail = this.matrixRequestStatus.get(context.uniqueId)!;
this.matrixRequestStatus.delete(context.uniqueId);
const ctx = context as FunctionCallContext;
this.matrixRequest.observe({
method: context.functionName,
result: successFail,
method: ctx.functionName,
}, timeMs);
}

private sdkResetMetric(metricName: string, context: IMetricContext) {
// We don't use this yet.
}

private sdkIncrementMetric(metricName: string, context: IMetricContext, amount: number) {
if (metricName === METRIC_MATRIX_CLIENT_SUCCESSFUL_FUNCTION_CALL) {
this.matrixRequestStatus.set(context.uniqueId, "success");
} else if (metricName === METRIC_MATRIX_CLIENT_FAILED_FUNCTION_CALL) {
this.matrixRequestStatus.set(context.uniqueId, "failed");
const ctx = context as FunctionCallContext;
this.matrixCallCounter.inc({method: ctx.functionName}, amount);
}
if (metricName === METRIC_MATRIX_CLIENT_FAILED_FUNCTION_CALL) {
const ctx = context as FunctionCallContext;
this.matrixFailedCallCounter.inc({method: ctx.functionName}, amount);
}
}

private sdkDecrementMetric(metricName: string, context: IMetricContext, amount: number) {
// We don't use this yet.
private sdkResetMetric() {
// Bot SDK doesn't use this yet.
}

private sdkDecrementMetric() {
// Bot SDK doesn't use this yet.
}

private sdkStartMetric() {
// We don't use this.
}

public stop() {
this.httpServer.close();
}
}
Loading