impl(generator): add Start and Await overloads to connections #14348

scotthart · 2024-06-18T16:49:09Z

part of the work for #7658

This PR adds the connection methods and decorator overrides to create an overload set for each LRO rpc, adding a "Start" method and an "Await" method. A new tag type NoAwaitTag is used to signal a call to the "Start" overload and passing an Operation signals a call to the "Await" overload.

A few Bigtable tests needed their EXPECT_CALL invocations adjusted to remove ambiguities the additional overloads introduced.

The PR is split into multiple commits to separate the generator changes, tests for the golden generated code, and all the generated code updates.

This change is

codecov · 2024-06-18T18:04:28Z

Codecov Report

Attention: Patch coverage is 64.75771% with 240 lines in your changes missing coverage. Please review.

Project coverage is 93.09%. Comparing base (514e2bd) to head (269f57b).
Report is 15 commits behind head on main.

Files	Patch %	Lines
.../internal/golden_thing_admin_tracing_connection.cc	20.00%	56 Missing ⚠️
.../v1/internal/golden_thing_admin_connection_impl.cc	64.00%	45 Missing ⚠️
...nerator/internal/connection_impl_rest_generator.cc	0.00%	32 Missing ⚠️
...n_tests/golden/v1/golden_thing_admin_connection.cc	0.00%	25 Missing ⚠️
...s/golden/v1/internal/request_id_connection_impl.cc	0.00%	25 Missing ⚠️
...nternal/golden_thing_admin_rest_connection_impl.cc	80.00%	20 Missing ⚠️
...olden/v1/internal/request_id_tracing_connection.cc	0.00%	14 Missing ⚠️
...lden/v1/mocks/mock_golden_thing_admin_connection.h	20.00%	8 Missing ⚠️
generator/internal/connection_impl_generator.cc	0.00%	7 Missing ⚠️
...tegration_tests/golden/v1/request_id_connection.cc	0.00%	5 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14348      +/-   ##
==========================================
- Coverage   93.74%   93.09%   -0.66%     
==========================================
  Files        2270     2183      -87     
  Lines      202965   192857   -10108     
==========================================
- Hits       190277   179543   -10734     
- Misses      12688    13314     +626

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dbolduc

I didn't get very far in the review. My browser keeps crashing when I open the "generated code" commit. Can you put the golden changes in its own commit?

dbolduc · 2024-06-18T20:28:59Z

.typos.toml

+  "NAM4",
+  # Metadata is mispelled in this proto message name found in
+  # google/cloud/aiplatform/v1/vizier_service.proto
+  "CheckTrialEarlyStoppingStateMetatdata"


dbolduc · 2024-06-18T20:30:02Z

generator/internal/connection_generator.cc

+      "  virtual StatusOr<$longrunning_operation_type$>\n"},
+                 // clang-format on
+                 // TODO(#14344): Remove experimental tag.
+                 {"  $method_name$(google::cloud::ExperimentalTag, "


nit: I would omit google::cloud:: from all of these things.

dbolduc · 2024-06-18T20:32:46Z

google/cloud/google_cloud_cpp_common.bzl

@@ -18,6 +18,7 @@

 google_cloud_cpp_common_hdrs = [
    "access_token.h",
+    "await_tag.h",


nit: rename to no_await_tag.h ?

dbolduc · 2024-06-24T08:00:10Z

generator/integration_tests/golden/v1/golden_kitchen_sink_connection.h

@@ -21,7 +21,9 @@

 #include "generator/integration_tests/golden/v1/golden_kitchen_sink_connection_idempotency_policy.h"
 #include "generator/integration_tests/golden/v1/internal/golden_kitchen_sink_retry_traits.h"
+#include "google/cloud/await_tag.h"


scotthart

Done.

Reviewable status: 0 of 2029 files reviewed, 4 unresolved discussions (waiting on @dbolduc)

dbolduc

Only nits, and suggestions so approved. The two important things are:

We should consider naming the wait method: Await$method_name$
We should change the OTelScope thingy in the synchronous tracing method.

dbolduc · 2024-06-25T04:37:09Z

generator/integration_tests/golden/v1/golden_thing_admin_connection.cc

@@ -52,6 +52,22 @@ GoldenThingAdminConnection::CreateDatabase(
    Status(StatusCode::kUnimplemented, "not implemented"));
 }

+  StatusOr<google::longrunning::Operation>


pre-checkers nit: remove space at the start of the line?

dbolduc · 2024-06-25T04:37:27Z

generator/integration_tests/golden/v1/golden_thing_admin_connection.cc

@@ -52,6 +52,22 @@ GoldenThingAdminConnection::CreateDatabase(
    Status(StatusCode::kUnimplemented, "not implemented"));
 }

+  StatusOr<google::longrunning::Operation>
+GoldenThingAdminConnection::CreateDatabase(
+    google::cloud::ExperimentalTag, google::cloud::NoAwaitTag,


nit: I'd drop the google::cloud::

dbolduc · 2024-06-25T04:39:17Z

generator/integration_tests/golden/v1/internal/golden_thing_admin_connection_impl.cc

@@ -126,6 +126,53 @@ GoldenThingAdminConnectionImpl::CreateDatabase(google::test::admin::database::v1
    polling_policy(*current), __func__);
 }

+


nit: extra space

dbolduc · 2024-06-25T04:51:46Z

generator/integration_tests/golden/v1/internal/golden_thing_admin_tracing_connection.cc

+    google::cloud::NoAwaitTag, google::test::admin::database::v1::CreateDatabaseRequest const& request) {
+  auto span = internal::MakeSpan(
+      "golden_v1::GoldenThingAdminConnection::CreateDatabase");
+  internal::OTelScope scope(span);


For synchronous calls, it is simpler to use:

opentelemetry::trace::Scope scope(span);

dbolduc · 2024-06-25T04:52:14Z

generator/integration_tests/golden/v1/internal/golden_thing_admin_tracing_connection.cc

+    google::longrunning::Operation const& operation) {
+  auto span = internal::MakeSpan(
+      "golden_v1::GoldenThingAdminConnection::CreateDatabase");
+  internal::OTelScope scope(span);


comment: FTR, this OTelScope is necessary.

dbolduc · 2024-06-25T04:55:36Z

generator/integration_tests/golden/v1/internal/golden_thing_admin_tracing_connection.cc

+GoldenThingAdminTracingConnection::UpdateDatabaseDdl(google::cloud::ExperimentalTag,
+    google::cloud::NoAwaitTag, google::test::admin::database::v1::UpdateDatabaseDdlRequest const& request) {
+  auto span = internal::MakeSpan(
+      "golden_v1::GoldenThingAdminConnection::UpdateDatabaseDdl");


Feel free to defer this.

I am noticing that the spans for the three overloads are identical in names and attributes. How will we tell them apart in the trace UI?

I think we would at least want an attribute equivalent of "NoAwaitTag"

dbolduc · 2024-06-25T05:00:16Z

generator/integration_tests/tests/golden_thing_admin_tracing_connection_test.cc

+  auto mock = std::make_shared<MockGoldenThingAdminConnection>();
+  EXPECT_CALL(*mock, CreateDatabase(_, _, _)).WillOnce([] {
+    EXPECT_TRUE(ThereIsAnActiveSpan());
+    EXPECT_TRUE(OTelContextCaptured());


this expectation is only necessary for async operations

dbolduc · 2024-06-25T05:04:47Z

generator/integration_tests/tests/golden_thing_admin_connection_test.cc

+        EXPECT_EQ(expected_operation.name(), r.name());
+        google::longrunning::Operation op;
+        op.set_name(r.name());
+        op.set_done(true);


comment for myself: I was wondering if we need/want tests for the polling loop side of things? Like, when the operation is not immediately done.

I think no. I think those cases are covered in async_long_running_operation_test.cc. We do not need to recreate the tests for AsyncAwaitLongRunningOperation in the connections.

dbolduc · 2024-06-25T05:29:08Z

generator/internal/tracing_connection_generator.cc

+    google::cloud::NoAwaitTag, $request_type$ const& request) {
+  auto span = internal::MakeSpan(
+      "$product_namespace$::$connection_class_name$::$method_name$");
+  internal::OTelScope scope(span);


s/internal::OtelScope/opentelemetry::trace::Scope

What you have works, it just does some unnecessary extra work (duplicating OTel's thread_local state) that we do not need to do in the synchronous case.

dbolduc · 2024-06-25T05:36:48Z

generator/internal/connection_generator.cc

+      "  virtual future<Status>\n",
+      "  virtual future<StatusOr<$longrunning_deduced_response_type$>>\n"},
+                 // TODO(#14344): Remove experimental tag.
+     {"  $method_name$(google::cloud::ExperimentalTag,"


I think I prefer to call these things: AwaitCreateFoo(...) or WaitForCreateFoo(...).

It stops being an overload set. I don't think that's bad.

auto operation = CreateFoo(request); auto foo = CreateFoo(..., *operation);

The second line isn't creating a foo. That already happened. It is waiting on a foo. That is why I like the different name.

It seems super unlikely, but it could open us up to RPC name clashes. If that is the concern, feel free to leave it as is.

scotthart

My concern is that method names are defined by the service, not us. While small, we risk future naming collisions.
Fixed.

Reviewable status: 0 of 2029 files reviewed, 13 unresolved discussions (waiting on @dbolduc)

.typos.toml line 67 at r2 (raw file):