Propagate :use_error_serialization_v2 option to GRPC client

[davehughes]

Attempt to address #295.

Alter Temporal::Connection::GRPC to respect a :use_error_serialization_v2 option, falling back to the global singleton value if not found, and update Temporal::Configuration#for_connection to pass its setting through when creating connections.

It looks like options can already be plumbed through by editing a Temporal::Configuration instance's connection_options, but this case seems slightly different because use_error_serialization_v2 is an explicit setting on the configuration.

[DeRauk]

@davehughes Thank you for the PR! Could you please rebase onto master to get the test fix for CI?

[njaremko]

Bump?

[davehughes]

@DeRauk thanks!

[jeffschoner]

@davehughes @DeRauk I'm not sure this is working as expected. I believe the value set in the configuration instance will always be used in grpc.rb. The :use_error_serialization_v2 option cannot be unset such that it will fall back to the global. Setting it to false or nil does not seem to change this behavior.

E.g.,

irb(main):001:0> options = {}
=> {}
irb(main):002:0> options.fetch(:use_error_serialization_v2, true)
=> true
irb(main):003:0> options = {}.merge(use_error_serialization_v2: nil)
=> {:use_error_serialization_v2=>nil}
irb(main):004:0> options.fetch(:use_error_serialization_v2, true)
=> nil
irb(main):005:0> options = {}.merge(use_error_serialization_v2: false)
=> {:use_error_serialization_v2=>false}
irb(main):006:0> options.fetch(:use_error_serialization_v2, true)
=> false

Reading this from the options that are passed in from the configuration is still the right way to go, rather than relying on global configuration being read via Temporal.configuration.

This change, however, does make a breaking change for anyone who has opted into error serialization v2 via global config.

What do you think about changing this from

options.fetch(:use_error_serialization_v2, Temporal.configuration.error_serialization_v2)

to

options[:use_error_serialization] || Temporal.configuration.error_serialization_v2

?

[davehughes]

Hmm...I see what you mean and how this is an accidentally breaking change for a set of scenarios.

My mental model of configuration usage here is that folks should either choose to configure via the global singleton or through local configuration objects depending on their needs. I'm not sure how strongly the project wants to hold to the semantics that the global config is a fallback vs. a distinct thing, though clearly there's some of the former going on.

I don't think the proposed change works as written, since if options[:use_error_serialization] == false, the code will always fall back to the global setting. But allowing nil values and falling back in the presence of a nil should work:

use_v2 = options[:use_error_serialization_v2]
use_v2 = Temporal.configuration.error_serialization_v2 if use_v2.nil?

In conjunction with using a nil default in the configuration constructor here should allow the global fallback to apply as you expect.

All that said, I'm really just a nomad passing through this project. I've left the job where I noticed the issue that led to this PR, and fixing this is not really a priority for me. (Although I do apologize for the inadvertent breakage, and would be happy to shepherd through a PR with the approach above if asked.)