Google-cloud-python: GAX can't log nested structs

[waprin]

This is the root issue in #2521 , if we feel it's the same issue can just rename that one. But when using gax,

from google.cloud import logging
client = logging.Client()
logger = client.logger('errors')

payload = {
    'a': {
        'b': 'c',
    }
}
logger.log_struct(payload)

produces:

  File "e.py", line 22, in <module>
    logger.log_struct(payload)
  File "/Users/waprin/.virtualenvs/tasks/lib/python2.7/site-packages/google/cloud/logging/logger.py", line 226, in log_struct
    client.logging_api.write_entries([entry_resource])
  File "/Users/waprin/.virtualenvs/tasks/lib/python2.7/site-packages/google/cloud/logging/_gax.py", line 108, in write_entries
    entry_pbs = [_log_entry_mapping_to_pb(entry) for entry in entries]
  File "/Users/waprin/.virtualenvs/tasks/lib/python2.7/site-packages/google/cloud/logging/_gax.py", line 607, in _log_entry_mapping_to_pb
    entry_pb.json_payload[key] = value
  File "/Users/waprin/.virtualenvs/tasks/lib/python2.7/site-packages/google/protobuf/internal/well_known_types.py", line 732, in __setitem__
    _SetStructValue(self.fields[key], value)
  File "/Users/waprin/.virtualenvs/tasks/lib/python2.7/site-packages/google/protobuf/internal/well_known_types.py", line 702, in _SetStructValue
    raise ValueError('Unexpected type')
ValueError: Unexpected type

[tseaver]

@dhermes I don't think this is actually a GAX/gRPC issue: our own _log_entry_mapping_to_pb function is likely at fault.

[daspecster]

It looks like this causes jsonPayload to be set for when _log_entry_mapping_to_pb() is called later.

This mapping in _log_entry_mapping_to_pb() should probably either be recursive or support some number of levels.

    if 'jsonPayload' in mapping:
        for key, value in mapping['jsonPayload'].items():
            entry_pb.json_payload[key] = value

@dhermes & @waprin WDYT of this over #2553?

[waprin]

SGTM. Bit surprising that that recursively setting a struct needs to be done manually, still trying to understand it.

[daspecster]

@waprin, it looks like the generated code side only accepts bool, text, and number types.

However the docs make it sound like jsonPayload should just accept the object.
https://cloud.google.com/logging/docs/api/ref_v2beta1/rest/v2beta1/LogEntry

Maybe the payload needs to be serialized first?

@bjwatson do you know if this is a bug we should add on googleapis/googleapis or if jsonPayload needs to be converted to a string first?

[bjwatson]

@daspecster I think this is because json_payload is not defined in the proto as map<string, string> like labels is. Instead it is defined as google.protobuf.Struct, which adds one more level to the object hierarchy.

Change this code:

    if 'jsonPayload' in mapping:
        for key, value in mapping['jsonPayload'].items():
            entry_pb.json_payload[key] = value

To something like:

    if 'jsonPayload' in mapping:
        fields = {}
        for key, value in mapping['jsonPayload'].items():
            fields[key] = value
        entry_pb.json_payload = google.protobuf.Struct(fields)

[bjwatson]

I flagged this as an opportunity to improve the LoggingServiceV2Api.write_log_entries() method sample in the GAPIC code: https://github.com/googleapis/toolkit/issues/636

[daspecster]

Thanks @bjwatson!

[dhermes]

@bjwatson What's the type of mapping['jsonPayload']? Seems like

        fields = {}
        for key, value in mapping['jsonPayload'].items():
            fields[key] = value

is a really long way of saying:

fields = mapping['jsonPayload'].copy()

[bjwatson]

@dhermes I'm guessing it's a dictionary. It's defined in the gcloud code: https://github.com/GoogleCloudPlatform/google-cloud-python/blob/0aca3f6ed05f3340333f231445a3d8b17d8d8e6c/logging/google/cloud/logging/logger.py#L223

Thanks for the tip on the .copy() function. Seems like a good simplification if it works.

[waprin]

@bjwatson @dhermes still having a bit of trouble getting this right. If I try the suggested solution I get:

 entry_pb.json_payload = Struct(fields)
TypeError: init() takes exactly 1 argument (2 given)

Doesn't seem like I can just construct Struct with a dict. So then I tried:

entry_pb.json_payload = Struct()
for key, value in mapping['jsonValue']:
    entry_pb.json_payload[key] = value

I get

ValueError: May not set values directly, call my_map[key].foo = 5

I see this in the docs

As with embedded message fields, messages cannot be directly assigned into a map value. Instead, to add a message as a map value you reference an undefined key, which constructs and returns a new submessage:

So I see in the Struct proto it is a map<string, Value>. So since Value is a Message I need to actually set its fields explicitly like:

entry_pb.json_payload[key].string_value = "whatever"

Unfortunately the Value is not always a string and so string_value is not always appropriate. We could cast the value to a string but that doesn't seem right as it would be changing the types. What I think I really want is a function that recursively goes through the dict and uses the Python types it sees to set the correct union type for the Value (string_value, struct_value, etc). But it really feels like that code should be in some sort of library and not in this logging specific file. We could throw it in core helpers at a minimum, but maybe this function exists somewhere else already?

Wondering if I'm missing something?

[dhermes]

w00t:

>>> import json
>>> from google.protobuf.json_format import Parse
>>> from google.protobuf.struct_pb2 import Struct
>>> msg = Struct()
>>> result = Parse(json.dumps({'a': {'b': 'c'}}), msg)
>>> result is msg
True
>>> msg
fields {
  key: "a"
  value {
    struct_value {
      fields {
        key: "b"
        value {
          string_value: "c"
        }
      }
    }
  }
}