Gracefully continue if LogEntry.proto_payload type URL is not in registry. #3270

dhermes · 2017-04-03T23:40:30Z

@lukesneeringer @tseaver I am unfortunately "out of time" until Wednesday so if either of you want to take this over the finish line (e.g. unit tests and maybe a system test with an unregistered type URL?) I would not protest.

…stry. Fixes googleapis#2674.

logging/google/cloud/logging/_gax.py

+    """
+    try:
+        return MessageToDict(entry_pb)
+    except TypeError:


logging/google/cloud/logging/_gax.py

@@ -270,11 +272,13 @@ def sink_update(self, project, sink_name, filter_, destination):
        path = 'projects/%s/sinks/%s' % (project, sink_name)
        sink_pb = LogSink(name=path, filter=filter_, destination=destination)
        try:
-            self._gax_api.update_sink(path, sink_pb, options=options)
+            sink_pb = self._gax_api.update_sink(path, sink_pb, options=options)


logging/google/cloud/logging/_gax.py

@@ -418,11 +424,14 @@ def metric_update(self, project, metric_name, filter_, description):
        metric_pb = LogMetric(name=path, filter=filter_,
                              description=description)
        try:
-            self._gax_api.update_log_metric(path, metric_pb, options=options)
+            metric_pb = self._gax_api.update_log_metric(


logging/google/cloud/logging/_http.py

@@ -293,7 +293,7 @@ def sink_update(self, project, sink_name, filter_, destination):
            'filter': filter_,
            'destination': destination,
        }
-        self.api_request(method='PUT', path=target, data=data)
+        return self.api_request(method='PUT', path=target, data=data)


logging/google/cloud/logging/_http.py

@@ -428,7 +428,7 @@ def metric_update(self, project, metric_name, filter_, description):
            'filter': filter_,
            'description': description,
        }
-        self.api_request(method='PUT', path=target, data=data)
+        return self.api_request(method='PUT', path=target, data=data)


logging/google/cloud/logging/entries.py

@@ -74,7 +77,13 @@ class _BaseEntry(object):
    """
    def __init__(self, payload, logger, insert_id=None, timestamp=None,
                 labels=None, severity=None, http_request=None):
-        self.payload = payload
+        if isinstance(payload, any_pb2.Any):


lukesneeringer · 2017-04-04T15:24:25Z

I will try to get this over the line today if I can.

…rsing.

dhermes

PTAL @lukesneeringer (and @tseaver and anyone else interested in this PR)

logging/google/cloud/logging/_gax.py

@@ -270,11 +272,13 @@ def sink_update(self, project, sink_name, filter_, destination):
        path = 'projects/%s/sinks/%s' % (project, sink_name)
        sink_pb = LogSink(name=path, filter=filter_, destination=destination)
        try:
-            self._gax_api.update_sink(path, sink_pb, options=options)
+            sink_pb = self._gax_api.update_sink(path, sink_pb, options=options)


logging/google/cloud/logging/entries.py

+            self.payload_pb = self.payload
+            self.payload = None
+        else:
+            self.payload_pb = None


logging/tests/system.py

+            self.assertIsNone(protobuf_entry.payload)
+            self.assertIsInstance(protobuf_entry.payload_pb, any_pb2.Any)
+            self.assertEqual(protobuf_entry.payload_pb.type_url, type_url)
+        else:


logging/tests/system.py

@@ -108,6 +108,7 @@ class TestLogging(unittest.TestCase):
            'precipitation': False,
        },
    }
+    TYPE_FILTER = 'protoPayload.@type = "{}"'


logging/tests/system.py

+        filter_ = self.TYPE_FILTER.format(type_url)
+        entry_iter = iter(
+            Config.CLIENT.list_entries(page_size=1, filter_=filter_))
+        protobuf_entry = next(entry_iter)


logging/tests/unit/test__gax.py

+                },
+                'requestTime': {
+                    'seconds': 1491000125,
+                }


logging/tests/unit/test__gax.py

+                'requestTime': {
+                    'seconds': 1491000125,
+                }
+            }


…p module.

dhermes · 2017-04-06T17:12:08Z

@tseaver @lukesneeringer Do you have any issues?

@waprin Can you vet my newly added system test?

lukesneeringer · 2017-04-06T17:57:32Z

No issues. Approved.

waprin · 2017-04-06T18:37:19Z

@dhermes tied up today but can review your tests and try to make mine less flaky (and loop back to any other pending issues for me in this repo) tomorrow.

dhermes · 2017-04-06T18:42:07Z

Thanks @waprin. AFAIK the other ones aren't very flaky any more (or not at all?).

Let's get this merged and then @waprin you can "verify" the new system test after the fact?

waprin · 2017-04-14T01:07:15Z

@dhermes sorry for delay, nice PR. One quick note about the system test - I think it's fine but in general the gotcha to be aware of with Logging is that you can get an empty page followed by a page with results. The nextPageToken is used as a sort of "pending" sign (not a fan of this but that's how it works). So in theory entry = next(iter) could fail if the query is too slow, really you always want to wrap it it in a loop (not sure if there is an elegant way to have client library do this for you). Now usually you only get slow queries when looking through huge amounts of logs like all of the ones for a project but it's something to be aware of. Again not really an issue just giving you a heads up on something that's burned me and others.

dhermes · 2017-04-14T02:11:31Z

@waprin I'll be on the lookout for that if there is a flaky failure.

…stry. (googleapis#3270) * Gracefully continue if LogEntry.proto_payload type URL is not in registry. Fixes googleapis#2674. * Adding unit / system tests for "graceful" continuation on LogEntry parsing. * Docs fix for PR 3270. * Adding rtype/returns to metric_update and sink_update in logging _http module. * Add trailing commas.

Gracefully continue if LogEntry.proto_payload type URL is not in regi…

6684ddd

…stry. Fixes googleapis#2674.

dhermes added the api: logging Issues related to the Cloud Logging API. label Apr 3, 2017

dhermes assigned lukesneeringer and tseaver Apr 3, 2017

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 3, 2017

dhermes commented Apr 3, 2017

View reviewed changes

Adding unit / system tests for "graceful" continuation on LogEntry pa…

9b1cdf8

…rsing.

dhermes commented Apr 6, 2017

View reviewed changes

Docs fix for PR 3270.

7a29cad

dhermes mentioned this pull request Apr 6, 2017

pageSize vs. maxResults #3277

Closed

dhermes added 2 commits April 6, 2017 10:10

Adding rtype/returns to metric_update and sink_update in logging _htt…

79834cd

…p module.

Add trailing commas.

2840791

dhermes merged commit ff3063e into googleapis:master Apr 6, 2017

dhermes deleted the fix-2674 branch April 6, 2017 18:42

dhermes mentioned this pull request Apr 6, 2017

Cut release 1.0.0 of logging package. #3278

Merged

Gracefully continue if LogEntry.proto_payload type URL is not in registry. #3270

Gracefully continue if LogEntry.proto_payload type URL is not in registry. #3270

Conversation

dhermes commented Apr 3, 2017

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.

lukesneeringer commented Apr 4, 2017

dhermes left a comment

Choose a reason for hiding this comment

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 commented Apr 6, 2017

lukesneeringer commented Apr 6, 2017

waprin commented Apr 6, 2017

dhermes commented Apr 6, 2017

waprin commented Apr 14, 2017

dhermes commented Apr 14, 2017