Add MonitoredResource to interface by waprin · Pull Request #3386 · googleapis/google-cloud-python

waprin · 2017-05-08T23:01:10Z

This adds MonitoredResource as an option to higher level interface.

I also changed the variable name from resource to entry in make_entry_resource because its' confusing to call it a resource when resource is overloaded.

Tests/docs are TBD, wanted +1 on overall direction then I'll write them.

Some notes:

This is intended to be a non-breaking change to existing users. If you use log_text, log_struct etc the default is still the global resource. However, within a Batch you could also explicitly set it to None and wait to set it until the final commit call.
Copied the Resource class from Monitoring. They are the same proto so we could move it to a common package . We could also use the proto but unclear if that will work with the _gax. This is what @jonparrott suggested and I think is pretty.

cc @liyanhui1228 @duggelz

theacodes

@waprin I anticipate you'll also be making test updates? Both we need to verify that this works with both http and gax flavors.

Sign in to view

        :type timestamp: :class:`datetime.datetime`
        :param timestamp: (Optional) timestamp of event being logged.

+        :type resource :class:``google.cloud.logging.resource`


Sign in to view

+             timestamp, resource))

-    def commit(self, client=None):
+    def commit(self, client=None, resource=None):


Sign in to view

+        ).format(**self.__dict__)
+
+
+class Resource(collections.namedtuple('Resource', 'type labels')):


waprin · 2017-05-08T23:50:14Z

Ya like I said just wanted +1 on overall direction before clean up tests docs since I hate writing all the tests/docs then being asked to totally change everything.

theacodes · 2017-05-09T16:51:01Z

@waprin direction looks good. Let me know when you're ready for another review.

waprin · 2017-05-09T21:51:32Z

@jonparrott ready for review, looks like we have an unrelated BigQuery system test failure.

theacodes

This mostly LGTM, with a few questions. I'd like @dhermes or @lukesneeringer to do a pass as well. :)

Thanks for doing this, @waprin!

Sign in to view

    serious code quality issues.
    """
-    session.interpreter = 'python3.6'
+    session.interpreter = 'python3.5'


Sign in to view

        self.assertEqual(entries[0].payload, text_payload)
        self.assertEqual(entries[0].timestamp, now.replace(tzinfo=UTC))

+    def test_log_text_with_resource(self):


waprin · 2017-05-09T23:33:00Z

System test snafu, forgot they say success without running without service account locally, fixing now.

dhermes

Mostly cosmetic changes, but one primary question:

Why does the Batch have self.resource but it's methods also accept resource different than self.resource?

Sign in to view

+from google.cloud.logging.resource import Resource
+
+
+_GLOBAL_RESOURCE = Resource(type='global', labels={})


Sign in to view

        :type timestamp: :class:`datetime.datetime`
        :param timestamp: (Optional) timestamp of event being logged.

+        :type resource :class:`google.cloud.logging.resource.Resource`


Sign in to view

+        :param resource: (Optional) Monitored resource of the entry, defaults
+                         to the global resource type.
+
+


Sign in to view

+        :param resource: (Optional) Monitored resource of the entry, defaults
+                         to the global resource type.
+
+


Sign in to view

        :param http_request: (optional) info about HTTP request associated with
                             the entry.

+        :type resource :class:``google.cloud.logging.resource.Resource`


Sign in to view

        self.assertEqual(len(batch.entries), 0)

    def test_log_text_defaults(self):
+        from google.cloud.logging.logger import _GLOBAL_RESOURCE


Sign in to view

        self.entries.append(
            ('text', text, labels, insert_id, severity, http_request,
-             timestamp))
+             timestamp, resource))


Sign in to view

        self.entries.append(
            ('struct', info, labels, insert_id, severity, http_request,
-             timestamp))
+             timestamp, resource))


Sign in to view

        self.entries.append(
            ('proto', message, labels, insert_id, severity, http_request,
-             timestamp))
+             timestamp, resource))


Sign in to view

            else:
                raise ValueError('Unknown entry type: %s' % (entry_type,))
+            if resource is not None:
+                info['resource'] = resource._to_dict()


waprin · 2017-05-10T00:30:11Z

@dhermes as far as Batch resource goes...

If you look at the API, when you write multiple entries, you can give each individual entry a resource but also the higher level write call (which we encapsulate as a Batch) a resource. The Batch resource (self.resource) is used wherever entries are missing one. So a few places you suggested we should fall back to self.resource, but really what we want there is just for the entry resource to be missing so that Batch resource overrides it on server side.

Also since all the log_* calls default to the global resource, callers would have to explicitly specify None for the individual resources then set the Batch resource. Debatable whether this is a use case people actually want, @jonparrott seemed skeptical of whether it should go in there but I wanted to keep the client library somewhat aligned with the API. I suppose I could rip it out and just ignore the whole Batch resource aspect altogether and make Resource required on all the entry calls.

theacodes · 2017-05-10T16:21:52Z

@waprin I was skeptical because I didn't see that the api has that functionality. We should match the API. We shouldn't go through and fill in the resource for each entry on our end, we should just submit it as part of the request to the server (in both gax and http).

waprin · 2017-05-10T16:44:53Z

Right, I believe the way I did it best matches the API semantics. Only setting it at the Batch level is also not right because your Batch might contain entries of different resource type.

Fixing coverage now...

Sign in to view

    :type http_request: dict
    :param http_request: (optional) info about HTTP request associated with
                         the entry
+    :type resource :class:`google.cloud.logging.resource.Resource`


Sign in to view

    :param http_request: (optional) info about HTTP request associated with
                         the entry
+
+    :type resource :class:`google.cloud.logging.resource.Resource`


Sign in to view

    :type client: :class:`google.cloud.logging.client.Client`
    :param client: The client to use.
+
+    :type resource: :class:`google.cloud.logging.resource.Resource`


Sign in to view

+        entry = {
            'logName': self.full_name,
-            'resource': {'type': 'global'},
+            'resource': resource._to_dict()


Sign in to view

        self.entries.append(
            ('text', text, labels, insert_id, severity, http_request,
-             timestamp))
+             timestamp, resource))


Sign in to view

    test runs (not system test runs), and then erases coverage data.
    """
-    session.interpreter = 'python3.6'
+    session.interpreter = 'python3.5'


Sign in to view

@@ -1,4 +1,4 @@
-# Copyright 2016 Google Inc.
+# Copyright 2017 Google Inc.


Sign in to view

+            labels={
+                'module_id': 'default',
+                'version_id': 'test'
+        })


Sign in to view

+                'labels': {
+                    'module_id':'default',
+                    'version': 'test'
+        }})


Sign in to view

            'resource': {
                'type': 'global',
+                'labels': {},
+


waprin · 2017-05-10T20:44:00Z

@jonparrott @dhermes docs built, just looked carefully at all the styles/docs issue and all lgtm so this is ready for another pass

Sign in to view

    :type http_request: dict
    :param http_request: (optional) info about HTTP request associated with
                         the entry
+    :type resource: :class:`~google.cloud.logging.resource.Resource`


Sign in to view

                             the entry.

+        :type resource: :class:`~google.cloud.logging.resource.Resource`
+        :param resource: Monitored resource of the entry


Sign in to view

        :param timestamp: (optional) timestamp of event being logged.
+
+        :type resource: :class:`~google.cloud.logging.resource.Resource`
+        :param resource: (Optional) Monitored resource of the entry


waprin · 2017-05-11T16:24:38Z

@dhermes ready for another pass/merge

theacodes

Looks almost good to me, two outstanding questions.

Sign in to view

                             labels=None, insert_id=None, severity=None,
-                             http_request=None, timestamp=None):
+                             http_request=None, timestamp=None,
+                             resource=_GLOBAL_RESOURCE):


Sign in to view

        self.assertEqual(entries[0].payload, text_payload)
        self.assertEqual(entries[0].timestamp, now.replace(tzinfo=UTC))

+    def test_log_text_with_resource(self):


waprin · 2017-05-11T17:50:07Z

System tests default to whatever the Client defaults to which is usually gRPC unless it's not supported. It seems a reasonable idea to parametrize the system tests to run with gRPC on and off, but don't think that's particular to these tests. Will create separate issue.

ghost · 2017-05-24T06:59:24Z

Hey guys, when will this be released in the google-cloud-logging package?

ghost · 2017-05-24T15:17:43Z

@lukesneeringer I can do a release today. WDYT?

ghost · 2017-06-22T17:12:24Z

@tseaver I've had an internal request to release this Logging fix ASAP. Since you're planning to do new releases this week, this should be covered, but I just wanted to make sure.

ghost · 2017-06-23T20:30:21Z

@bjwatson Still on my radar: at this point I will be making releases over the weekend, to catch up with some unplanned sitting-in-waiting-room time this week.

ghost · 2017-06-23T20:33:16Z

Thanks @tseaver! Sorry to hear about excessive waiting room time this week.

Add MonitoredResource to interface

c9c0733

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

waprin requested a review from theacodes May 8, 2017 23:01

theacodes self-assigned this May 8, 2017

theacodes suggested changes May 8, 2017

View reviewed changes

Bill Prin added 2 commits May 9, 2017 14:15

Add tests, fix stuff

bb8bf4a

more docs fixups

5559ba9

waprin changed the title ~~[DO NOT MERGE] Add MonitoredResource to interface~~ Add MonitoredResource to interface May 9, 2017

theacodes approved these changes May 9, 2017

View reviewed changes

dhermes reviewed May 9, 2017

View reviewed changes

Bill Prin added 2 commits May 9, 2017 17:20

system tests fix and dhermes review

46eff7a

fix year

02d5ff3

Bill Prin added 2 commits May 9, 2017 17:33

style fixups

ce6f32f

style fixups

cf0bd5c

Bill Prin added 3 commits May 10, 2017 09:51

make entry resource resource is never none

1aec1e8

style mixup

23f3c90

Add coverage

de04ce3

dhermes reviewed May 10, 2017

View reviewed changes

Bill Prin added 5 commits May 10, 2017 11:28

fix cover

762983c

missing tilde

b0ce33a

dhermes comments

040bd1c

fix 3.6

bcf54f8

dhermes

84ed6ac

Bill Prin added 5 commits May 10, 2017 12:34

fix nox

8df8c83

fix global nox

4ba34a0

touchups

4d1ee71

style touchups

6bb5ddd

style fixup

a2a6be2

dhermes reviewed May 10, 2017

View reviewed changes

Bill Prin added 2 commits May 10, 2017 14:56

dhermes comments

ef54bc1

tweak docs

09c4d90

theacodes reviewed May 11, 2017

View reviewed changes

theacodes approved these changes May 11, 2017

View reviewed changes

theacodes merged commit 20f3368 into master May 11, 2017

theacodes deleted the logging_resource branch May 11, 2017 17:52

theacodes mentioned this pull request May 11, 2017

Python Logging API always defaults to Custom logs #2673

Closed

waprin mentioned this pull request May 11, 2017

Should system tests be parametrized to run under both HTTP and gRPC? #3403

Closed

tseaver mentioned this pull request Jun 26, 2017

Prep logging-1.1.0 release. #3528

Merged

		).format(**self.__dict__)


		class Resource(collections.namedtuple('Resource', 'type labels')):

		from google.cloud.logging.resource import Resource


		_GLOBAL_RESOURCE = Resource(type='global', labels={})

		:param resource: (Optional) Monitored resource of the entry, defaults
		to the global resource type.

		@@ -1,4 +1,4 @@
		# Copyright 2016 Google Inc.
		# Copyright 2017 Google Inc.

Conversation

waprin commented May 8, 2017

Uh oh!

theacodes left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

waprin commented May 8, 2017

Uh oh!

theacodes commented May 9, 2017

Uh oh!

waprin commented May 9, 2017

Uh oh!

theacodes left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

waprin commented May 9, 2017

Uh oh!

dhermes left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!