refactor(bigtable): future-proof ReadRows mocking

[dbolduc]

EDIT: The mystery feature was #12010

---

Part of the work for <mystery bigtable feature>

Googlers can see: go/cloud-cxx:how-to-grow-bigtable-read-rows

The new Connection API allows us to add additional ReadRowsRequest configuration parameters.

I think we should include app_profile_id in the params struct. It is easier to check in the params struct than in google::cloud::mocks::CurrentOptions().

Note that we cannot internal::ExtractOption<AppProfileIdOption> in the connection layer, because the current options are const&. I think it is safer to just use .get<> instead of changing the method to Options& internal::CurrentOptions().

---

This change is 

[codecov]

Codecov Report

Patch coverage: 97.70% and no project coverage change.

Comparison is base (eaefb73) 93.78% compared to head (85ecbbd) 93.78%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #11725   +/-   ##
=======================================
  Coverage   93.78%   93.78%           
=======================================
  Files        1829     1829           
  Lines      164878   164947   +69     
=======================================
+ Hits       154627   154695   +68     
- Misses      10251    10252    +1     

Impacted Files	Coverage Δ
google/cloud/bigtable/data_connection.h	100.00% <ø> (ø)
...gle/cloud/bigtable/internal/data_connection_impl.h	100.00% <ø> (ø)
google/cloud/bigtable/data_connection.cc	41.81% <83.33%> (+5.81%)	⬆️
...oud/bigtable/internal/data_connection_impl_test.cc	96.57% <94.73%> (-0.04%)	⬇️
...gle/cloud/bigtable/examples/howto_mock_data_api.cc	100.00% <100.00%> (ø)
...le/cloud/bigtable/internal/data_connection_impl.cc	98.81% <100.00%> (+0.01%)	⬆️
...cloud/bigtable/internal/data_tracing_connection.cc	100.00% <100.00%> (ø)
.../bigtable/internal/data_tracing_connection_test.cc	100.00% <100.00%> (ø)
google/cloud/bigtable/mocks/mock_data_connection.h	100.00% <100.00%> (ø)
google/cloud/bigtable/table_test.cc	100.00% <100.00%> (ø)

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[devbww]

Note that we cannot internal::ExtractOption<AppProfileIdOption> in the connection layer, because the current options are const&.

Can you stop the AppProfileIdOption from getting into an OptionsSpan in the first place?

In some stuff I'm working on with a new "params-destined option", the client calls now look like ...

  opts = internal::MergeOptions(std::move(opts), options_);
  auto foo_option = internal::ExtractOption<FooOption>(opts);
  internal::OptionsSpan span(std::move(opts));
  // use foo_option somehow

Or, might an existing ReadRows() mock depend on seeing the AppProfileIdOption in CurrentOptions()? (In my case, FooOption is brand new.)

[dbolduc]

Can you stop the AppProfileIdOption from getting into an OptionsSpan in the first place?

No, I don't think so. I have to keep calling this connection method from the client (which doesn't accept an app profile id):

google-cloud-cpp/google/cloud/bigtable/table.cc

Lines 222 to 223 in 1260047

	return connection_->ReadRows(table_name_, std::move(row_set), rows_limit,
	std::move(filter));

I could reverse the flow of the Connection methods. But then there would be no point to extracting the option. I would have to add it back in.

Table::ReadRows(..., opts) {
  opts = internal::MergeOptions(std::move(opts), options_);
  auto app_profile_id = internal::ExtractOption<AppProfileIdOption>(opts);
  OptionsSpan o(std::move(opts));
  auto params = ReadRowsParams{...};
  connection_->ReadRowsFull(params);
}

DataConnection::ReadRowsFull(ReadRowsParams params) {
  auto options = internal::CurrentOptions();
  options.set<AppProfileIdOption>(params.app_profile_id);
  OptionsSpan o(std::move(options));
  return ReadRows(params.foo, params.bar, params.baz, params.etc);
}

Might an existing ReadRows() mock depend on seeing the AppProfileIdOption in CurrentOptions()?

Yeah, that can also be a problem.

In some stuff I'm working on with a new "params-destined option", the client calls now look like ...

Yeah, that looks right to me. I just can't use it in bigtable for backwards compatibility reasons. :(