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

Ability to run health checks in parallel #1965

Closed
1 task done
noftaly opened this issue Jul 26, 2022 · 3 comments
Closed
1 task done

Ability to run health checks in parallel #1965

noftaly opened this issue Jul 26, 2022 · 3 comments

Comments

@noftaly
Copy link

noftaly commented Jul 26, 2022

Is there an existing issue that is already proposing this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe it

Hello, I have an app with an increasing number of indicators (currently ~10), and the /health endpoint is starting to feel rather slow. I've noticed that the indicators seem to be running in series.

Describe the solution you'd like

I'm suggesting to run them in parallel, or at least add an option to run them in parallel.
This could be achieved with Promise.all or Promise.allSettled (Node.js 12.9.0)

Teachability, documentation, adoption, migration strategy

The api would be the same as now, with this.healthCheckService.check(HealthIndicatorFunction[]), only it would be running each element in the array concurrently. If you are afraid of some possible breaking change, or if you want to keep the possibility to run them in series for whatever reason, maybe we could add a new method .checkParallel(HealthIndicatorFunction[])

What is the motivation / use case for changing the behavior?

Increase performances of apps that have a lot of health indicators (I personally have my database, a cache db, an instant-search db, 4 storage buckets, memory, ping checks to different frontend apps etc...)

Thank you !

@BrunnerLivio
Copy link
Member

Totally! I need to double check the history because I am certain it used to be parallel at some point. Maybe related to a bug - I’ll need to check.

I may be wrong but Promse.allSettled wasn’t supported back in the days. We can’t use Promise.all because I want to collect all error responses & write a summary. Promise.all aborts on the first rejection.

Either way I believe we can do this behavioural change directly on the HealthCheckService.check function. I wouldn’t consider this a breaking chang & if someones application breaks then they probably did something quite wrong 😅

I’ll check back on this in approx. 2 days because I am still on vacation 🏖

@noftaly
Copy link
Author

noftaly commented Jul 26, 2022

Thank you for the swift response!

Yep i figured you would need Promise.allSettled, which was introduced in Node.js 12.9.0 and is coincidentally the minimum Node.js version required for Nestjs!
So i guess it would be ok to use it.

No problem take your time, enjoy your vacation 😄

@BrunnerLivio
Copy link
Member

Released with 9.1.0 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants