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

Jump to bottom

Ensure getPrometheusMetrics() can be called in any order #355

Merged
merged 3 commits into from
Sep 27, 2021
Merged

Ensure getPrometheusMetrics() can be called in any order #355

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
41186a2
Fix getPrometheusMetrics to only throw if register is true
Half-Shot Sep 20, 2021
b38e3c2
changelog
Half-Shot Sep 20, 2021
7a3b55d
Apply metrics once
Half-Shot Sep 21, 2021
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
1 change: 1 addition & 0 deletions changelog.d/355.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a bug that prevented bridges from calling `getPrometheusMetrics` without first calling `listen` in `Bridge`.
7 changes: 3 additions & 4 deletions src/bridge.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1509,6 +1509,7 @@ export class Bridge {
* The instance will automatically register the Matrix SDK metrics by calling
* {PrometheusMetrics~registerMatrixSdkMetrics}.
* @param {boolean} registerEndpoint Register the /metrics endpoint on the appservice HTTP server. Defaults to true.
* Note: `listen()` must have been called if this is true or this will throw.
* @param {Registry?} registry Optionally provide an alternative registry for metrics.
*/
public getPrometheusMetrics(registerEndpoint = true, registry?: Registry): PrometheusMetrics {
Expand All @@ -1524,10 +1525,8 @@ export class Bridge {

metrics.registerMatrixSdkMetrics(this.botSdkAS);

// TODO(paul): register some bridge-wide standard ones here

// In case we're called after .run()
if (this.appService && registerEndpoint) {
// This will throw if true and not called after run.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we want to throw early if we know that it's gonna happen. Two layers from here we'll get a Error('Cannot call addAppServicePath before calling .run()'), and in this context the user is calling getPrometheusMetrics rather than addAppServicePath before calling listen() rather than run().

Ideally I'd see all of these methods that require listen to be called first to be moved to a separate object that gets returned from listen(), so that API itself would prevent you from doing it wrong, but maybe that's an undertaking big enough for a 4.0.0 ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, definitely a bad smell that we have these at all but I think best reserved for 4.0.0.

if (registerEndpoint && this.appService) {
metrics.addAppServicePath(this);
}

Expand Down