feat: Add attempt_direct_path argument to create_channel #583

parthea · 2024-01-26T20:39:21Z

Fixes #482
b/267782870

vchudnov-g

This is looking good. I only reviewed the sync file and not the tests yet; I'll pick this back up tomorrow, but feel free to start responding to the comments here (or wait if you prefer; all good).

vchudnov-g · 2024-01-31T22:42:11Z

google/api_core/grpc_helpers.py

+    else:
+        # Use grpc.compute_engine_channel_credentials in order to support Direct Path.
+        # See https://grpc.github.io/grpc/python/grpc.html#grpc.compute_engine_channel_credentials
+        # TODO(b/323073050): Although `grpc.compute_engine_channel_credentials`


Since this is public code, I would prefer to a public (GitHub) issue in this TODO.

Fixed in e9199a0

vchudnov-g · 2024-01-31T22:43:59Z

google/api_core/grpc_helpers.py

+        attempt_direct_path (Optional[bool]): If set, Direct Path will be attempted when
+            the request is made. Direct Path provides a proxyless connection which
+            increases the available throughput, reduces latency, and increases
+            reliability. Outside of GCE, the direct path request may fallback


Do we need to spell out what GCE stands for in public docs? Not everyone looking at this file may be using GCE.

Fixed in e7e3cf7

vchudnov-g · 2024-01-31T22:44:18Z

google/api_core/grpc_helpers.py

+            If a `ServiceUnavailable` response is received when the request is sent, it is
+            recommended that the client repeat the request with `attempt_direct_path` set to `False`
+            as the Service may not support Direct Path. Using `ssl_credentials` with `attempt_direct_path`
+            set to `True` will result in `ValueError` as it is not yet supported.


Suggested change

set to `True` will result in `ValueError` as it is not yet supported.

set to `True` will result in `ValueError` as this combination is not yet supported.

Fixed in e7e3cf7

vchudnov-g · 2024-01-31T22:55:17Z

google/api_core/grpc_helpers.py

+        attempt_direct_path (Optional[bool]): If set, Direct Path will be attempted when
+            the request is made. Direct Path provides a proxyless connection which
+            increases the available throughput, reduces latency, and increases
+            reliability. Outside of GCE, the direct path request may fallback
+            to DNS if this is configured by the Service. This argument should only
+            be set in a GCE environment and for Services that are known to support Direct Path.
+            If a `ServiceUnavailable` response is received when the request is sent, it is
+            recommended that the client repeat the request with `attempt_direct_path` set to `False`
+            as the Service may not support Direct Path. Using `ssl_credentials` with `attempt_direct_path`
+            set to `True` will result in `ValueError` as it is not yet supported.


"Should only be set in a GCE environment" is confusing in light of "Outside of GCE, the request may fall back". Clarify. I suspect you mean something like this:

Suggested change

attempt_direct_path (Optional[bool]): If set, Direct Path will be attempted when

the request is made. Direct Path provides a proxyless connection which

increases the available throughput, reduces latency, and increases

reliability. Outside of GCE, the direct path request may fallback

to DNS if this is configured by the Service. This argument should only

be set in a GCE environment and for Services that are known to support Direct Path.

If a `ServiceUnavailable` response is received when the request is sent, it is

recommended that the client repeat the request with `attempt_direct_path` set to `False`

as the Service may not support Direct Path. Using `ssl_credentials` with `attempt_direct_path`

set to `True` will result in `ValueError` as it is not yet supported.

attempt_direct_path (Optional[bool]): If set, Direct Path will be attempted when

the request is made. Direct Path is only available within a Google Compute

Engine environment and provides a proxyless connection which increases the

available throughput, reduces latency, and increases reliability.

- This argument should only

be set in a GCE environment and for Services that are known to support Direct

Path.

- If this argument is set outside of GCE, then this request will fail

unless the back-end service happens to have configured fall-back to DNS.

- If the request causes a `ServiceUnavailable` response, we

recommend that the client repeat the request with `attempt_direct_path` set to

`False` as the Service may not support Direct Path.

- Using `ssl_credentials` with `attempt_direct_path`

set to `True` will result in `ValueError` as this combination is not yet

supported.

(And similarly for the async version)

WDYT?

Fixed in e7e3cf7

vchudnov-g · 2024-01-31T22:55:39Z

google/api_core/grpc_helpers.py

+        attempt_direct_path (Optional[bool]): If set, Direct Path will be attempted when
+            the request is made. Direct Path provides a proxyless connection which
+            increases the available throughput, reduces latency, and increases
+            reliability. Outside of GCE, the direct path request may fallback


Suggested change

reliability. Outside of GCE, the direct path request may fallback

reliability. Outside of GCE, the direct path request may fall back

but see comment below

Fixed in e7e3cf7

vchudnov-g · 2024-01-31T22:57:33Z

google/api_core/grpc_helpers.py

+        target (str): The target service address which is converted into a format compatible with Direct Path.
+            If the target contains `dns:///` or does not have contain `:///`, the target will be converted in
+            a format compatible with Direct Path, otherwise the original target will be returned.


Suggested change

target (str): The target service address which is converted into a format compatible with Direct Path.

If the target contains `dns:///` or does not have contain `:///`, the target will be converted in

a format compatible with Direct Path, otherwise the original target will be returned.

target (str): The target service address which is converted into a format compatible with Direct Path.

If the target contains `dns:///` or does not contain `:///`, the target will be converted in

a format compatible with Direct Path; otherwise the original target will be returned.

So the idea is that a :/// (except for dns:///) already denotes Direct Path, so no action is needed, correct? It might be good to be explicit about this assumption.

Done in 44d5845

vchudnov-g · 2024-02-01T01:29:20Z

google/api_core/grpc_helpers.py

+            a format compatible with Direct Path, otherwise the original target will be returned.
+    """
+
+    dns_prefix = "dns:///"


It might be helpful to clarify in a comment what the dns_prefix means, which I take it is to be explicit about an endpoint living in the Internet (ie outside GCP).

Fixed in e6ea7ec

vchudnov-g · 2024-02-01T01:36:54Z

google/api_core/grpc_helpers.py

@@ -288,6 +300,7 @@ def create_channel(
    default_scopes=None,
    default_host=None,
    compression=None,
+    attempt_direct_path: Optional[bool] = None,


Suggested change

attempt_direct_path: Optional[bool] = None,

attempt_direct_path: Optional[bool] = False,

Since it's meant to be a Boolean, might as well.

Fixed in 0f3a0e4

vchudnov-g

LGTM! Nothing blocking, but I think some of my suggestions can make things tighter/clearer.

vchudnov-g · 2024-02-01T18:05:41Z

google/api_core/grpc_helpers.py

+    # If `ssl_credentials` is set and `attempt_direct_path` is set to `True`,
+    # raise ValueError as this is not yet supported.
+    # See https://github.com/googleapis/python-api-core/issues/590
+    if ssl_credentials is not None and attempt_direct_path:


Could we simplify to just this?:

Suggested change

if ssl_credentials is not None and attempt_direct_path:

if ssl_credentials and attempt_direct_path:

Fixed in 60b8acb

vchudnov-g · 2024-02-01T18:10:47Z

google/api_core/grpc_helpers.py

    if HAS_GRPC_GCP:  # pragma: NO COVER
        if compression is not None and compression != grpc.Compression.NoCompression:
            _LOGGER.debug(
                "Compression argument is being ignored for grpc_gcp.secure_channel creation."
            )
+        if attempt_direct_path:
+            warnings.warn(


For consistency, the _LOGGER.debug in the previous lines should probably also become a warnings.warn

Fixed in 1a7578d and d96499f

vchudnov-g · 2024-02-01T18:13:31Z

google/api_core/grpc_helpers.py

+    Given a target, return a modified version which is compatible with Direct Path.
+
+    Args:
+        target (str): The target service address in the format 'hostname:port', 'dns://hostname' or other


Suggested change

target (str): The target service address in the format 'hostname:port', 'dns://hostname' or other

target (str): The target service address in the format 'hostname[:port]', 'dns://hostname[:port]' or other

Fixed in af3a21b

vchudnov-g · 2024-02-01T18:13:56Z

google/api_core/grpc_helpers.py

+
+    Args:
+        target (str): The target service address in the format 'hostname:port', 'dns://hostname' or other
+            compatible format.


"other compatible format": link to a place that lists compatible formats, or remove this phrase which I think is puzzling by itself.

Removed in af3a21b

vchudnov-g · 2024-02-01T18:21:07Z

google/api_core/grpc_helpers.py

+    direct_path_prefix = ":///"
+    if direct_path_prefix not in target:


Suggested change

direct_path_prefix = ":///"

if direct_path_prefix not in target:

direct_path_separator = ":///"

if direct_path_separator not in target:

Fixed in c0052e6

vchudnov-g · 2024-02-01T18:39:08Z

google/api_core/grpc_helpers.py

    return grpc.secure_channel(
        target, composite_credentials, compression=compression, **kwargs
    )


+def _modify_target_for_direct_path(target: str) -> str:


Q: In general, an API endpoint could depend on the URL path and not just the host+port. Is this true of Google APIs? If so, where do we deal with the path part of the URI? (I realize these functions specify only host+port as inputs, so it's clear what they expect and they're doing the right thing, but I was wondering about this more general question.)

If so, where do we deal with the path part of the URI?

I believe this is part of the transcode method

Transcodes a grpc request pattern into a proper HTTP request following the rules outlined here,

python-api-core/google/api_core/path_template.py

Lines 250 to 259 in b72929f

def transcode(http_options, message=None, **request_kwargs):

"""Transcodes a grpc request pattern into a proper HTTP request following the rules outlined here,

https://github.com/googleapis/googleapis/blob/master/google/api/http.proto#L44-L312

Args:

http_options (list(dict)): A list of dicts which consist of these keys,

'method' (str): The http method

'uri' (str): The path template

'body' (str): The body field name (optional)

(This is a simplified representation of the proto option `google.api.http`)

https://github.com/googleapis/gapic-generator-python/blob/b4e6cfc0be8594e2575910699389600f5e92e552/gapic/templates/%25namespace/%25name_%25version/%25sub/services/%25service/transports/rest.py.j2#L372

vchudnov-g · 2024-02-01T18:45:33Z

google/api_core/grpc_helpers_async.py

@@ -21,7 +21,7 @@
 import asyncio
 import functools

-from typing import Generic, Iterator, AsyncGenerator, TypeVar
+from typing import AsyncGenerator, Generic, Iterator, Optional, TypeVar


General notes:

The comments from the sycn version apply here too

Just a note for the future (not blocking this PR): a lot of the code seems identical or very similar to the sync code. We should consolidate sensibly to eliminate duplication. (I just filed consolidate duplicated code #593 to track more such instances)

Thanks for opening that issue! I also made changes to the async code based on the sync code feedback.

vchudnov-g · 2024-02-01T18:47:05Z

tests/unit/test_grpc_helpers.py

-def test_create_channel_implicit(grpc_secure_channel, default, composite_creds_call):
+def test_create_channel_implicit(
+    grpc_secure_channel,
+    default,


default is too generic a name. I assume this is meant to take the google.auth.default you have patched in the decorators. Is there any way this parameter name could be made more descriptive (like auth-default, say), or does @mock preclude that?

(I realize this was pre-existing, so no need to spend too much time on this. But if it's easy...)

Fixed in d63402e

vchudnov-g · 2024-02-01T19:05:21Z

tests/unit/test_grpc_helpers.py

@@ -365,52 +365,92 @@ def test_wrap_errors_streaming(wrap_stream_errors):
    wrap_stream_errors.assert_called_once_with(callable_)


-@mock.patch("grpc.composite_channel_credentials")
+@pytest.mark.parametrize(


Would it be worth making this test function take a target parameter, and then parametrizing it as we do for test_create_channel_implicit_with_default_host below? target seems to be used the same way in both places, and this would allow us to check here the dns:/// and another-c2p:/// cases you parametrize below.

Done in 396d8fa

vchudnov-g · 2024-02-01T19:11:24Z

tests/asyncio/test_grpc_helpers_async.py

@@ -298,48 +298,82 @@ def test_wrap_errors_streaming(wrap_stream_errors):
    wrap_stream_errors.assert_called_once_with(callable_)


-@mock.patch("grpc.composite_channel_credentials")
+@pytest.mark.parametrize(


Same comments as in sync version apply here.

Done in 396d8fa

product-auto-label bot added the size: m Pull request size is medium. label Jan 26, 2024

parthea force-pushed the add-support-for-direct-path branch 5 times, most recently from 676ca58 to e777b53 Compare January 31, 2024 00:56

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Jan 31, 2024

parthea force-pushed the add-support-for-direct-path branch from e777b53 to 0e974b6 Compare January 31, 2024 01:09

parthea mentioned this pull request Jan 31, 2024

Add support for using ssl_credentials and attempt_direct_path in google.api_core.grpc_helpers.create_channel #590

Open

parthea force-pushed the add-support-for-direct-path branch 2 times, most recently from c4f537d to 257d4df Compare January 31, 2024 01:22

feat: Add attempt_direct_path argument to create_channel

421becd

parthea force-pushed the add-support-for-direct-path branch from 1d40546 to 421becd Compare January 31, 2024 01:28

parthea added 2 commits January 30, 2024 20:28

Merge branch 'main' into add-support-for-direct-path

65b994e

add more test cases

b1010ea

parthea marked this pull request as ready for review January 31, 2024 01:33

parthea requested review from a team as code owners January 31, 2024 01:33

parthea assigned vchudnov-g Jan 31, 2024

parthea added 2 commits January 31, 2024 20:03

fix docstring

9a1957a

fix docstring

b223b85

vchudnov-g reviewed Feb 1, 2024

View reviewed changes

vchudnov-g approved these changes Feb 1, 2024

View reviewed changes

parthea added 6 commits February 4, 2024 14:54

update docstring of attempt_direct_path arg

e7e3cf7

update docstring of target arg

44d5845

Add comment for dns_prefix local variable

e6ea7ec

Set the default value of attempt_direct_path to False

0f3a0e4

simplify conditional statement

60b8acb

use warnings.warn instead of _LOGGER.debug

1a7578d

parthea added 4 commits February 4, 2024 15:06

update docstring of target arg in _modify_target_for_direct_path

af3a21b

s/direct_path_prefix/direct_path_separator

c0052e6

default->google_auth_default

d63402e

parametrize target in def test_create_channel_implicit

396d8fa

parthea force-pushed the add-support-for-direct-path branch from b4a6edf to 396d8fa Compare February 4, 2024 17:48

Merge branch 'main' into add-support-for-direct-path

d3598ee

parthea mentioned this pull request Feb 4, 2024

Add ability to detect Google Compute Engine Environment in _create_composite_credentials #598

Closed

parthea added 3 commits February 4, 2024 17:53

Add github issue for TODO

e9199a0

filter deprecation warning related to grpcio-gcp

d96499f

format docstring

d41ca10

parthea merged commit 94726e7 into main Feb 4, 2024
27 checks passed

parthea deleted the add-support-for-direct-path branch February 4, 2024 20:00

release-please bot mentioned this pull request Feb 4, 2024

chore(main): release 2.17.0 #599

Merged

	set to `True` will result in `ValueError` as it is not yet supported.
	set to `True` will result in `ValueError` as this combination is not yet supported.

	reliability. Outside of GCE, the direct path request may fallback
	reliability. Outside of GCE, the direct path request may fall back

	attempt_direct_path: Optional[bool] = None,
	attempt_direct_path: Optional[bool] = False,

	if ssl_credentials is not None and attempt_direct_path:
	if ssl_credentials and attempt_direct_path:

	target (str): The target service address in the format 'hostname:port', 'dns://hostname' or other
	target (str): The target service address in the format 'hostname[:port]', 'dns://hostname[:port]' or other

		direct_path_prefix = ":///"
		if direct_path_prefix not in target:

	def transcode(http_options, message=None, **request_kwargs):
	"""Transcodes a grpc request pattern into a proper HTTP request following the rules outlined here,
	https://github.com/googleapis/googleapis/blob/master/google/api/http.proto#L44-L312

	Args:
	http_options (list(dict)): A list of dicts which consist of these keys,
	'method' (str): The http method
	'uri' (str): The path template
	'body' (str): The body field name (optional)
	(This is a simplified representation of the proto option `google.api.http`)

feat: Add attempt_direct_path argument to create_channel #583

feat: Add attempt_direct_path argument to create_channel #583

Conversation

parthea commented Jan 26, 2024 • edited Loading

vchudnov-g left a comment

Choose a reason for hiding this comment

vchudnov-g Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vchudnov-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parthea commented Jan 26, 2024 •

edited

Loading

vchudnov-g Jan 31, 2024 •

edited

Loading