feat(common): add support for call-tree-specific options #7669

devbww · 2021-11-28T05:42:58Z

Provide access to the prevailing Options for an operation without having
to plumb function parameters through the various and sundry layers, like
connection.

This also means that helper libraries can determine things like whether a
tracing component is enabled, without having to distort their APIs.

For example, if all ServiceClient::Operation() calls are implemented like

class ServiceClient {
 public
  ServiceClient(std::shared_ptr<ServiceConnection> connection,
                Options options = {})
      : connection_(std::move(connection)),
        options_(ServiceDefaultOptions(std::move(options))) {}

  T Operation(..., Options options = {}) {
    internal::OptionsSpan span(internal::MergeOptions(options, options_));
    ...
  }

 private:
  std::shared_ptr<ServiceConnection> connection_;
  Options options_;
};

then the connection layer, or any other internal library, can retrieve the
prevailing options using internal::CurrentOptions(). If the operation is
asynchronous, the invocation-time Options are also installed during its
callbacks.

This change is

Provide access to the prevailing `Options` for an operation without having to plumb function parameters through the various and sundry layers, like connection. This also means that helper libraries can determine things like whether a tracing component is enabled, without having to distort their APIs. For example, if all `ServiceClient::Operation()` calls are implemented like ``` class ServiceClient { public ServiceClient(std::shared_ptr<ServiceConnection> connection, Options options = {}) : connection_(std::move(connection)), options_(ServiceDefaultOptions(std::move(options))) {} T Operation(..., Options options = {}) { internal::OptionsSpan span(internal::MergeOptions(options, options_)); ... } private: std::shared_ptr<ServiceConnection> connection_; Options options_; }; ``` then the connection layer, or any other internal library, can retrieve the prevailing options using `internal::CurrentOptions()`. If the operation is asynchronous, the invocation-time `Options` are also installed during its callbacks.

google-cloud-cpp-bot · 2021-11-28T05:45:15Z

Google Cloud Build Logs
For commit: f7a5ca793c7624c2a8f6d9960b03e02397c94501

Public Access Build Logs
Cloud Build Console (requires auth)
Kokoro Logs (requires auth)

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

codecov · 2021-11-28T06:12:54Z

Codecov Report

Merging #7669 (7284576) into main (8d5c432) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #7669      +/-   ##
==========================================
- Coverage   95.28%   95.28%   -0.01%     
==========================================
  Files        1254     1254              
  Lines      113312   113401      +89     
==========================================
+ Hits       107969   108052      +83     
- Misses       5343     5349       +6

Impacted Files	Coverage Δ
google/cloud/options.h	`97.43% <ø> (ø)`
google/cloud/internal/async_connection_ready.cc	`88.23% <100.00%> (+0.73%)`	⬆️
google/cloud/internal/async_read_stream_impl.h	`98.71% <100.00%> (+0.06%)`	⬆️
...ogle/cloud/internal/async_read_write_stream_impl.h	`93.05% <100.00%> (+0.51%)`	⬆️
google/cloud/internal/async_retry_loop_test.cc	`99.65% <100.00%> (+0.06%)`	⬆️
google/cloud/internal/async_rpc_details.h	`94.44% <100.00%> (+0.32%)`	⬆️
...le/cloud/internal/default_completion_queue_impl.cc	`97.15% <100.00%> (+0.15%)`	⬆️
google/cloud/options.cc	`100.00% <100.00%> (ø)`
google/cloud/options_test.cc	`100.00% <100.00%> (ø)`
...e/cloud/testing_util/fake_completion_queue_impl.cc	`94.04% <100.00%> (+0.22%)`	⬆️
... and 9 more

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 8d5c432...7284576. Read the comment docs.

coryan

A suggestion for a different implementation which might be (maybe) a more targeted change and easier to maintain as we add asynchronous operations.

coryan · 2021-11-30T19:05:49Z

google/cloud/internal/async_read_stream_impl.h

@@ -150,10 +151,12 @@ class AsyncReadStreamImpl
     private:
      void Cancel() override {}  // LCOV_EXCL_LINE
      bool Notify(bool ok) override {
+        OptionsSpan span(options_);


I wonder if we should set these spans in the completion queue instead. The Start() function is a good place to capture the current options:

google-cloud-cpp/google/cloud/internal/default_completion_queue_impl.cc

Lines 238 to 241 in b92933b

void DefaultCompletionQueueImpl::StartOperation(

std::unique_lock<std::mutex> lk, std::shared_ptr<AsyncGrpcOperation> op,

absl::FunctionRef<void(void*)> start) {

void* tag = op.get();

and the notifications would be a good place to create the span:

google-cloud-cpp/google/cloud/internal/default_completion_queue_impl.cc

Lines 175 to 179 in b92933b

auto op = FindOperation(tag);

++notify_counter_;

if (op->Notify(ok)) {

ForgetOperation(tag);

}

I considered that, but thought that changing the tag map (which is really just a collection of AsyncGrpcOperations) to a map of AsyncGrpcOperation/Options pairs was both a little more complicated, and belied the fact that the Options logically belonged to the operation.

It is certainly the case that the behavior is now distributed to the AsyncGrpcOperation sub-classes, but I've been thinking of that as a feature. For example, the existence of the internal::CompletionQueueImpl interface implies that this behavior should not belong in a particular instantiation (the DefaultCompletionQueueImpl) of that interface.

Perhaps experience with this will lead to a better understanding, but for now I'd vote for leaving it as is.

For example, the existence of the internal::CompletionQueueImpl interface implies that this behavior should not belong in a particular instantiation (the DefaultCompletionQueueImpl) of that interface.

FWIW, the only reason for the interface is mocking 🤷

Perhaps experience with this will lead to a better understanding, but for now I'd vote for leaving it as is.

SGTM.

coryan · 2021-11-30T19:15:36Z

google/cloud/options.cc

+// The prevailing options for the current operation.  Thread local, so
+// additional propagation must be done whenever work for the operation
+// is done in another thread.
+thread_local Options current_options;


This obviously has a non-trivial destructor. I am certain you know better than I do whether this is accepted by the Google Style Guide. Just asking to maybe learn something.

The non-trivial destructor for the namespace-scoped object is not a problem because the object is only accessible to its thread, and is not destroyed until the thread terminates. So, there is no issue with destruction order, and indeed the Style Guide says as much.

What the Style Guide still does require for namespace-scoped objects, thread local or not, is a compile-time-constant initializer. In the thread-local case I guess this is to guard against threads being created before global initialization is complete. The workaround is the same in both cases ... to convert the namespace-scoped object to a function local, so that it is initialized on the first call, no matter when that occurs. I've done that. PTAL.

Ack, thanks for looking into it.

devjgm

Looks like a nice solution that's hidden from our users but will make our implementation plumbing much easier. Thanks! Just a couple small questions. The PR LGTM, but I'll let Carlos do the real review.

devjgm · 2021-11-30T20:22:07Z

google/cloud/options.h

+  ~OptionsSpan();
+
+ private:
+  Options opts_;


nit: If I understand this correctly, opts_ is actually the previous options that will be restored after the span goes out of scope. That is, opts_ is not the same as opts, which was passed to the c'tor. If you think this distinction is worth capturing, consider renaming this data member to something like restore_ or old_opts_ or something.

I had considered that, but concluded that the meaning of the member changes with time. For the majority of its life it is indeed the "options to restore", but after the initializer list is complete it is the "options to install" (i.e., it is the opts passed to the constructor), and after restoration it is a "moved from options". All in all, it seemed better to give it a generic name, and let the code specify its meaning at any point.

devjgm · 2021-11-30T20:22:49Z

google/cloud/options.h

+ *
+ * @param opts the `Options` to install.
+ */
+class ABSL_MUST_USE_RESULT OptionsSpan {


Q: Is ABSL_MUST_USE_RESULT needed here, or nice to have, or... what's this here for?

It is there to try to call out usages like OptionsSpan(opts); where you forget the variable name, and the temporary span dies at the semicolon. For example, see https://github.com/abseil/abseil-cpp/blob/master/absl/base/attributes.h#L410-L414. (Previously we might have used a macro trick to do something similar.)

That said, when I tried to test that, it didn't seem to trigger a warning. But I'm not sure that wasn't just an artifact of which compiler+version I used, so I decided to leave it anyway. But, happy to remove also.

google-cloud-cpp-bot · 2021-12-01T08:19:03Z

Google Cloud Build Logs
For commit: 7284576d0c7c6223f8d7495a561ae5efae59e896

Public Access Build Logs
Cloud Build Console (requires auth)
Kokoro Logs (requires auth)

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 28, 2021

devbww marked this pull request as ready for review November 28, 2021 06:58

devbww requested a review from a team as a code owner November 28, 2021 06:58

devbww mentioned this pull request Nov 30, 2021

Consider passing Options to all Client methods #7198

Closed

coryan reviewed Nov 30, 2021

View reviewed changes

devjgm reviewed Nov 30, 2021

View reviewed changes

Address review comments.

7284576

coryan approved these changes Dec 1, 2021

View reviewed changes

devbww merged commit 823c121 into googleapis:main Dec 1, 2021

devbww deleted the options-span branch December 1, 2021 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(common): add support for call-tree-specific options #7669

feat(common): add support for call-tree-specific options #7669

devbww commented Nov 28, 2021 •

edited by coryan

Loading

google-cloud-cpp-bot commented Nov 28, 2021

codecov bot commented Nov 28, 2021 •

edited

Loading

coryan left a comment

coryan Nov 30, 2021

devbww Dec 1, 2021

coryan Dec 1, 2021

coryan Nov 30, 2021

devbww Dec 1, 2021

coryan Dec 1, 2021

devjgm left a comment

devjgm Nov 30, 2021

devbww Dec 1, 2021

devjgm Nov 30, 2021

devbww Dec 1, 2021

google-cloud-cpp-bot commented Dec 1, 2021

	void DefaultCompletionQueueImpl::StartOperation(
	std::unique_lock<std::mutex> lk, std::shared_ptr<AsyncGrpcOperation> op,
	absl::FunctionRef<void(void*)> start) {
	void* tag = op.get();

	auto op = FindOperation(tag);
	++notify_counter_;
	if (op->Notify(ok)) {
	ForgetOperation(tag);
	}

feat(common): add support for call-tree-specific options #7669

feat(common): add support for call-tree-specific options #7669

Conversation

devbww commented Nov 28, 2021 • edited by coryan Loading

google-cloud-cpp-bot commented Nov 28, 2021

codecov bot commented Nov 28, 2021 • edited Loading

Codecov Report

coryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devjgm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-cloud-cpp-bot commented Dec 1, 2021

devbww commented Nov 28, 2021 •

edited by coryan

Loading

codecov bot commented Nov 28, 2021 •

edited

Loading