Update metadata handling and add block to expose grpc ops internally

[jbolinger]

This is a WIP for a potential new toolkit feature. The main goal is to invoke all internal grpc calls using the return_op=true flag so that any additional grpc information is available and can be trapped in gax, via an optional block. Those blocks will then be used by the generated gapic methods to trap and expose things like response metadata to the caller.

Related toolkit change: https://github.com/googleapis/toolkit/pull/1865

Note: Do not merge until the feature is finalized!

[codecov-io]

Codecov Report

Merging #118 into master will increase coverage by 0.01%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #118      +/-   ##
==========================================
+ Coverage   99.48%   99.49%   +0.01%     
==========================================
  Files          19       19              
  Lines        1937     1990      +53     
==========================================
+ Hits         1927     1980      +53     
  Misses         10       10

Impacted Files	Coverage Δ
lib/google/gax/settings.rb	99% <100%> (+0.03%)	:arrow_up:
spec/google/gax/api_callable_spec.rb	98.43% <100%> (+0.31%)	:arrow_up:
lib/google/gax/api_callable.rb	98.43% <100%> (+0.09%)	:arrow_up:
spec/google/gax/settings_spec.rb	100% <100%> (ø)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 19306ce...243231c. Read the comment docs.

[landrito]

Interesting! Can you provide an example of how a user would use this feature?

[jbolinger]

Yes, sorry I meant to do that but forgot!

So, the end user would do something like this:

output = client.batch_annotate_images(
    [{
         image: {
             source: { image_uri: "gs://gapic-toolkit/President_Barack_Obama.jpg" }
         },
         features: [{ type: :FACE_DETECTION }]
     }],
    options: Google::Gax::CallOptions.new(metadata: { 'x-some-header' => 'some value' })
  )

puts "output is: #{output}"
puts "response metadata are: #{client.last_response_headers}"

The GAPIC layer will be modified to do something like this:

def batch_annotate_images requests, options: nil
    req = {
      requests: requests
    }.delete_if { |_, v| v.nil? }
    req = Google::Gax::to_proto(req, Google::Cloud::Vision::V1::BatchAnnotateImagesRequest)
    @batch_annotate_images.call(req, options) { |op| @last_response_headers = op.metadata }
end

[jbolinger]

I'd also like to enable the following, or something similar, but have to think it through a little more so we don't break the old way and it's consistent with the other APIs

client.batch_annotate_images(...) do | response, metadata |
   # do stuff with the result...
done

[geigerj]

I still think that passing return_op: true for every gRPC call, including the majority of calls where we don't care about the trailers and just retrieve the operation result, seems ugly. But per offline discussion, I won't block the review over it.

Just for confirmation, @apolcyn -- is there any downside to our libraries setting return_op: true and blocking on the result for calls where we don't actually need the operation, as opposed to not setting return_op?

[apolcyn]

Just for confirmation, @apolcyn -- is there any downside to our libraries setting return_op: true and blocking on the result for calls where we don't actually need the operation, as opposed to not setting return_op?

Once the RPC has been started, the behavior of return_op: true, and return_op: false will be the same. return_op true effectively just allows splitting up instantiation and RPC invocation, and effectively just gives a different surface API.

[jbolinger]

@geigerj @landrito - not super urgent, but bumping so we can get this merged relatively soon.

Any outstanding concerns? The only issue I can recall is if we wanted to return nil when a block was passed to prevent exposing the same thing through the block and the return value. I don't really have an opinion either way. Thoughts? I miss anything else?

[geigerj]

I was just waiting for a resolution to the question about on permitting both blocks and return values, and two other minor threads:

• Should we warn if users try to use the kwargs argument to pass headers? (IMO: yes, probably, but might be best to do this in a follow-up GAX release coordinated with google-cloud-ruby so that g-c-r users won't see warnings if they upgrade their dependencies GAX and g-c-r at the same time.)

• Is it possible to simplify the test mocking? (We chatted offline a little about this; it wasn't clear there was an easy way to.)

  I'm not blocking on a particular decision on these, as long as there is one.

[jbolinger]

Let's leave the warning out for now since it's documented - we can add it later per your suggestion.

I haven't found a better way for the mocks. Happy to update if someone has a suggestion though.

That just leaves us with the return issue. I don't have a strong opinion either way. I'm sort of inclined to leave it as-is simply because I found it useful for dev purposes (makes it easy to dump the server response to the console while doing other things in the block). Granted, that is a bit bizarre and I'm 100% willing to change it to return nil if there is any strong opinion to do so.

[geigerj]

@jbolinger -- before you merge, could you check my comment on paginated calls here: https://github.com/googleapis/gax-ruby/pull/118/files#diff-30f305d8fc0cc5476762707ed876f556L229?

[jbolinger]

Will do. I wanted to test that against a live API but haven't gotten to it. Can you think of a good one (not logging)?

[geigerj]

@jbolinger you could try Pub/Sub. I think if you use the gapic-test project, there are already some test resources (i.e., subscriptions/topics/messages) set up for you to run against.