CallOptions kwargs doesn't work

[geigerj]

What

The CallOptions kwarg seems to be passed to metadata instead of being used as arbitrary optional args. See: https://github.com/googleapis/gax-ruby/blob/e8c66e40fe2881cf9b606f33acf4e8b7d13cefdc/lib/google/gax/api_callable.rb#L376:

Why

Users may want to use gRPC optional arguments that we don't explicitly support through GAX. In particular, I encountered this bug trying to use the gRPC setting return_op:

require 'google/cloud/logging'

client = Google::Cloud::Logging::V2::LoggingServiceV2Client.new
options = Google::Gax::CallOptions.new kwargs: {return_op: true}
client.list_logs 'projects/gapic-test', options: options

# Results in:
#   Google::Gax::RetryError: GaxError Exception occurred in retry method that was not classified as transient, caused by Header values must be of type string or array
#   ...

[landrito]

Ack. Can you add a priority label to this?

[geigerj]

Unfortunately not, but treat it as P2. (I'll get Luke to give me admin permissions so that I can create priority labels.)

[jbolinger]

I'm not sure that there is currently a way to pass "return_op: true" to the grpc layer if we want to allow this. kwargs are passed as the metadata parameter from gax and I don't see a way to set return_op:

https://github.com/grpc/grpc/blob/0819ff56f5b7bed6a351db8913d7bb75be0e4d95/src/ruby/lib/grpc/generic/service.rb#L170

https://github.com/grpc/grpc/blob/0819ff56f5b7bed6a351db8913d7bb75be0e4d95/src/ruby/lib/grpc/generic/client_stub.rb#L148-L153

[geigerj]

@jbolinger Right. The issue is that we should not be passing kwargs as the metadata parameter; this is a mistake. Rather, it should be "double splatted" out into arbitrary keyword arguments of the gRPC stub method.

That is,

my_gapic_client.my_rpc(request, Google::Gax::CallOptions.new(
  kwargs: {
    metadata: { 'x-some-header' => 'some value' },
    return_op: true,
    some_other_parameter: some_other_argument
  }
))

should trigger the gRPC stub call

my_grpc_stub.my_rpc(
  ...,
  metadata: { 'x-some-header' => 'some value' },
  return_op: true,
  some_other_parameter: some_other_argument
)

[geigerj]

Note: a decent amount of code in google-cloud-ruby appears to rely on this buggy behavior, so we should be very careful with the release: https://github.com/GoogleCloudPlatform/google-cloud-ruby/search?utf8=%E2%9C%93&q=calloptions.new

It may be helpful to fall back on the old behavior if it looks like metadata is being bound directly to the kwargs parameter so as not to break any old clients that upgrade their GAX dependency.

[jbolinger]

If that's the case why not document kwargs as metadata, leave it as is, and add the other fields either individually or as another splat with a different name? We could eventually fix the unfortunate name - would anyone but us care?

Is there currently anything used as kwargs that is not metadata?

[geigerj]

Is there currently anything used as kwargs that is not metadata?

By necessity of the bug, no. :man_shrugging:

If that's the case why not document kwargs as metadata, leave it as is, and add the other fields either individually or as another splat with a different name? We could eventually fix the unfortunate name - would anyone but us care?

I think it would be a bit more than a little "unfortunate" if the parameter called kwargs that is documented as passing kwargs to the call instead passed metatdata, and some other parameter passed the kwargs. It's also confusing because this privileges metadata above other kwargs for no reason other than historical accident. For now (and this might not be true in the future), we control both all the library code and all of the code that calls it; why not make it right?

I am open to introducing a new parameter (say, stub_kwargs) as long as we deprecate kwargs and raise a warning when it's used. (We should still update google-cloud-ruby in this case so that users don't wind up triggering the warning when they update their dependencies.)

[jbolinger]

Cleaning up - not as relevant since #118 and the more recent interceptor customization. Will reopen if a need arises.