Bump Python common-protos package to 1.5.0

[bjwatson]

@jmuk @swcloud @garrettjonesgoogle @michaelbausor @pongad @jskeet You will want to regenerate your own common proto packages, with this change. We should have always included this, since Logging can depend upon it.

The issue was spotted by a user: https://github.com/GoogleCloudPlatform/google-cloud-python/issues/2572

See https://github.com/googleapis/api-client-staging/pull/89 for the Python changes to include GAE Logging.

[jskeet]

It's not clear to me how logging depends on this... if this is Logging.V2, that has a struct_payload and an any_payload... both of which can be filled in with arbitrary information without us having to include anything special in our common protos.

If anything special is needed for this - e.g. a TypeRegistry - then it would be needed for other protos too, including user-defined ones.

[jmuk]

I'm not yet convinced this needs to be in a common protos, because logging requires it for unpacking an Any value. It looks like it is an implicit dependency from logging to appengine/logging. I'm also wondering what else. Potentially other types of logging could be there.

(By the way, ProtoBuf.js does not support unpack of Any, therefore this problem won't happen at all).

[bjwatson]

@jmuk is correct that this is required for interpreting an Any value. Also, I think that @jskeet is correct that users would need to add their own user-defined types to a registry.

Nevertheless, this is for one of our types: we document the RequestLog class as part of our Stackdriver Logging documentation, so we should provide code that ensures it works.

@jmuk I surveyed the other "API Shared Types" in that documentation, and this is it. Everything else is already in common protos or in the Logging gRPC code, except for RequestLog and LogLine, which are defined here for AppEngine Logging.

[jskeet]

Just adding the type into the package won't automatically add it into a type registry, at least not in C#... but actually, I'm not sure why it's being a problem at all. Is there any JSON conversion going on here? Because if not, it should be possible to pack the message without anything else...

[bjwatson]

Our understanding is that Python protoc loads the type into the registry: https://github.com/googleapis/api-client-staging/pull/89/files#diff-b9b18c5536b5968e3433bd00c724a26bR455.

The Python proto code is converting JSON into an appropriate type of Any class (code), and that's why it needs proto code for the target type.

[codecov-io]

Current coverage is 91.23% (diff: 100%)

Merging #147 into master will not change coverage

@@             master       #147   diff @@
==========================================
  Files             3          3          
  Lines           719        719          
  Methods         150        150          
  Messages          0          0          
  Branches        116        116          
==========================================
  Hits            656        656          
  Misses           63         63          
  Partials          0          0          

Powered by Codecov. Last update 1d19d9e...63c7ba9

[jmuk]

I think @jskeet is questioning why not using json_payload if it is JSON. Is that a good way to process it through 'Any'?

Also, what about AuditLogs? https://cloud.google.com/logging/docs/audit/api/

[jmuk]

I chatted with @bjwatson offline. We see that this situation is consistent with iam-v1 -- I was unsure this is the right solution because appengine/logging has v1 version in it. Since Python's common protos package is going to include iam-v1 -- then including appengine/logging/v1 makes more sense to me. That would be up to the packaging policy on the language.

That said, I have no further questions on this PR. Please go with this.

[bjwatson]

@jskeet @jmuk I see that json_payload should be used for user-defined types, but Google supports proto_payload for a certain types: https://cloud.google.com/logging/docs/view/logs_index#common

Note that RequestLog is one of them. @jmuk is correct about Audit Log Datatypes. I created an issue to track this: https://github.com/googleapis/googleapis/issues/187

@jskeet I'm going to go ahead with the Python fix, but wait to hear back from you or any others before merging this PR.

[jskeet]

I'm confused as to why JSON comes into this at all. In C# if I wanted to log something I'd just build the message to log, pack in an Any and I'd be done.

If Python really needs to go via JSON, then this sounds like a reasonable fix - but I doubt that it's required in all languages.

[bjwatson]

@jskeet You would still need the generated proto code for RequestLog in order to build the message.

BTW, this use case for Python is using the proto_payload field. The google-cloud-python code is just taking care of parsing the JSON into a message (using the Python Protobuf Parse(...) method) before calling the gRPC method. The user would run into the same problem even if they tried building the RequestLog message themselves.

[jskeet]

Yes, you'd need the generated proto code - but I'm assuming that if you need that, you'd have it... and as C# doesn't run on AppEngine Standard anyway, it would be a pretty unusual scenario, basically. But I'm confused by this bit:

The user would run into the same problem even if they tried building the RequestLog message themselves.

Surely only if they'd done so by parsing JSON. If they don't touch JSON, there should be no need for a type registry. At least, there isn't in C# - again, I don't know the details of the Python proto code, but I wouldn't have thought JSON would be a requirement there.

[bjwatson]

I'm talking about building the RequestLog message by hand, as you suggested, rather than going through some kind of JSON parsing. And if a user needs to do that, then we should provide the generated proto code, because RequestLog is a Google message.

Since C# doesn't run on AppEngine Standard, then maybe it's not such a big deal to not provide RequestLog for that language. The reason I pinged all the language leads, is so that y'all can each make this determination for your language.

The requirement for supporting JSON in Python predated me joining the team. It's already baked into the google-cloud-python code. @dhermes or @tseaver could provide more insight about that.

[dhermes]

The requirement for supporting JSON in Python

It's not our requirement, just a historical artifact. IIRC the original impl. was done using JSON-over-HTTP because gRPC wasn't ready yet.

I am happy to delete all the JSON-over-HTTP code, but I don't know why that is useful?

[bjwatson]

@dhermes That's right! I forgot that Python needs to support both gRPC and HTTP/JSON.

Could the JSON code be removed from the gRPC path in the future without breaking the surface? The main reason would be to make it more efficient, so that it doesn't have to do so many conversions.

I'm guessing the answer is no, since the example in https://github.com/GoogleCloudPlatform/google-cloud-python/issues/2572 has the user passing a Struct to represent a RequestLog, rather than a RequestLog itself. It seems like a JSON-style of specification is supported even when using log_proto.

[dhermes]

Could the JSON code be removed from the gRPC path in the future without breaking the surface?

Yes but this won't help you. If someone has a native protobuf object and they add it as the protoPayload in a log entry, it'll still need to be converted to JSON to be sent JSON-over-HTTP.

[bjwatson]

@dhermes Thanks. I better understand the requirements now for why log_proto is implemented that way.

[bjwatson]

@dhermes convinced me that this should be in its own package. Unfortunately my hands are tied until we declare 2.x for Python googleapis-common-protos. I'm tracking that in https://github.com/googleapis/api-client-staging/issues/90, so that we can make any other necessary breaking changes at the same time.

I'll modify this PR so that the GAE Logging code is conditional just for Python, since I'm committed to that for the moment.

[theacodes]

The inclusion of this proto in the Python library has caused several issues downstream because it creates a google.appengine package:

1) oauth2client's application default credentials checks for the existence of google.appengine. This causes oauth2client to think that it's in app engine standard when it is not, which leads to errors.

2) If googleapis-common-protos is installed on GAE standard, it will shadow the google.appengine package provided by the runtime/sdk.

The google.appengine package should be considered sacred because it is so very difficult for us to make any changes to it. Is there any way we can reverse this change or expose it under a different name?

[bjwatson]

I've done as @jonparrott requested, and removed the GAE Logging dependency. See https://github.com/googleapis/api-client-staging/pull/91 for more discussion.

[geigerj]

It's unfortunate that we need to break semver to do this. If you think this is the best course of action, then LGTM. But I think we should consider that if we're not totally set on what should/shouldn't be in googleapis-common-protos, then we should deprecate googleapis-common-protos 1.x and reset the version back to 0.x.

[theacodes]

Resetting the version would break upgrade logic. Since this proto was never truly used by any downstream clients, I do think this is the best course of action.

On Wed, Nov 2, 2016, 12:06 PM Jacob Geiger notifications@github.com wrote:

It's unfortunate that we need to break semver to do this. If you think this is the best course of action, then LGTM. But I think we should consider that if we're not totally set on what should/shouldn't be in googleapis-common-protos, then we should deprecate googleapis-common-protos 1.x and reset the version back to 0.x.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/googleapis/packman/pull/147#issuecomment-257967499, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPUc7S0DOdCUWlZi7ew0mxcE_dcJow6ks5q6N8pgaJpZM4KlMqf .

[geigerj]

Ok, SGTM then.

[bjwatson]

Thanks. I agree that it's not ideal to break semver; this shouldn't be 1.x, but it is.

My lesson learned is that we need a broader review circle before adding anything to common-protos in the future.