Opencensus Tracing: Start tracing bidi streaming callables. #636

igorbernstein2 · 2019-01-03T16:48:22Z

This depends on #633 and is extracted from #613.

This introduces TracedBidiCallables that will create a trace and complete it. It is inserted as one of the outermost links in the callable chain to allow all other callables to contribute their annotations.
google-cloud-java that extend the chains (like cloud bigtable) will also use this class to trace their extensions.

This depends on googleapis#633 and is extracted from googleapis#613. This introduces TracedBidiCallables that will create a trace and complete it. It is inserted as one of the outermost links in the callable chain to allow all other callables to contribute their annotations. google-cloud-java that extend the chains (like cloud bigtable) will also use this class to trace their extensions.

igorbernstein2 · 2019-01-05T07:16:24Z

Rebased & ready for review

codecov-io · 2019-01-05T07:18:37Z

Codecov Report

Merging #636 into master will increase coverage by 0.22%.
The diff coverage is 90%.

@@             Coverage Diff             @@
##             master    #636      +/-   ##
===========================================
+ Coverage     75.57%   75.8%   +0.22%     
- Complexity     1008    1011       +3     
===========================================
  Files           187     188       +1     
  Lines          4365    4410      +45     
  Branches        343     344       +1     
===========================================
+ Hits           3299    3343      +44     
  Misses          911     911              
- Partials        155     156       +1

Impacted Files	Coverage Δ	Complexity Δ
...a/com/google/api/gax/grpc/GrpcCallableFactory.java	`76.92% <100%> (+1.11%)`	`9 <0> (ø)`	⬇️
...google/api/gax/tracing/TracedResponseObserver.java	`100% <100%> (+16%)`	`7 <4> (+1)`	⬆️
...com/google/api/gax/tracing/TracedBidiCallable.java	`87.17% <87.17%> (ø)`	`2 <2> (?)`
...api/gax/grpc/InstantiatingGrpcChannelProvider.java	`75% <0%> (ø)`	`27% <0%> (ø)`	⬇️

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 b93f052...e2ad761. Read the comment docs.

# Conflicts: # gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallableFactory.java

vam-google

LGTM with a bunch of comments. None of the comments makes me think that this PR can't be merged as is, so LGTM, but please check the comments and address them before submitting.

vam-google · 2019-01-28T18:48:09Z

gax/src/main/java/com/google/api/gax/tracing/TracedBidiCallable.java

@@ -0,0 +1,220 @@
+/*
+ * Copyright 2018 Google LLC


vam-google · 2019-01-28T18:49:30Z

gax/src/main/java/com/google/api/gax/tracing/TracedBidiCallable.java

+/**
+ * A wrapper callable that will wrap a callable chain in a trace.
+ *
+ * <p>This class is meant to be an internal implementation google-cloud-java clients only.


Please remove the mention of google-cloud-java from the doc. Gax should be generator-specific and should not really know about google-cloud-java. In @InternalApi it is kind of ok, since the whole gist of the annotation is to talk about "internal stuff", but it is not ok in the actual documentation of the class.

vam-google · 2019-01-28T18:54:39Z

gax/src/main/java/com/google/api/gax/tracing/TracedBidiCallable.java

+            @Override
+            public void cancel() {
+              cancellationCauseHolder.compareAndSet(
+                  null, new CancellationException("Cancelled without cause"));


Isn't CancellationException("Cancelled without cause") == CancellationException() in essence? If yes, please remove the description.

vam-google · 2019-01-28T19:07:02Z

gax/src/main/java/com/google/api/gax/tracing/TracedBidiCallable.java

+
+    @Override
+    public void onResponse(ResponseT response) {
+      tracer.responseReceived();


Does the order of calls make any difference here (or can make any at least potentially in a corner case)? It seems logical to report response received (i.e. call tracer) as a last statement, so the reporting is done when everything is really done.

It does, we don't want to include the caller's callback in the trace

vam-google · 2019-01-28T19:08:54Z

gax/src/main/java/com/google/api/gax/tracing/TracedBidiCallable.java

+    public void onError(Throwable t) {
+      Throwable cancellationCause = cancellationCauseHolder.get();
+      if (cancellationCause != null) {
+        tracer.operationCancelled();


What would be the value of Throwable t here? Can it really be dropped? Won't it be same as cancellationCause? If no, why?

This is outdated, I switched to using the TracedResponseObserver from the server streaming PR

vam-google · 2019-01-28T19:11:26Z

gax/src/main/java/com/google/api/gax/tracing/TracedBidiCallable.java

+
+    @Override
+    public void send(RequestT request) {
+      tracer.requestSent();


This looks suspicious: requestSent() (past tense) is called before send() (the actual "sending" operation seems like).

I think it makes sense if you consider the message leaving the callers control as sent

vam-google · 2019-01-28T19:12:48Z

gax/src/main/java/com/google/api/gax/tracing/TracedBidiCallable.java

+
+    @Override
+    public void closeSend() {
+      innerStream.closeSend();


Should this (and the method above) call any tracer.<something>?

I don't think that closing the input stream important enough to trace and the closeSendWithError is the equivalent of calling cancel() on the future from unary callable: we trace both on the completion

vam-google · 2019-01-28T19:15:54Z

gax/src/test/java/com/google/api/gax/tracing/TracedBidiCallableTest.java

+  }
+
+  private static class FakeClientStream implements ClientStream<String> {
+    private List<String> sent = Lists.newArrayList();


Please do not use the guava's <CollectionType>.new<CollectionType>() methods (here and in the other places). This is what is said for this method in guava's documentation:

Note for Java 7 and later: this method is now unnecessary and should be treated as deprecated. Instead, use the `ArrayList#ArrayList()` constructor directly, taking advantage of the new "diamond" syntax

vam-google · 2019-01-28T19:20:02Z

gax/src/test/java/com/google/api/gax/tracing/TracedBidiCallableTest.java

+  }
+
+  private static class FakeBidiCallable extends BidiStreamingCallable<String, String> {
+    RuntimeException syncError;


Why aren't these fields declared private as well? They are part of a private class, please make them private (so we do not need to check the visibility of the class itself determine the actual visibility level of the fields).

Same applies to FakeBidiObserver and FakeStreamController. It could by stylistic thing, but FakeClientStream has its fields declared as private explicitly.

vam-google · 2019-01-28T19:24:47Z

gax/src/test/java/com/google/api/gax/tracing/TracedBidiCallableTest.java

+    }
+  }
+
+  private static class FakeStreamController implements StreamController {


Can some of these Fake* classes be replaced with mocks? This one seems like it can.

I removed this class, but left the others because the fakes were easier to deal with

vam-google · 2019-01-28T20:19:05Z

gax/src/main/java/com/google/api/gax/tracing/TracedBidiCallable.java

+        ApiTracer tracer,
+        ResponseObserver<ResponseT> innerObserver,
+        AtomicReference<Throwable> cancellationCauseHolder) {
+      this.tracer = tracer;


This seems identical to the Server Streaming PR, but it does not have null pointer check. It is either the null check is redundant there or missing here.

I switched to the ResponseObserver from the server streaming PR

# Conflicts: # gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallableFactory.java

igorbernstein2 · 2019-02-02T21:40:28Z

Thanks for reviewing!

igorbernstein2 requested a review from vam-google January 3, 2019 16:48

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 3, 2019

igorbernstein2 added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 3, 2019

igorbernstein2 changed the title ~~Start tracing bidi streaming callables.~~ WIP: Start tracing bidi streaming callables. Jan 3, 2019

igorbernstein2 added 2 commits January 5, 2019 02:05

fixes

00d216a

igorbernstein2 force-pushed the oc-bidi branch from e7ad7d5 to 00d216a Compare January 5, 2019 07:15

igorbernstein2 changed the title ~~WIP: Start tracing bidi streaming callables.~~ Start tracing bidi streaming callables. Jan 5, 2019

igorbernstein2 removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Jan 5, 2019

igorbernstein2 changed the title ~~Start tracing bidi streaming callables.~~ WIP: Start tracing bidi streaming callables. Jan 10, 2019

igorbernstein2 added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 10, 2019

igorbernstein2 changed the title ~~WIP: Start tracing bidi streaming callables.~~ WIP: Opencensus Tracing: Start tracing bidi streaming callables. Jan 11, 2019

igorbernstein2 added 2 commits January 11, 2019 15:05

Merge remote-tracking branch 'origin/master' into oc-bidi

91e049f

# Conflicts: # gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallableFactory.java

cancellation & static imports

21832a6

igorbernstein2 changed the title ~~WIP: Opencensus Tracing: Start tracing bidi streaming callables.~~ Opencensus Tracing: Start tracing bidi streaming callables. Jan 11, 2019

igorbernstein2 removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 11, 2019

vam-google approved these changes Jan 28, 2019

View reviewed changes

vam-google reviewed Jan 28, 2019

View reviewed changes

igorbernstein2 added 3 commits January 31, 2019 17:38

Merge remote-tracking branch 'origin/master' into oc-bidi

03f8e20

# Conflicts: # gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallableFactory.java

address some feedback

9f2015f

address more feedback

e2ad761

igorbernstein2 merged commit 2897235 into googleapis:master Feb 2, 2019

igorbernstein2 deleted the oc-bidi branch February 2, 2019 21:51

Opencensus Tracing: Start tracing bidi streaming callables. #636

Opencensus Tracing: Start tracing bidi streaming callables. #636

Uh oh!

Conversation

igorbernstein2 commented Jan 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igorbernstein2 commented Jan 5, 2019

Uh oh!

codecov-io commented Jan 5, 2019 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vam-google left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 commented Feb 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

igorbernstein2 commented Jan 3, 2019 •

edited

Loading

codecov-io commented Jan 5, 2019 •

edited by codecov bot

Loading