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

Allow adding health check callback to Dapr App run in gRPC #723

Merged
merged 2 commits into from
Jun 26, 2024

Conversation

evhen14
Copy link
Contributor

@evhen14 evhen14 commented Jun 19, 2024

Description

Add health check support to the SDK for gRPC following the example of the go-sdk. The dapr docs state that the /dapr.proto.runtime.v1.AppCallbackHealthCheck/HealthCheck endpoint is called, but this SDK doesn't support specifying that method implementation.

The suggested way to use it would be app.register_health_check(health_check_func)

Use case 1: gRPC endpoints depend on other components to function properly. The health checks callback can be used to validate the liveness of the other components

Use case 2: Run gRPC app next to a web app in the same container in a separate thread. The entire app is healthy only when both dapr app and web app are healthy. I expect that dapr thread could die or stop being responsive, hence the way to add health checks.

Issue reference

It's the outcome of the issue #722 I raised earlier where I see a gap for adding meaningful health checks

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@evhen14 evhen14 requested review from a team as code owners June 19, 2024 03:26
Copy link

@passuied passuied Jun 21, 2024

Choose a reason for hiding this comment

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

typo: should be _health_servicer.py

@berndverst
Copy link
Member

Please run tox -e ruff for our autoformatter of source files.

Copy link
Member

@berndverst berndverst left a comment

Choose a reason for hiding this comment

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

Great contribution - and LGTM to me. Please run tox -e ruff to apply our autoformatter for code and commit the changes.

I'll approve and merge once that is done.

@yevgen-el8
Copy link

@berndverst tox -e ruff was applied. Thx for your feedback!

berndverst
berndverst previously approved these changes Jun 24, 2024
@berndverst
Copy link
Member

ext/dapr-ext-grpc/dapr/ext/grpc/_health_servicer.py:17: error: Function "cb" could always be true in boolean context  [truthy-function]
ext/dapr-ext-grpc/dapr/ext/grpc/_health_servicer.py:22: error: Attribute "_health_check_cb" already defined on line 19  [no-redef]
ext/dapr-ext-grpc/dapr/ext/grpc/_health_servicer.py:22: error: Incompatible types in assignment (expression has type "None", variable has type "Callable[[], None]")  [assignment]

Copy link

codecov bot commented Jun 25, 2024

Codecov Report

Attention: Patch coverage is 97.26027% with 2 lines in your changes missing coverage. Please review.

Project coverage is 86.22%. Comparing base (fc0e9d1) to head (eed11ed).
Report is 25 commits behind head on main.

Files Patch % Lines
...xt/dapr-ext-grpc/dapr/ext/grpc/_health_servicer.py 94.73% 1 Missing ⚠️
ext/dapr-ext-grpc/tests/test_app.py 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #723      +/-   ##
==========================================
- Coverage   86.37%   86.22%   -0.15%     
==========================================
  Files          79       79              
  Lines        4094     4166      +72     
==========================================
+ Hits         3536     3592      +56     
- Misses        558      574      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@evhen14 evhen14 force-pushed the health-check-support-grpc branch 2 times, most recently from 6e17c56 to f87e2a5 Compare June 25, 2024 22:05
@berndverst berndverst merged commit 477acd3 into dapr:main Jun 26, 2024
15 of 16 checks passed
@yaron2 yaron2 added this to the v1.14 milestone Jul 31, 2024
@marcduiker
Copy link
Contributor

@holopin-bot @evhen14 Thank you!

Copy link

holopin-bot bot commented Aug 15, 2024

Congratulations @evhen14, the maintainer of this repository has issued you a badge! Here it is: https://holopin.io/claim/clzvb5nb524130djsaga7l99m

This badge can only be claimed by you, so make sure that your GitHub account is linked to your Holopin account. You can manage those preferences here: https://holopin.io/account.
Or if you're new to Holopin, you can simply sign up with GitHub, which will do the trick!

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.

6 participants