Allows Explicitly Enabling/Disabling GAX for Logging/Pubsub #2553

waprin · 2016-10-17T20:03:35Z

Currently gax can’t log nested structs, so this is a workaround to allow forced HTTP logging.

Workaround to resolve #2521 until #2552 is resolved.

logging/google/cloud/logging/client.py

+    def __init__(self, project=None, credentials=None,
+                 http=None, use_gax=True):
+        super(Client, self).__init__(project, credentials, http)
+        self.use_gax = use_gax


dhermes · 2016-10-17T20:09:20Z

Not sure this will cause coverage to go below 100% because the branches are all implicit in one-line or statements. But I can see the branches. So pretty please write some unit tests?

Also update the logging Client docstring with the new argument (pretty sure Pylint will get mad about that).

daspecster · 2016-10-17T20:14:10Z

What does this add that the DISABLE_GRPC envar doesn't do? Could the workaround just be setting that to True?

dhermes · 2016-10-17T20:20:47Z

@daspecster It's a manual override, since the gRPC client is b0rken (see the issue that @waprin linked to).

waprin · 2016-10-17T20:25:08Z

@daspecster Yes but error reporting currently uses the logging client, it doesn't seem right for one module to be forcing a global environment variable.

daspecster · 2016-10-17T20:28:11Z

Yeah I looked at the issues. If this gets added, then we may want to set a reminder/issue to take it out once the bug in gRPC is resolved right?

Currently gax can’t log nested structs, so this is a workaround to allow forced HTTP logging.

waprin · 2016-10-17T21:44:02Z

@dhermes added unit test and coverage passes.

logging/unit_tests/test_client.py

@@ -16,7 +16,6 @@


 class TestClient(unittest.TestCase):
-


logging/unit_tests/test_client.py

@@ -66,7 +65,6 @@ def _generated_api(*args, **kw):
            return wrapped

        class _GaxLoggingAPI(object):
-


logging/unit_tests/test_client.py

@@ -475,22 +484,19 @@ def create_scoped(self, scope):


 class _DummyLoggingAPI(object):
-


daspecster · 2016-10-19T13:14:23Z

@waprin looks like we have a fix to support that structure.

dhermes · 2016-10-19T15:15:09Z

@waprin I think a few things should happen:

Close this PR
We can have a new PR which allows use_gax as a constructor argument in all GAX / GAPIC / gRPC supported APIs (or re-purpose this PR to do the same)
We fix error reporting / logging by fixing our b0rken code

waprin · 2016-10-19T18:13:52Z

@dhermes SGTM, will just re-purpose this PR

waprin · 2016-10-20T21:41:14Z

@dhermes added pub/sub.

error_reporting/google/cloud/error_reporting/client.py

@@ -106,7 +106,7 @@ def __init__(self, project=None,
                 service=None,
                 version=None):
        self.logging_client = google.cloud.logging.client.Client(
-            project, credentials, http)
+            project=project, credentials=credentials, http=http, use_gax=False)


logging/google/cloud/logging/client.py

+
+    :type use_gax: bool or :class:`NoneType`
+    :param use_gax: An optional parameter that explicitly specifies whether
+                    to use the gRPC transport (gax) or HTTP


logging/unit_tests/test_client.py


        with _Monkey(MUT, _USE_GAX=False):
+            client = self._makeOne(self.PROJECT, credentials=_Credentials())
+            conn = client.connection = object()


logging/unit_tests/test_client.py

+
+        creds = _Credentials()
+        with _Monkey(MUT,
+                     _USE_GAX=True):


logging/unit_tests/test_client.py

+    def test_no_gax_ctor(self):
+        from google.cloud.logging.connection import _LoggingAPI
+        from google.cloud.logging import client as MUT
+        from google.cloud._testing import _Monkey


logging/google/cloud/logging/client.py

@@ -71,11 +71,23 @@ class Client(JSONClient):
    :param http: An optional HTTP object to make requests. If not passed, an
                 ``http`` object is created that is bound to the
                 ``credentials`` for the current object.
+
+    :type use_gax: bool or :class:`NoneType`


logging/google/cloud/logging/client.py

@@ -71,11 +71,23 @@ class Client(JSONClient):
    :param http: An optional HTTP object to make requests. If not passed, an
                 ``http`` object is created that is bound to the
                 ``credentials`` for the current object.
+
+    :type use_gax: bool or :class:`NoneType`
+    :param use_gax: An optional parameter that explicitly specifies whether


pubsub/unit_tests/test_client.py


        with _Monkey(MUT, _USE_GAX=False):
+            client = self._makeOne(project=self.PROJECT, credentials=creds)


pubsub/unit_tests/test_client.py


        with _Monkey(MUT, _USE_GAX=False):
+            client = self._makeOne(project=self.PROJECT, credentials=creds)


dhermes

LGTM (merge squash in the UI once Travis is green?)

waprin · 2016-10-21T21:19:45Z

@dhermes yes please squash merge at your convienence

Allows Explicitly Enabling/Disabling GAX for Logging/Pubsub

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 17, 2016

dhermes reviewed Oct 17, 2016

View reviewed changes

Enable forced HTTP for Error Reporting

bda9705

Currently gax can’t log nested structs, so this is a workaround to allow forced HTTP logging.

waprin force-pushed the logging_gax_optional branch from f45bfe2 to c0cd817 Compare October 17, 2016 21:43

daspecster mentioned this pull request Oct 17, 2016

GAX can't log nested structs #2552

Closed

dhermes reviewed Oct 17, 2016

View reviewed changes

waprin force-pushed the logging_gax_optional branch from c0cd817 to d417c1d Compare October 17, 2016 23:59

dhermes review

fceaacf

waprin force-pushed the logging_gax_optional branch from d417c1d to fceaacf Compare October 18, 2016 00:00

waprin changed the title ~~Enable forced HTTP for Error Reporting~~ Add JSONOrGaxClient That Allows Explicitly Enabling/Disabling GAX Oct 20, 2016

waprin force-pushed the logging_gax_optional branch from 3ffc5cd to b834d13 Compare October 20, 2016 21:24

waprin changed the title ~~Add JSONOrGaxClient That Allows Explicitly Enabling/Disabling GAX~~ Allows Explicitly Enabling/Disabling GAX for Logging/Pubsub Oct 20, 2016

waprin force-pushed the logging_gax_optional branch from b834d13 to 950afd1 Compare October 20, 2016 22:30

Add pubs

088e8b4

waprin force-pushed the logging_gax_optional branch from 950afd1 to 088e8b4 Compare October 20, 2016 22:31

dhermes reviewed Oct 21, 2016

View reviewed changes

Bill Prin added 3 commits October 21, 2016 13:59

dhermes review

cbd8155

more dhermes

8bfa396

fix docstrings

20e3f47

dhermes approved these changes Oct 21, 2016

View reviewed changes

dhermes merged commit 1fab756 into googleapis:master Oct 21, 2016

dhermes mentioned this pull request Nov 14, 2016

Upgrading core to version to 0.21.0. #2733

Merged

richkadel pushed a commit to richkadel/google-cloud-python that referenced this pull request May 6, 2017

Merge pull request googleapis#2553 from waprin/logging_gax_optional

344cd8e

Allows Explicitly Enabling/Disabling GAX for Logging/Pubsub

		@@ -66,7 +65,6 @@ def _generated_api(args, *kw):
		return wrapped

		class _GaxLoggingAPI(object):

		@@ -475,22 +484,19 @@ def create_scoped(self, scope):


		class _DummyLoggingAPI(object):


		with _Monkey(MUT, _USE_GAX=False):
		client = self._makeOne(project=self.PROJECT, credentials=creds)

Allows Explicitly Enabling/Disabling GAX for Logging/Pubsub #2553

Allows Explicitly Enabling/Disabling GAX for Logging/Pubsub #2553

Conversation

waprin commented Oct 17, 2016

This comment was marked as spam.

This comment was marked as spam.

dhermes commented Oct 17, 2016

daspecster commented Oct 17, 2016

dhermes commented Oct 17, 2016

waprin commented Oct 17, 2016

daspecster commented Oct 17, 2016

waprin commented Oct 17, 2016

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

daspecster commented Oct 19, 2016

dhermes commented Oct 19, 2016

waprin commented Oct 19, 2016

waprin commented Oct 20, 2016

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

dhermes left a comment

Choose a reason for hiding this comment

waprin commented Oct 21, 2016