Add support for parsing log entries w/ 'protoPayload' from resources #1578

tseaver · 2016-03-07T16:39:13Z

Uses #1574 as a base.

Note that we can't feasibly allow writing such entries until we have an answer for #1577.

tseaver · 2016-03-11T17:43:33Z

@dhermes 8d5ae2b is the commit which disables the too-many-lines/too-many-statements limits altogether for test code. That feels cleaner to me than either disabling the limit file-by-file, or repeatedly nudging it upwards.

dhermes · 2016-03-11T19:55:08Z

@tseaver too-many-lines / too-many-statements should not be disabled. They provide valuable feedback when we've maybe gone a little too far. e.g. when the row.py cartel should've been broken up (#1602)

gcloud/logging/entries.py

+    See:
+    https://cloud.google.com/logging/docs/api/ref_v2beta1/rest/v2beta1/LogEntry
+    """
+    _PAYLOAD_KEY = 'protoPayload'


dhermes · 2016-03-11T19:58:04Z

I take it this hasn't been rebased yet?

tseaver · 2016-03-11T20:03:08Z

too-many-lines / too-many-statements

For testing code, I disagree: "it takes as much as it takes" to cover the MUT, and the arbitrary restriction on testcase method length tends to lead to "getting fancy" in testing code.

tseaver · 2016-03-11T20:07:58Z

I take it this hasn't been rebased yet?

It is current with the tip of the logging-api branch.

dhermes · 2016-03-11T20:12:22Z

RE: Rebased, the "Add 'Logger.list_entries'." commit must not have merged cleanly then. Maybe you want to cherry-pick the last two commits?

tseaver · 2016-03-11T20:22:10Z

Github seems confused: e4b4b0d is already present on the logging-api branch, When I try to rebase against logging-api in my local checkout, that commit does not appear anywhere.

dhermes · 2016-03-11T20:35:52Z

Try a bit more to unconfuse it? Maybe re-fetch the remote locally?

tseaver · 2016-03-11T20:48:18Z

How? The histories look identical to me:

$ git pull upstream logging-api 
From github.com:GoogleCloudPlatform/gcloud-python
 * branch            logging-api -> FETCH_HEAD
Already up-to-date.

$ git log -3 upstream/logging-api
commit b8151b1e0f7dbf7b9b5a5beff65ca49042798a99
Merge: 68baa1b e4b4b0d
Author: Tres Seaver <tseaver@palladion.com>
Date:   Fri Mar 11 12:44:00 2016 -0500

    Merge pull request #1574 from tseaver/logging-logger_list_entries

    Add 'Logger.list_entries'.

commit e4b4b0d2d1a6bfb9348ff2f073a9577cdf03c378
Author: Tres Seaver <tseaver@palladion.com>
Date:   Wed Mar 9 16:17:14 2016 -0500

    Add 'Logger.list_entries'.

commit 68baa1b05ac0b15e608397756790bb5f06d14846
Merge: e898bca d7a2e72
Author: Tres Seaver <tseaver@palladion.com>
Date:   Fri Mar 11 11:55:02 2016 -0500

    Merge pull request #1569 from tseaver/logging-client_list_entries

    Add 'Client.list_entries'.

$ git log -3 logging-api
commit b8151b1e0f7dbf7b9b5a5beff65ca49042798a99
Merge: 68baa1b e4b4b0d
Author: Tres Seaver <tseaver@palladion.com>
Date:   Fri Mar 11 12:44:00 2016 -0500

    Merge pull request #1574 from tseaver/logging-logger_list_entries

    Add 'Logger.list_entries'.

commit e4b4b0d2d1a6bfb9348ff2f073a9577cdf03c378
Author: Tres Seaver <tseaver@palladion.com>
Date:   Wed Mar 9 16:17:14 2016 -0500

    Add 'Logger.list_entries'.

commit 68baa1b05ac0b15e608397756790bb5f06d14846
Merge: e898bca d7a2e72
Author: Tres Seaver <tseaver@palladion.com>
Date:   Fri Mar 11 11:55:02 2016 -0500

    Merge pull request #1569 from tseaver/logging-client_list_entries

    Add 'Client.list_entries'.

$ git log -5 logging-parse_protobuf_entries 
commit 124ccf08c929794f2cd4ba5ba9fef3d9fdbf42d3
Author: Tres Seaver <tseaver@palladion.com>
Date:   Mon Mar 7 11:37:16 2016 -0500

    Add support for parsing log entries w/ 'protoPayload' from resources.

    We can't feasibly allow writing such entries until we have an answer for

commit 465699c255e39132cc075e114f44807150d9cce2
Author: Tres Seaver <tseaver@palladion.com>
Date:   Mon Mar 7 11:35:57 2016 -0500

    Disable 'too-many-statements'/'too-many-lines' for test code.

commit b8151b1e0f7dbf7b9b5a5beff65ca49042798a99
Merge: 68baa1b e4b4b0d
Author: Tres Seaver <tseaver@palladion.com>
Date:   Fri Mar 11 12:44:00 2016 -0500

    Merge pull request #1574 from tseaver/logging-logger_list_entries

    Add 'Logger.list_entries'.

commit e4b4b0d2d1a6bfb9348ff2f073a9577cdf03c378
Author: Tres Seaver <tseaver@palladion.com>
Date:   Wed Mar 9 16:17:14 2016 -0500

    Add 'Logger.list_entries'.

commit 68baa1b05ac0b15e608397756790bb5f06d14846
Merge: e898bca d7a2e72
Author: Tres Seaver <tseaver@palladion.com>
Date:   Fri Mar 11 11:55:02 2016 -0500

    Merge pull request #1569 from tseaver/logging-client_list_entries

    Add 'Client.list_entries'.

dhermes · 2016-03-11T20:50:07Z

Looks like you've sorted it out. Only two commits now in this PR.

tseaver · 2016-03-11T20:50:11Z

I just tried a new push -f from the loggin-parse_protobuf_entries branch, which seems to have cleared the stray commit.

dhermes · 2016-03-11T20:53:11Z

For testing code, I disagree: "it takes as much as it takes" to cover the MUT, and the arbitrary restriction on testcase method length tends to lead to "getting fancy" in testing code.

It's not hard to argue that code like the lint-disabled test_upload_from_file_w_explicit_client_resumable is hard to follow. These restrictions give us a signal which is valuable to us when we get too deep in our own code and lose context.

We can't feasibly allow writing such entries until we have an answer for

tseaver · 2016-03-21T20:46:09Z

@dhermes 7c52009 drops suppressing the project-wide 'too-many-statements' check for test code, and instead disables it locally for the one newly-added test: looking at it, I don't see any reason to break it up. PTAL

dhermes · 2016-03-22T01:34:24Z

Only real pending issue is my comment about seeing an example of a proto payload from a real API request. Have you been able to produce one?

tseaver · 2016-03-22T01:55:17Z

From the "Try It" form:

POST https://logging.googleapis.com/v2beta1/entries:list?key={YOUR_API_KEY}

{
 "projectIds": [
  "citric-celerity-697"
 ]
}

returns:

200 OK

- Hide headers -

Cache-Control:  private
Content-Encoding:  gzip
Content-Length:  5768
Content-Type:  application/json; charset=UTF-8
Date:  Tue, 22 Mar 2016 01:52:27 GMT
Server:  ESF
Vary:  Origin, X-Origin, Referer

{
 "entries": [
  {
   "textPayload": "Logging from gcloud-python!",
   "insertId": "2016-03-03|11:28:12.764719-08|10.95.84.10|1527583067",
   "resource": {
    "type": "global"
   },
   "timestamp": "2016-03-03T19:28:12.750563482Z",
   "logName": "projects/citric-celerity-697/logs/hacking-around"
  },
  {
   "insertId": "2016-03-03|11:48:07.740612-08|10.95.212.81|-486515530",
   "jsonPayload": {
    "weather": "cloudy",
    "message": "Now with arbitrary key-values!"
   },
   "resource": {
    "type": "global"
   },
   "timestamp": "2016-03-03T19:48:07.728837588Z",
   "logName": "projects/citric-celerity-697/logs/hacking-around"
  },
  {
   "protoPayload": {
    "@type": "type.googleapis.com/google.cloud.audit.AuditLog",
    "status": {
    },
    "authenticationInfo": {
    },
    "requestMetadata": {
    },
    "serviceName": "bigquery.googleapis.com",
    "methodName": "datasetservice.list",
    "authorizationInfo": [
     {
      "resource": "projects/citric-celerity-697",
      "permission": "bigquery.datasets.list",
      "granted": true
     },
     {
      "resource": "projects/citric-celerity-697/datasets/test_listing_datasets",
      "permission": "bigquery.datasets.get",
      "granted": true
     }
    ],
    "resourceName": "projects/citric-celerity-697/datasets",
    "serviceData": {
     "@type": "type.googleapis.com/google.cloud.bigquery.logging.v1.AuditData",
     "datasetListRequest": {
     }
    }
   },
   "insertId": "52D3EC4A89C80.A5FD1C7.7C0038C9",
   "resource": {
    "type": "bigquery_resource"
   },
   "timestamp": "2016-03-04T20:14:54.160Z",
   "severity": "INFO",
   "logName": "projects/citric-celerity-697/logs/cloudaudit.googleapis.com%2Fdata_access"
  },
...
 ]
}

dhermes · 2016-03-22T01:57:22Z

Really helpful! Thanks

dhermes · 2016-03-22T01:57:57Z

LGTM (though I still think reducing the test boilerplate for the do-nothing subclasses would be a net positive)

Addresses: #1578 (comment)

tseaver · 2016-03-22T15:25:53Z

@dhermes I changed gcloud.logging.test_entries to test _BaseEntry directly, dropping tests for the method-less derived classes. I will merge when Travis passes.

dhermes · 2016-03-22T15:27:51Z

SGTM thanks

Add support for parsing log entries w/ 'protoPayload' from resources

tseaver added the api: logging Issues related to the Cloud Logging API. label Mar 7, 2016

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

tseaver force-pushed the logging-api branch from 87bccf9 to d38fd23 Compare March 8, 2016 15:49

tseaver mentioned this pull request Mar 8, 2016

Where are the allowed log entry protobuf types documented? #1577

Closed

tseaver assigned dhermes Mar 11, 2016

dhermes reviewed Mar 11, 2016
View reviewed changes

gcloud/logging/entries.py

See:

https://cloud.google.com/logging/docs/api/ref_v2beta1/rest/v2beta1/LogEntry

"""

_PAYLOAD_KEY = 'protoPayload'

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Add support for parsing log entries w/ 'protoPayload' from resources.

7c52009

We can't feasibly allow writing such entries until we have an answer for

Test '_BaseEntry' directly, drop tests for methodless subclasses.

fd8965c

Addresses: #1578 (comment)

tseaver added a commit that referenced this pull request Mar 22, 2016

Merge pull request #1578 from tseaver/logging-parse_protobuf_entries

9f37a05

Add support for parsing log entries w/ 'protoPayload' from resources

tseaver merged commit 9f37a05 into googleapis:logging-api Mar 22, 2016

tseaver deleted the logging-parse_protobuf_entries branch March 22, 2016 15:34

dhermes mentioned this pull request Apr 1, 2016

Upgrading version to 0.12.0 #1689

Merged

theacodes unassigned dhermes Sep 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for parsing log entries w/ 'protoPayload' from resources #1578

Add support for parsing log entries w/ 'protoPayload' from resources #1578

tseaver commented Mar 7, 2016

tseaver commented Mar 11, 2016

dhermes commented Mar 11, 2016

This comment was marked as spam.

This comment was marked as spam.

dhermes commented Mar 11, 2016

tseaver commented Mar 11, 2016

tseaver commented Mar 11, 2016

dhermes commented Mar 11, 2016

tseaver commented Mar 11, 2016

dhermes commented Mar 11, 2016

tseaver commented Mar 11, 2016

dhermes commented Mar 11, 2016

tseaver commented Mar 11, 2016

dhermes commented Mar 11, 2016

tseaver commented Mar 21, 2016

dhermes commented Mar 22, 2016

tseaver commented Mar 22, 2016

dhermes commented Mar 22, 2016

dhermes commented Mar 22, 2016

tseaver commented Mar 22, 2016

dhermes commented Mar 22, 2016

Add support for parsing log entries w/ 'protoPayload' from resources #1578

Add support for parsing log entries w/ 'protoPayload' from resources #1578

Conversation

tseaver commented Mar 7, 2016

tseaver commented Mar 11, 2016

dhermes commented Mar 11, 2016

This comment was marked as spam.

This comment was marked as spam.

dhermes commented Mar 11, 2016

tseaver commented Mar 11, 2016

tseaver commented Mar 11, 2016

dhermes commented Mar 11, 2016

tseaver commented Mar 11, 2016

dhermes commented Mar 11, 2016

tseaver commented Mar 11, 2016

dhermes commented Mar 11, 2016

tseaver commented Mar 11, 2016

dhermes commented Mar 11, 2016

tseaver commented Mar 21, 2016

dhermes commented Mar 22, 2016

tseaver commented Mar 22, 2016

dhermes commented Mar 22, 2016

dhermes commented Mar 22, 2016

tseaver commented Mar 22, 2016

dhermes commented Mar 22, 2016