add an outbound health check #641

mukundansundar · 2023-11-23T11:04:10Z

Description

closes #611

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #611

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

Signed-off-by: Mukundan Sundararajan <[email protected]>

mukundansundar

I wanted to split out health check apis from the service invocation api, so that even when service invocation protocol is chosen as grpc, http is still used as the health check protocol.

mukundansundar · 2023-11-23T11:05:09Z

dapr/clients/__init__.py

@@ -173,3 +175,27 @@ async def invoke_method_async(
        else:
            raise NotImplementedError(
                'Please use `dapr.aio.clients.DaprClient` for async invocation')
+
+    def wait(self, timeout_s: float):


This is float because DaprGrpcClient already has a method wait as float, I can remove that method if that would be better....

mukundansundar · 2023-11-23T11:05:14Z

dapr/clients/__init__.py

+        """
+        self.helath_client.wait(int(timeout_s))
+
+    async def wait_async(self, timeout_s: float):


This is float because DaprGrpcClient already has a method wait as float, I can remove that method if that would be better....

mukundansundar · 2023-11-23T11:07:25Z

dapr/clients/http/helpers.py

+    """Dapr Health Client"""
+
+    def __init__(self, timeout: Optional[int] = 60):
+        self._client = DaprHttpClient(DefaultJSONSerializer(), timeout, None, None)


is there a need to pass in address from DaprClient initialization?
Both http and grpc might not have the same endpoint address.

That's unrelated to this PR IMO. We can open an issue for discussion in the future. I don't think we need to tackle that until someone has a concrete need for it. The HTTP Client is only used by Actors and by Service Invocation (and now the health client), though using the SDK for service invocation is no longer the recommended way. Perhaps you can take a look how the DotNet SDK handles this and compare.

codecov · 2023-11-23T11:08:43Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (6ee22eb) 86.16% compared to head (2bdcb3d) 86.23%.

Files	Patch %	Lines
dapr/clients/__init__.py	66.66%	2 Missing ⚠️
dapr/aio/clients/__init__.py	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #641      +/-   ##
==========================================
+ Coverage   86.16%   86.23%   +0.07%     
==========================================
  Files          75       76       +1     
  Lines        3737     3779      +42     
==========================================
+ Hits         3220     3259      +39     
- Misses        517      520       +3

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

berndverst

Instead of creating another instance of DaprHTTPClient and also the Serializer - why not a simple static utility method that uses urllib ? I would much prefer that.

That method could then be imported within the wait methods and used there.

Allocating an entire Dapr HTTP Client with Serializer just feels inefficient from a memory perspective.

berndverst · 2023-11-27T21:35:05Z

Your change broke the configuration API example by the way

		== APP == Traceback (most recent call last):
		== APP ==   File "configuration.py", line 47, in <module>
		== APP ==     asyncio.run(executeConfiguration())
		== APP ==   File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/asyncio/runners.py", line 44, in run
		== APP ==     return loop.run_until_complete(main)
		== APP ==   File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/asyncio/base_events.py", line 616, in run_until_complete
		== APP ==     return future.result()
		== APP ==   File "configuration.py", line 25, in executeConfiguration
		== APP ==     d.wait(20)
		== APP ==   File "/home/runner/work/python-sdk/python-sdk/dapr/clients/__init__.py", line 189, in wait
		== APP ==     self.helath_client.wait(int(timeout_s))
		== APP ==   File "/home/runner/work/python-sdk/python-sdk/dapr/clients/http/helpers.py", line 81, in wait
		== APP ==     loop.run_until_complete(awaitable)
		== APP ==   File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/asyncio/base_events.py", line 592, in run_until_complete
		== APP ==     self._check_running()
		== APP ==   File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/asyncio/base_events.py", line 552, in _check_running
		== APP ==     raise RuntimeError('This event loop is already running')
		== APP == RuntimeError: This event loop is already running
		== APP == sys:1: RuntimeWarning: coroutine 'DaprHealthClient.wait_async' was never awaited

berndverst · 2023-11-27T21:36:14Z

dapr/clients/__init__.py

@@ -72,6 +73,7 @@ def __init__(
        """
        super().__init__(address, interceptors, max_grpc_message_length)
        self.invocation_client = None
+        self.helath_client = DaprHealthClient(timeout=http_timeout_seconds)


Suggested change

self.helath_client = DaprHealthClient(timeout=http_timeout_seconds)

self.health_client = DaprHealthClient(timeout=http_timeout_seconds)

mukundansundar · 2023-12-12T18:36:40Z

Instead of creating another instance of DaprHTTPClient and also the Serializer - why not a simple static utility method that uses urllib ? I would much prefer that.

That method could then be imported within the wait methods and used there.

Allocating an entire Dapr HTTP Client with Serializer just feels inefficient from a memory perspective.

will change the PR to use urllib instead.

berndverst · 2024-02-06T19:33:21Z

Update on this?

add an outbound health check

2bdcb3d

Signed-off-by: Mukundan Sundararajan <[email protected]>

mukundansundar commented Nov 23, 2023

View reviewed changes

berndverst requested changes Nov 27, 2023

View reviewed changes

berndverst reviewed Nov 27, 2023

View reviewed changes

berndverst closed this Feb 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add an outbound health check #641

add an outbound health check #641

mukundansundar commented Nov 23, 2023

mukundansundar left a comment

mukundansundar Nov 23, 2023

mukundansundar Nov 23, 2023

mukundansundar Nov 23, 2023

berndverst Nov 27, 2023 •

edited

Loading

codecov bot commented Nov 23, 2023

berndverst left a comment

berndverst commented Nov 27, 2023

berndverst Nov 27, 2023

mukundansundar commented Dec 12, 2023

berndverst commented Feb 6, 2024

	self.helath_client = DaprHealthClient(timeout=http_timeout_seconds)
	self.health_client = DaprHealthClient(timeout=http_timeout_seconds)

add an outbound health check #641

add an outbound health check #641

Conversation

mukundansundar commented Nov 23, 2023

Description

Issue reference

Checklist

mukundansundar left a comment

Choose a reason for hiding this comment

mukundansundar Nov 23, 2023

Choose a reason for hiding this comment

mukundansundar Nov 23, 2023

Choose a reason for hiding this comment

mukundansundar Nov 23, 2023

Choose a reason for hiding this comment

berndverst Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Nov 23, 2023

Codecov Report

berndverst left a comment

Choose a reason for hiding this comment

berndverst commented Nov 27, 2023

berndverst Nov 27, 2023

Choose a reason for hiding this comment

mukundansundar commented Dec 12, 2023

berndverst commented Feb 6, 2024

berndverst Nov 27, 2023 •

edited

Loading