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 DependenciesHealthcheck dependency #17

Merged
merged 3 commits into from
Sep 26, 2024
Merged

Add DependenciesHealthcheck dependency #17

merged 3 commits into from
Sep 26, 2024

Conversation

RB387
Copy link
Collaborator

@RB387 RB387 commented Sep 20, 2024

Description

Added

  • Added magic_di.healthcheck.DependenciesHealthcheck class to make health checks of injected dependencies that implement magic_di.healthcheck.PingableProtocol interface

Fixed

  • Inject dependencies inside of event loop in magic_di.utils.inject_and_run to prevent wrong event loop usage inside of the injected dependencies

Checklist

  • Tests covering the new functionality have been added
  • Documentation has been updated OR the change is too minor to be documented
  • Changes are listed in the CHANGELOG.md OR changes are insignificant


injector: DependencyInjector

async def ping_dependencies(self, max_concurrency: int = 1) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be beneficial if this would return information about which ones failed (and which one succeeded)? Then it would be possible to include that information in the response of the healthcheck endpoint. At minimum, I think there could be some logging inside this method about failures.

Copy link
Collaborator

@erhosen erhosen Sep 23, 2024

Choose a reason for hiding this comment

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

Currently one of our services uses this approach: each dependency (such as redis, mongo or kafka-consumer) implements a ping method, that might raise an exception. Each of those pings has they own set of exceptions (e.g., ConnectionFailure, NetworkTimeout, OperationFailure for MongoDB), and we’ve intentionally decided not to catch them.

This approach ensures that in case of fire, we get a complete stack trace in the logs, making it easier to debug what happened.

And it also leads to 500 error status code for a typical FastAPI/Starlette server. Since these health checks are primarily designed for Kubernetes - correct status code is basically the only thing that matters

What do you think about it?

Copy link
Collaborator Author

@RB387 RB387 Sep 23, 2024

Choose a reason for hiding this comment

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

Would it be beneficial if this would return information about which ones failed (and which one succeeded)?

Hmm. Do we have any case where we need to know which dependencies are healthy or not?
Usually we only want to know if everything is healthy.

At minimum, I think there could be some logging inside this method about failures.

That’s a good point. I added debug logging, but I decided not to log failures since they are raised to user and user can catch them and log if needed.

But, what I’m thinking about is, do we need the ignore_exceptions argument? To ignore some concrete exceptions that are raised by some dependencies

Copy link
Collaborator

@jerry-git jerry-git Sep 26, 2024

Choose a reason for hiding this comment

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

Yeah, I guess it makes sense, at least in K8s (probes) use cases. I was just thinking that peeps in the wider community might have also non-K8s use cases in which it might be beneficial to know which ones failed. Anyway, I guess this will be sufficient for majority.

do we need the ignore_exceptions argument

At least I can't think of a real world use case in which one would like to check the healthiness of all dependencies but ignore some 🤔

Copy link
Collaborator

@jerry-git jerry-git left a comment

Choose a reason for hiding this comment

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

🍬

@RB387 RB387 merged commit bb31fa4 into main Sep 26, 2024
14 checks passed
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.

3 participants