Opencensus Tracing: Start tracing client streaming callables. #637

igorbernstein2 · 2019-01-03T18:45:18Z

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

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

TODO:
[ ] trace operation cancellation after it's reviewed in #634

This depends on googleapis#633 and is extracted from googleapis#613. This introduces TracedClientStreamCallables 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:21:59Z

rebased & ready for review

codecov-io · 2019-01-05T07:25:45Z

Codecov Report

Merging #637 into master will increase coverage by 0.17%.
The diff coverage is 92.3%.

@@             Coverage Diff              @@
##             master     #637      +/-   ##
============================================
+ Coverage     75.54%   75.71%   +0.17%     
- Complexity     1025     1029       +4     
============================================
  Files           192      194       +2     
  Lines          4543     4608      +65     
  Branches        352      356       +4     
============================================
+ Hits           3432     3489      +57     
- Misses          954      961       +7     
- Partials        157      158       +1

Impacted Files	Coverage Δ	Complexity Δ
...a/com/google/api/gax/grpc/GrpcCallableFactory.java	`75% <100%> (+0.97%)`	`9 <0> (ø)`	⬇️
...api/gax/tracing/TracedClientStreamingCallable.java	`91.83% <91.83%> (ø)`	`2 <2> (?)`
...oogle/api/gax/grpc/GrpcChannelUUIDInterceptor.java	`33.33% <0%> (ø)`	`1% <0%> (?)`
...api/gax/grpc/InstantiatingGrpcChannelProvider.java	`75.17% <0%> (+0.17%)`	`27% <0%> (ø)`	⬇️
...a/com/google/api/gax/tracing/OpencensusTracer.java	`87.2% <0%> (+0.64%)`	`31% <0%> (+1%)`	⬆️

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 6359c0a...7c4560c. Read the comment docs.

vam-google · 2019-01-28T19:42:18Z

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

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


vam-google · 2019-01-28T19:43:17Z

gax/src/main/java/com/google/api/gax/tracing/TracedClientStreamingCallable.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 (more details in the previous PR).

vam-google · 2019-01-28T20:23:38Z

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

+  }
+
+  private static class FakeStreamObserver implements ApiStreamObserver<String> {
+    List<String> messages = Lists.newArrayList();


Please make these private

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

vam-google

LGTM (to speedup the process, but please check/address the comments first)

vam-google · 2019-02-20T01:37:35Z

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

+
+    @Override
+    public void onError(Throwable t) {
+      if (t == null) {


So onError can be called only in case of cancelation? Why? Btw, please rename t to throwable or error or exception.

Why is it different from similar logic in TraceResponseObserver on line 153?

This is the request stream half. Sending an error on the request stream will end up closing the response stream. So this is more similar to unary future cancellation then a TraceResponseObserver

vam-google · 2019-02-20T01:40:12Z

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

+    }
+
+    @Override
+    public void onError(Throwable t) {


tracer is not called in this method (suspicious)

added a comment explaining that this will end up closing the entire stream and tracing is deferred until then

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

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

vam-google

(One more comment, which I forgot to add initially).

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

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

igorbernstein2 requested a review from vam-google January 3, 2019 18:45

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 client streaming callables.~~ WIP: Start tracing client 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

fixes

b2e0b6a

igorbernstein2 force-pushed the oc-client-streaming branch from c1abb7d to b2e0b6a Compare January 5, 2019 07:21

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

handle operation cancellation & use static imports

e88b3db

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

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

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

662c4f1

vam-google reviewed Jan 28, 2019

View reviewed changes

igorbernstein2 added 2 commits February 2, 2019 17:49

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

9471275

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

address feedbac

48e762b

JustinBeckwith added the 🚨 This issue needs some love. label Feb 7, 2019

yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Feb 7, 2019

igorbernstein2 added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Feb 7, 2019

vam-google approved these changes Feb 20, 2019

View reviewed changes

gax/src/main/java/com/google/api/gax/tracing/TracedClientStreamingCallable.java Show resolved Hide resolved

igorbernstein2 added 4 commits February 20, 2019 11:07

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

fdf3e60

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

fix merge

1ff0684

address feedback

b921f81

address feedback

7c4560c

igorbernstein2 merged commit b685cf0 into googleapis:master Feb 20, 2019

igorbernstein2 deleted the oc-client-streaming branch February 20, 2019 17:28

This was referenced Feb 20, 2019

Release gax-java v1.40.0 #674

Merged

Release gax-java v1.41.0 #680

Closed

This was referenced Feb 28, 2019

Release gax-java v1.41.0 #686

Closed

Release gax-java v1.42.0 #689

Merged

Opencensus Tracing: Start tracing client streaming callables. #637

Opencensus Tracing: Start tracing client streaming callables. #637

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!

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!

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 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!

Uh oh!

Uh oh!

vam-google left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

igorbernstein2 commented Jan 3, 2019 •

edited

Loading

codecov-io commented Jan 5, 2019 •

edited by codecov bot

Loading