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

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

Merged
merged 3 commits into from
Sep 27, 2021

Conversation

Half-Shot
Copy link
Contributor

This prevented us from calling this until we were listening for messages.

src/bridge.ts Outdated

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

@Half-Shot
Copy link
Contributor Author

I think this way ensures we set the metrics up once, and it should now work in either order.

@Half-Shot Half-Shot changed the title Fix getPrometheusMetrics to only throw if register is true Ensure getPrometheusMetrics() can be called in any order Sep 21, 2021
@Half-Shot Half-Shot merged commit 364a1a0 into develop Sep 27, 2021
@Half-Shot Half-Shot deleted the hs/fix-metrics-check branch May 2, 2023 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants