Add an HTTP client for service invocation and default to using it by wcs1only · Pull Request #182 · dapr/python-sdk

wcs1only · 2021-01-28T23:39:30Z

Description

Created an HTTP client to use for service invocation. Also added a configuration option allow GRPC to be used as well.

Note: This PR was getting quite long, so I've split out another issue to add tracing.

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: #176

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

codecov · 2021-01-29T04:09:45Z

Codecov Report

Merging #182 (0ce0695) into master (803f286) will increase coverage by 1.54%.
The diff coverage is 89.23%.

@@            Coverage Diff             @@
##           master     #182      +/-   ##
==========================================
+ Coverage   87.11%   88.65%   +1.54%     
==========================================
  Files          47       50       +3     
  Lines        1862     1948      +86     
==========================================
+ Hits         1622     1727     +105     
+ Misses        240      221      -19

Impacted Files	Coverage Δ
dapr/clients/http/dapr_actor_http_client.py	`48.57% <37.50%> (+11.28%)`	⬆️
dapr/clients/http/client.py	`93.47% <93.47%> (ø)`
dapr/clients/main.py	`95.45% <95.45%> (ø)`
dapr/clients/__init__.py	`100.00% <100.00%> (ø)`
dapr/clients/grpc/_response.py	`94.28% <100.00%> (+0.12%)`	⬆️
dapr/clients/grpc/client.py	`91.21% <100.00%> (+1.35%)`	⬆️
dapr/clients/http/dapr_invocation_http_client.py	`100.00% <100.00%> (ø)`
dapr/conf/global_settings.py	`100.00% <100.00%> (ø)`
dapr/serializers/json.py	`89.47% <100.00%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 803f286...0ce0695. Read the comment docs.

artursouza · 2021-01-30T00:19:35Z

 from dapr.clients.base import DaprActorClientBase
 from dapr.clients.exceptions import DaprInternalError, ERROR_CODE_UNKNOWN
-from dapr.clients.grpc.client import DaprClient
+from dapr.clients.generic import DaprClient


generic is an overloaded term here. Can you name this package something else? Maybe main?

Yeah, can do.

artursouza · 2021-01-30T00:23:50Z

+            http_querystring: Optional[MetadataTuple] = None) -> InvokeMethodResponse:
+
+        if self.invocation_client:
+            return self.invocation_client.invoke_method(


Why tests do not cover this if-else?

The unit tests are hitting the DaprHttpInvocationClient class directly

artursouza · 2021-01-30T00:25:00Z

                matched with the response data type
        """
+
+        if self.content_type == "application/x-protobuf":


handle lower case vs upper case and content type optional suffix (application/x-protobuf; gzip)

The http library already correctly parses out the suffix for us, but I'll add a test to confirm this. Will also make case sensitive.

artursouza · 2021-01-30T00:25:25Z



-class DaprClient:
+class DaprGrpcClient:


Why renaming it if it is in another package?

Because there is a new DaprClient which inherits from this class. Would be confusing if I named it the same, wouldn't it?

You can name it differently when importing it. It is already in another package.

Yeah, but we only named it so generically in the first place because we only had the gRPC client and nothing else. Now that we have both gRPC and HTTP, naming them more specifically makes sense.

artursouza · 2021-01-30T00:28:20Z

        dict_obj = obj
+
+        # importing this from top scope creates a circular import
+        from dapr.actor.runtime.config import ActorRuntimeConfig


Wait. Why we did not have this before?

We did, I just moved it out of the module scope.

You mean we had a circular import before?

Right, if I attempted include the json serializer without including all the actor classes first, it would try to pull in ActorRuntimeConfig which would pull in the rest of the actor classes, which would pull in the json serializer. This was the easiest way to break the loop.

artursouza · 2021-01-30T00:28:50Z

 from unittest.mock import patch

-from dapr.clients.grpc.client import DaprClient
+from dapr.clients.grpc.client import DaprGrpcClient as DaprClient


This is odd because it was named correctly before.

This is what I mean, you can just name it DaprGrpcClient when using it. So the class can keep the original name.

Yeah, but I'd rather not have two classes which are both called DaprClient in the overall project, even if I can alias one of them away. Prefer to just call the DaprGrpcClient the same everywhere and use the generic name DaprClient only for the public facing class that does both Grpc and HTTP. I've updated this to not use an alias, rather just use the name DaprGrpcClient everywhere, so there is no ambiguity anywhere.

artursouza · 2021-01-30T01:27:24Z

+        if http_verb is not None:
+            verb = http_verb
+
+        headers = {}


We need DAPR_API_TOKEN and the traceparent header to be set here.

Ended up putting this in dapr/clients/http/client.py:50 so it can be shared with the Actor API client.

wcs1only · 2021-01-30T04:26:33Z

Tracing and API token added. I've verified the sample. Everything looks right, though the grpc integration has an extra span in the stack.

wcs1only force-pushed the http_invocation branch 11 times, most recently from 027d418 to a70aa3b Compare January 29, 2021 04:07

wcs1only force-pushed the http_invocation branch 3 times, most recently from 3714150 to ba83fb7 Compare January 29, 2021 23:54

wcs1only marked this pull request as ready for review January 30, 2021 00:00

wcs1only requested review from artursouza, mukundansundar and youngbupark January 30, 2021 00:00

artursouza suggested changes Jan 30, 2021

View reviewed changes

wcs1only force-pushed the http_invocation branch from ba83fb7 to ebbf94e Compare January 30, 2021 01:13

artursouza reviewed Jan 30, 2021

View reviewed changes

wcs1only force-pushed the http_invocation branch 2 times, most recently from 3de731f to c159dd5 Compare January 30, 2021 05:00

wcs1only requested a review from artursouza January 30, 2021 05:02

Add an HTTP client for service invocation and default to using it

0ce0695

wcs1only force-pushed the http_invocation branch from c159dd5 to 0ce0695 Compare January 30, 2021 18:57

artursouza approved these changes Jan 30, 2021

View reviewed changes

artursouza merged commit a115d9b into dapr:master Jan 30, 2021

Uh oh!

Conversation

wcs1only commented Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issue reference

Checklist

Uh oh!

codecov Bot commented Jan 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wcs1only Jan 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wcs1only Jan 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wcs1only commented Jan 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wcs1only commented Jan 28, 2021 •

edited

Loading

codecov Bot commented Jan 29, 2021 •

edited

Loading

wcs1only Jan 30, 2021 •

edited

Loading

wcs1only Jan 30, 2021 •

edited

Loading