Opencensus Tracing: Start tracing unary callable. by igorbernstein2 · Pull Request #634 · googleapis/gax-java

igorbernstein2 · 2018-12-28T15:35:52Z

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

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

vam-google

My understanding was that we can proceed with Traced* callable wrappers only if we find the way to make retrying logic non-aware of tracing (item 2 here).

Traced callables add a lot of complexity and I do not see a good way to get rid from tracing-specific logic in retry logic. If retries, as a more "core" piece, already have a hard dependency on tracing, I would make tracing just a core component of the whole thing (i.e. just put necessary tracing-specific stuff where needed, no need in wrapping instrumentation).

Also I do not understand the relationship between "not having Traced* callables in the callables chain" and "having noop tracer". It feels like exactly same thing (two ways to "turn tracing off").

I have really strong feelings about this one and would like to avoid Traced* callables if there is no strong need to have them.

This depends on googleapis#633 and is extracted from googleapis#613. This introduces TracedUnaryCallables 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.

codecov-io · 2019-01-05T06:45:16Z

Codecov Report

Merging #634 into master will increase coverage by 0.19%.
The diff coverage is 91.89%.

@@             Coverage Diff              @@
##             master     #634      +/-   ##
============================================
+ Coverage     74.56%   74.76%   +0.19%     
- Complexity      949      958       +9     
============================================
  Files           180      182       +2     
  Lines          4172     4209      +37     
  Branches        334      334              
============================================
+ Hits           3111     3147      +36     
- Misses          908      909       +1     
  Partials        153      153

Impacted Files	Coverage Δ	Complexity Δ
...a/com/google/api/gax/grpc/GrpcCallableFactory.java	`75.4% <100%> (+4.25%)`	`8 <1> (+1)`	⬆️
...java/com/google/api/gax/tracing/TraceFinisher.java	`100% <100%> (ø)`	`3 <3> (?)`
...om/google/api/gax/tracing/TracedUnaryCallable.java	`100% <100%> (ø)`	`2 <2> (?)`
...ogle/api/gax/httpjson/HttpJsonCallableFactory.java	`32% <57.14%> (+9.77%)`	`2 <1> (+1)`	⬆️
...main/java/com/google/api/gax/tracing/SpanName.java	`50% <0%> (+50%)`	`2% <0%> (+2%)`	⬆️

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 122eda6...8e87c1e. Read the comment docs.

codecov-io · 2019-01-05T06:45:16Z

Codecov Report

Merging #634 into master will increase coverage by 0.07%.
The diff coverage is 89.47%.

@@             Coverage Diff              @@
##             master     #634      +/-   ##
============================================
+ Coverage     74.56%   74.64%   +0.07%     
- Complexity      949      961      +12     
============================================
  Files           180      183       +3     
  Lines          4172     4216      +44     
  Branches        334      334              
============================================
+ Hits           3111     3147      +36     
- Misses          908      916       +8     
  Partials        153      153

Impacted Files	Coverage Δ	Complexity Δ
...java/com/google/api/gax/tracing/NoopApiTracer.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...a/com/google/api/gax/grpc/GrpcCallableFactory.java	`74.57% <100%> (+3.42%)`	`9 <2> (+2)`	⬆️
...java/com/google/api/gax/tracing/TraceFinisher.java	`100% <100%> (ø)`	`4 <4> (?)`
...om/google/api/gax/tracing/TracedUnaryCallable.java	`100% <100%> (ø)`	`2 <2> (?)`
...ogle/api/gax/httpjson/HttpJsonCallableFactory.java	`32% <57.14%> (+9.77%)`	`3 <2> (+2)`	⬆️
.../com/google/api/gax/grpc/GrpcTransportChannel.java	`59.09% <0%> (-13.14%)`	`9% <0%> (ø)`
.../com/google/api/gax/batching/BatchingSettings.java	`80.95% <0%> (ø)`	`2% <0%> (ø)`	⬇️
...om/google/api/gax/core/ResourceCloseException.java	`0% <0%> (ø)`	`0% <0%> (?)`
... and 1 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 122eda6...167a200. Read the comment docs.

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

As discussed offline, the separate callables are necessary. I've rebased this PR on top of the plumbing work, so it's ready for review

vam-google

LGTM with some comments, please address them before pushing.

Sign in to view

  }
+
+  @InternalApi("Visible for testing")
+  static SpanName getSpanName(MethodDescriptor<?, ?> methodDescriptor) {


Sign in to view

  }
+
+  @InternalApi("Visible for testing")
+  static SpanName getSpanName(@Nonnull ApiMethodDescriptor<?, ?> methodDescriptor) {


Sign in to view

+import com.google.common.util.concurrent.MoreExecutors;
+
+/**
+ * A wrapper callable that will wrap a callable chain in a trace.


Sign in to view

+    this.spanName = spanName;
+  }
+
+  /** Calls the wrapped {@link UnaryCallable} within the context of a new trace. */


Sign in to view

+
+      return future;
+    } catch (RuntimeException e) {
+      finisher.onFailure(e);


Sign in to view

+
+@RunWith(JUnit4.class)
+public class TraceFinisherTest {
+  @Rule public MockitoRule mockitoRule = MockitoJUnit.rule();


igorbernstein2 · 2019-01-10T21:37:25Z

Added operation cancellation tracing & added static imports as suggested in #640.

PTAL

vam-google

LGTM, with 2 (essentially 1) minor comments.

Sign in to view

-    String fullServiceName = methodDescriptor.getFullMethodName().substring(0, index);
-    String methodName = methodDescriptor.getFullMethodName().substring(index + 1);
+  static SpanName getSpanName(@Nonnull MethodDescriptor<?, ?> methodDescriptor) {
+    Pattern pattern = Pattern.compile("^.*?([^./]+)/([^./]+)$");


Sign in to view

-    int index = methodDescriptor.getFullMethodName().indexOf('.');
-    String serviceName = methodDescriptor.getFullMethodName().substring(0, index);
-    String methodName = methodDescriptor.getFullMethodName().substring(index + 1);
+    Pattern pattern = Pattern.compile("^(.+)\\.(.+)$");


igorbernstein2 requested a review from vam-google December 28, 2018 15:35

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 28, 2018

vam-google reviewed Jan 4, 2019

View reviewed changes

igorbernstein2 added 4 commits January 5, 2019 01:39

trace http json calls as well

2dbe1e7

fix test

49cdadf

oops

8e87c1e

igorbernstein2 force-pushed the oc-unary branch from a7a3302 to 8e87c1e Compare January 5, 2019 06:42

a bit more work

0080661

This was referenced Jan 5, 2019

Opencensus Tracing: Start tracing server streaming callables. #635

Merged

Opencensus Tracing: Start tracing bidi streaming callables. #636

Merged

Opencensus Tracing: Start tracing client streaming callables. #637

Merged

vam-google approved these changes Jan 9, 2019

View reviewed changes

igorbernstein2 added 3 commits January 9, 2019 22:28

improve method name parsing

1cbd55c

address feedback

93fabeb

trace operation cancellation and use static mockito imports in tests

8c17afa

vam-google approved these changes Jan 11, 2019

View reviewed changes

address feedback

167a200

igorbernstein2 changed the title ~~Start tracing unary callable.~~ Opencensus Tracing: Start tracing unary callable. Jan 11, 2019

igorbernstein2 merged commit 33a57b6 into googleapis:master Jan 11, 2019

igorbernstein2 deleted the oc-unary branch January 11, 2019 19:05

This was referenced Jan 14, 2019

Release gax-java v1.36.1 #645

Closed

Release gax-java v1.37.0 #646

Merged

Uh oh!

Conversation

igorbernstein2 commented Dec 28, 2018

Uh oh!

vam-google left a comment • 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!

codecov-io commented Jan 5, 2019

Codecov Report

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 5, 2019

Uh oh!

vam-google left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

igorbernstein2 commented Jan 10, 2019

Uh oh!

vam-google left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vam-google left a comment •

edited

Loading

codecov-io commented Jan 5, 2019 •

edited by codecov Bot

Loading