Opencensus Tracing: Start tracing server streaming callables. #635

igorbernstein2 · 2019-01-02T23:50:50Z

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

This introduces TracedServerStreamingCallables 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 TracedServerStreamingCallables 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.

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

+    String clientName = fullServiceName.substring(serviceIndex + 1);
+
+    return SpanName.of(clientName, methodName);
+  }


igorbernstein2 · 2019-01-05T07:05:36Z

Rebased, this is ready for review

codecov-io · 2019-01-05T07:08:19Z

Codecov Report

Merging #635 into master will increase coverage by 0.72%.
The diff coverage is 90.47%.

@@             Coverage Diff              @@
##             master     #635      +/-   ##
============================================
+ Coverage     74.64%   75.37%   +0.72%     
- Complexity      961      979      +18     
============================================
  Files           183      185       +2     
  Lines          4216     4239      +23     
  Branches        334      335       +1     
============================================
+ Hits           3147     3195      +48     
+ Misses          916      891      -25     
  Partials        153      153

Impacted Files	Coverage Δ	Complexity Δ
...a/com/google/api/gax/grpc/GrpcCallableFactory.java	`75.8% <100%> (+1.23%)`	`9 <0> (ø)`	⬇️
...api/gax/tracing/TracedServerStreamingCallable.java	`100% <100%> (ø)`	`2 <2> (?)`
...google/api/gax/tracing/TracedResponseObserver.java	`84% <84%> (ø)`	`6 <6> (?)`
...m/google/api/gax/httpjson/HttpRequestRunnable.java	`54.16% <0%> (-13.49%)`	`5% <0%> (ø)`
...le/api/gax/rpc/ServerStreamingAttemptCallable.java	`87.15% <0%> (+0.36%)`	`17% <0%> (ø)`	⬇️
...m/google/api/gax/retrying/BasicRetryingFuture.java	`93.61% <0%> (+0.75%)`	`23% <0%> (+1%)`	⬆️
...n/java/com/google/api/gax/rpc/AttemptCallable.java	`82.6% <0%> (+2.6%)`	`4% <0%> (ø)`	⬇️
...om/google/api/gax/rpc/CheckingAttemptCallable.java	`81.81% <0%> (+2.87%)`	`4% <0%> (ø)`	⬇️
... and 3 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 33a57b6...394d839. Read the comment docs.

igorbernstein2 · 2019-01-11T16:28:30Z

I added cancellation handling similar to #634

vam-google

My only real concern now (it applies to all of the PRs) is the code duplication in the internal static classes inside the Traced*Callable classes.

vam-google · 2019-01-28T20:20:07Z

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

+          new StreamController() {
+            @Override
+            public void cancel() {
+              wasCancelled = true;


This looks like the only difference between this internal static class and the bidistream version https://github.com/googleapis/gax-java/pull/636/files#diff-08e9aa9f0dcb325e4bd2a1770d907a77R98. In general it seems like a lot of duplication (sometimes completely identical code, like this example) between the internal static classes of the Traced*Callable.

Please strongly consider. Moving the internal static classes to a package private classes under tracing package and reuse them in all of the Traced* callables.

vam-google · 2019-01-28T20:21:08Z

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

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


Same ordering-specific question as in another PR (BTW, Ideally we would want this code to be shared, please see the above coment)

From bidi PR
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.

The span should only wrap the work that the client/callable is doing, not the user's callbacks. Ie. if someone does this:

client.readRowsCallable().call(request, new ResponseObserver() { void onResponse(Row row) { Thread.sleep(1000) } }

We don't want to include the 1 second sleep. That should be part of the caller's span not the clients

vam-google · 2019-01-28T20:24:14Z

gax/src/main/java/com/google/api/gax/tracing/TracedServerStreamingCallable.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 google-cloud-java from the documentation.

From the bidi PR:
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.

Is the implication that google-cloud-java clients are allowed to use any @InternalOnly api?

It is ok to leave mention of google-cloud-java in @InternalApi annotation description (which, probably, can be considered as "permissions" to use in under google-cloud-java). Here I'm asking to remove it from the documentation only (google-cloud-java is mentioned twice in this class I'm asking to remove one of the mentions).

igorbernstein2 · 2019-01-28T21:21:33Z

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

+    @Override
+    public void onError(Throwable t) {
+      if (wasCancelled) {
+        tracer.operationCancelled();


From bidi PR
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 to workaround the the retry implementation using cancellations to end polling. I don't want these to be traced as cancellations. I added a comment explaining the motivation

igorbernstein2 · 2019-01-28T21:22:15Z

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

+
+    @Override
+    public void onComplete() {
+      tracer.operationSucceeded();


From bidi PR

Same ordering question as for the onResponse() method.

Lets keep discussion, in https://github.com/googleapis/gax-java/pull/635/files#r251609835

igorbernstein2 · 2019-01-28T22:46:54Z

All feedback should be addressed. PTAL

vam-google

LGTM, but please check the comment about mentioning google-cloud-java in the doc.

igorbernstein2 · 2019-01-31T20:14:37Z

Thanks for reviewing!

igorbernstein2 requested a review from vam-google January 2, 2019 23:50

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 2, 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 server streaming callables.~~ WIP: Start tracing server streaming callables. Jan 3, 2019

igorbernstein2 commented Jan 5, 2019

View reviewed changes

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

String clientName = fullServiceName.substring(serviceIndex + 1);

return SpanName.of(clientName, methodName);

}

This comment was marked as spam.

Sign in to view

igorbernstein2 force-pushed the oc-server-streaming branch from 28f135d to a60fb70 Compare January 5, 2019 07:05

igorbernstein2 changed the title ~~WIP: Start tracing server streaming callables.~~ Start tracing server 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 server streaming callables.~~ WIP: Start tracing server 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

Expose cancellation & use static imports

bd57b3a

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

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

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

Merge remote-tracking branch 'origin/master' into oc-server-streaming

80e441a

vam-google reviewed Jan 28, 2019

View reviewed changes

igorbernstein2 commented Jan 28, 2019

View reviewed changes

address feedback

a2eb0e3

vam-google approved these changes Jan 31, 2019

View reviewed changes

address feedback

394d839

igorbernstein2 merged commit b93f052 into googleapis:master Jan 31, 2019

igorbernstein2 deleted the oc-server-streaming branch January 31, 2019 22:36

andreamlin mentioned this pull request Feb 1, 2019

Release gax-java v1.38.0 #655

Merged

Opencensus Tracing: Start tracing server streaming callables. #635

Opencensus Tracing: Start tracing server streaming callables. #635

Uh oh!

Conversation

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

Uh oh!

Uh oh!

This comment was marked as spam.

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!

igorbernstein2 commented Jan 11, 2019

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!

vam-google Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Jan 28, 2019

Uh oh!

vam-google left a comment

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 commented Jan 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

igorbernstein2 commented Jan 2, 2019 •

edited

Loading

codecov-io commented Jan 5, 2019 •

edited by codecov bot

Loading

vam-google Jan 31, 2019 •

edited

Loading