Opencensus Tracing: Add plumbing for tracing #633

igorbernstein2 · 2018-12-27T22:08:28Z

This has been extracted from #613

This doesn't provide any extra functionality. Instead it just sets up the foundation for integrating tracing in to gax.
The general idea is to add a tracing abstraction layer that all parts of gax can call into. This is accomplished by introducing

ApiTracer interface that has methods for all annotations that a span should contain
ApiTracerFactory interface: since an ApiTracer is stateful and has a 1:1 mapping with operations, a factory is introduced to cleanly switch implementations of the tracer

By default a NoopApiTracerFactory is configured that will return NoopApiTracers. In the future a parallel OpenCensusApiTracerFactory will be added. A client implementation will be able to opt into tracing by setting the factory in StubSettings. The actual ApiTracer will be propagated by CallContext.

This has been extracted from googleapis#613 This doesn't provide any extra functionality. Instead it just sets up the foundation for integrating tracing in to gax. The general idea is to add a tracing abstraction layer that all parts of gax can call into. This is accomplished by introducing - ApiTracer interface that has methods for all annotations that a span should contain - ApiTracerFactory interface: since an ApiTracer is stateful and has a 1:1 mapping with operations, a factory is introduced to cleanly switch implementations of the tracer By default a NoopApiTracerFactory is configured that will return NoopApiTracers. In the future a parallel OpenCensusApiTracerFactory will be added. A client implementation will be able to opt into tracing by setting the factory in StubSettings. The actual ApiTracer will be propagated by CallContext.

codecov-io · 2018-12-27T22:11:48Z

Codecov Report

Merging #633 into master will decrease coverage by 0.75%.
The diff coverage is 28.57%.

@@             Coverage Diff              @@
##             master     #633      +/-   ##
============================================
- Coverage     74.93%   74.18%   -0.76%     
- Complexity      935      938       +3     
============================================
  Files           177      180       +3     
  Lines          4102     4164      +62     
  Branches        328      333       +5     
============================================
+ Hits           3074     3089      +15     
- Misses          875      919      +44     
- Partials        153      156       +3

Impacted Files	Coverage Δ	Complexity Δ
...main/java/com/google/api/gax/tracing/SpanName.java	`0% <0%> (ø)`	`0 <0> (?)`
...java/com/google/api/gax/tracing/NoopApiTracer.java	`0% <0%> (ø)`	`0 <0> (?)`
...m/google/api/gax/retrying/NoopRetryingContext.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...ain/java/com/google/api/gax/rpc/ClientContext.java	`82.81% <100%> (+0.55%)`	`8 <0> (ø)`	⬇️
...m/google/api/gax/tracing/NoopApiTracerFactory.java	`25% <25%> (ø)`	`1 <1> (?)`
.../java/com/google/api/gax/grpc/GrpcCallContext.java	`78.4% <25%> (-5.68%)`	`43 <1> (+1)`
...m/google/api/gax/httpjson/HttpJsonCallContext.java	`62.35% <44.44%> (-3.87%)`	`24 <4> (ø)`
...main/java/com/google/api/gax/rpc/StubSettings.java	`73.98% <45.45%> (-2.81%)`	`11 <1> (+1)`
... 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 6f4ee42...63801fb. Read the comment docs.

codecov-io · 2018-12-27T22:11:48Z

Codecov Report

Merging #633 into master will decrease coverage by 0.37%.
The diff coverage is 52.17%.

@@             Coverage Diff              @@
##             master     #633      +/-   ##
============================================
- Coverage     74.93%   74.56%   -0.38%     
- Complexity      935      949      +14     
============================================
  Files           177      180       +3     
  Lines          4102     4172      +70     
  Branches        328      334       +6     
============================================
+ Hits           3074     3111      +37     
- Misses          875      908      +33     
  Partials        153      153

Impacted Files	Coverage Δ	Complexity Δ
...main/java/com/google/api/gax/tracing/SpanName.java	`0% <0%> (ø)`	`0 <0> (?)`
...java/com/google/api/gax/tracing/NoopApiTracer.java	`0% <0%> (ø)`	`0 <0> (?)`
...m/google/api/gax/retrying/NoopRetryingContext.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...ain/java/com/google/api/gax/rpc/ClientContext.java	`82.81% <100%> (+0.55%)`	`8 <0> (ø)`	⬇️
.../java/com/google/api/gax/grpc/GrpcCallContext.java	`85.6% <100%> (+1.52%)`	`48 <4> (+6)`	⬆️
...main/java/com/google/api/gax/rpc/StubSettings.java	`73.98% <45.45%> (-2.81%)`	`11 <1> (+1)`
...m/google/api/gax/tracing/NoopApiTracerFactory.java	`66.66% <66.66%> (ø)`	`2 <2> (?)`
...m/google/api/gax/httpjson/HttpJsonCallContext.java	`69.41% <77.77%> (+3.19%)`	`28 <7> (+4)`	⬆️
...va/com/google/api/gax/retrying/RetryAlgorithm.java	`75% <0%> (-10.72%)`	`7% <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 6f4ee42...986f42c. Read the comment docs.

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.

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.

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.

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.

vam-google

In general looks good. My only real concern is: do we really want to abstract opencensus completely (please read ApiTracer review comment for more details)?

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

 @BetaApi("Reference ApiCallContext instead - this class is likely to experience breaking changes")
 @InternalExtensionOnly
 public final class GrpcCallContext implements ApiCallContext {
+  private static final CallOptions.Key<ApiTracer> TRACER_KEY = Key.create("gax.tracer");


gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonCallContext.java

+  @Override
+  public ApiTracer getTracer() {
+    if (tracer == null) {
+      return NoopApiTracer.create();


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

+
+  private NoopApiTracer() {}
+
+  public static ApiTracer create() {


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

+   * Signals that the overall operation has failed and no further attempts will be made. The tracer
+   * is now considered closed and should no longer be used.
+   */
+  void operationFailed(Throwable error);


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

+   * {@link Scope} removes any context that the underlying implementation might've set in {@link
+   * #inScope()}.
+   */
+  interface Scope extends AutoCloseable {


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

@@ -0,0 +1,45 @@
+/*
+ * Copyright 2017 Google LLC


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

+import com.google.api.core.InternalApi;
+
+/**
+ * Factory that will build fake {@link ApiTracer}s.


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

+
+  @Override
+  public boolean equals(Object obj) {
+    return obj instanceof NoopApiTracerFactory;


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

+  /**
+   * Creates a new instance of the name.
+   *
+   * @param clientName The name of the client. ie BigtableData


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

+  public abstract String getMethodName();
+
+  /** Creates a new instance with the clientName overriden. */
+  public SpanName withClientName(String clientName) {


vam-google

In general LGTM, with several semi-important comments. Please address those before submitting, if found reasonable.

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

+  public abstract String getMethodName();
+
+  /** Creates a new instance with the clientName overriden. */
+  public SpanName withClientName(String clientName) {


gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonCallContextTest.java

+    HttpJsonCallContext ctxWithDefaultTracer = HttpJsonCallContext.createDefault();
+    ApiTracer defaultTracer = ctxWithDefaultTracer.getTracer();
+
+    // Explicit tracers override default tracers


gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonCallContextTest.java

+    Truth.assertThat(ctxWithDefaultTracer.merge(ctxWithExplicitTracer).getTracer())
+        .isSameAs(explicitTracer);
+
+    // Default tracer do not override explicit tracers


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

 @BetaApi("Reference ApiCallContext instead - this class is likely to experience breaking changes")
 @InternalExtensionOnly
 public final class GrpcCallContext implements ApiCallContext {
+  private static final CallOptions.Key<ApiTracer> TRACER_KEY = Key.create("gax.tracer");


igorbernstein2 · 2019-01-05T06:34:16Z

I think addressed all of the concerns. Thanks for the review! I'm gonna go ahead and merge this. If there is anything else I should change I'll do it in a follow up PR

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.

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.

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.

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.

* Start tracing unary callable. 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. * trace http json calls as well * fix test * oops * a bit more work * improve method name parsing * address feedback * trace operation cancellation and use static mockito imports in tests * address feedback

* Start tracing bidi streaming callables. 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. * fixes * cancellation & static imports * address some feedback * address more feedback

* Start tracing client streaming callables. 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. * fixes * handle operation cancellation & use static imports * address feedbac * fix merge * address feedback * address feedback

igorbernstein2 requested a review from vam-google December 27, 2018 22:08

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

rename tracer method to subject action

3336fe6

igorbernstein2 mentioned this pull request Dec 28, 2018

Opencensus Tracing: Start tracing unary callable. #634

Merged

igorbernstein2 mentioned this pull request Jan 2, 2019

Opencensus Tracing: Start tracing server streaming callables. #635

Merged

igorbernstein2 mentioned this pull request Jan 3, 2019

Opencensus Tracing: Start tracing bidi streaming callables. #636

Merged

igorbernstein2 mentioned this pull request Jan 3, 2019

Opencensus Tracing: Start tracing client streaming callables. #637

Merged

vam-google reviewed Jan 3, 2019

View reviewed changes

address feedback

6938879

vam-google approved these changes Jan 4, 2019

View reviewed changes

address feedback

986f42c

igorbernstein2 merged commit 122eda6 into googleapis:master Jan 5, 2019

igorbernstein2 deleted the oc-tracer branch January 5, 2019 06:38

andreamlin mentioned this pull request Jan 9, 2019

Release gax-java v1.36.0 #642

Merged

igorbernstein2 changed the title ~~Add plumbing for tracing~~ Opencensus Tracing: Add plumbing for tracing Jan 11, 2019


		private NoopApiTracer() {}

		public static ApiTracer create() {

Opencensus Tracing: Add plumbing for tracing #633

Opencensus Tracing: Add plumbing for tracing #633

Uh oh!

Conversation

igorbernstein2 commented Dec 27, 2018

Uh oh!

codecov-io commented Dec 27, 2018

Codecov Report

Uh oh!

codecov-io commented Dec 27, 2018 • edited 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!

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!

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!

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!

igorbernstein2 commented Jan 5, 2019

codecov-io commented Dec 27, 2018 •

edited

Loading