Opencensus Tracing: Implement ApiTracer using opencensus api #641

igorbernstein2 · 2019-01-08T23:29:34Z

No description provided.

codecov · 2019-01-09T04:28:55Z

Codecov Report

Merging #641 into master will increase coverage by 0.23%.
The diff coverage is 82.4%.

@@             Coverage Diff              @@
##             master     #641      +/-   ##
============================================
+ Coverage     75.22%   75.45%   +0.23%     
- Complexity      971     1000      +29     
============================================
  Files           183      185       +2     
  Lines          4197     4322     +125     
  Branches        335      343       +8     
============================================
+ Hits           3157     3261     +104     
- Misses          887      906      +19     
- Partials        153      155       +2

Impacted Files	Coverage Δ	Complexity Δ
...main/java/com/google/api/gax/tracing/SpanName.java	`80% <100%> (+30%)`	`4 <1> (+2)`	⬆️
...oogle/api/gax/tracing/OpencensusTracerFactory.java	`42.85% <42.85%> (ø)`	`3 <3> (?)`
...a/com/google/api/gax/tracing/OpencensusTracer.java	`90.29% <90.29%> (ø)`	`24 <24> (?)`

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 6c8cd40...f96e981. Read the comment docs.

vam-google

(Lots of comments, but most of them are minor)

build.gradle

        authCredentials: "com.google.auth:google-auth-library-credentials:${authVersion}",
        commonProtos: "com.google.api.grpc:proto-google-common-protos:${commonProtosVersion}",
        apiCommon: "com.google.api:api-common:1.7.0",
+        opencensusApi: "io.opencensus:opencensus-api:${opencensusVersion}",


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

  /** Adds an annotation that the attempt succeeded. */
  void attemptSucceeded();

+  /** Add an annotation that the attempt was cancelled by the user. */


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

   * Adds an annotation that the attempt failed and that no further attempts will be made because
   * retry limits have been reached.
+   *
+   * @param error


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

+ * <pre>
+ *   ClientName.ServerStreamingMethod
+ *     - attributes:
+ *       - {@code attempt count}: number of attempts sent before the logical operation completed


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

+ * <p>This implementation wraps an OpenCensus {@link Span} for every tracer and annotates that
+ * {@link Span} with various events throughout the lifecycle of the logical operation.
+ *
+ * <pre>


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

+
+    attributes.put("attempt count", AttributeValue.longAttributeValue(currentAttemptId + 1));
+
+    if (totalAttemptRequests > 0) {


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

+ * <p>This class wraps the {@link Tracer} provided by Opencensus in {@code Tracing.getTracer()}. It
+ * will be used to create new spans and wrap them in {@link OpencensusTracer} defined in gax.
+ */
+@InternalApi("For google-cloud-java client use only")


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

+  @InternalApi("Visible for testing")
+  OpencensusTracerFactory(Tracer internalTracer, @Nullable String clientNameOverride) {
+    this.internalTracer =
+        Preconditions.checkNotNull(internalTracer, "internalTracer can't be null");


igorbernstein2 · 2019-01-23T21:47:55Z

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

+ * <p>This class wraps the {@link Tracer} provided by Opencensus in {@code Tracing.getTracer()}. It
+ * will be used to create new spans and wrap them in {@link OpencensusTracer} defined in gax.
+ */
+@InternalApi("For google-cloud-java client use only")


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

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


igorbernstein2 · 2019-01-17T20:51:08Z

Will address the rest of the feedback shortly. Thanks for reviewing!

igorbernstein2 · 2019-01-23T21:48:20Z

All feedback has been addressed. PTAL

vam-google

LGTM, but please at least make the OpencensusTracer's fields volatile (it won't harm, performance effect is neglectable, please read the comment for details).

vam-google · 2019-01-28T18:03:59Z

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

+  }
+
+  private Map<String, AttributeValue> baseOperationAttributes() {
+    HashMap<String, AttributeValue> attributes = Maps.newHashMap();


In any case Maps.newHashMap() should not be used for java 7+ (I think the only reason why it exists is to imitate "diamond" generics in java 6, since they were added in java 7 we can use normal new HashMap<>() (this is what the documentation for Maps.newHashMap() suggests)).

vam-google · 2019-01-28T18:43:13Z

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

+  /** {@inheritDoc} */
+  @Override
+  public void requestSent() {
+    attemptSentMessages++;


I'm not sure what you mean by adding noise with synchronization here. It is really unnerving to keep a class which will be initizlized/modified/consumed by various threads unsynchronzied (I did read the IBM's article about thread safety categories).

Well, we always can add some "thread friendliness" by doing something like the following:

Make fields (like attemptSentMessages) at at least volatile (to ensure thread visibility).

Or Make them Atomic* (visibility + atomic increments of each field)

Or Synchronize the whole class (could be done in an old-fashioned way of adding synchronize keyword to each method signature, which can greatly reduce amount of boilerplate code, if that is what you meant by noise) (to ensure atomicity of the whole thing).

In any case, this is a reporting class, which does not affect decision-making process and even if updates to it stay non-atomic and/or are not properly syncronized by the outer code the worst thing which can happen is slightly wrong report numbers. Taking that into account can you at least make the non-final fields of method at least volatile (better Anomic) to make me sleep better?

As for performance effect, we already do locking, so it won't change anything drastically. According to Jeff Dean's numbers Mutex lock/unlock is about 25ns (and those are quite old, the google's internal version of the table says around 17ns now). In any case it is quite cheap, especially taking into account how much an actual network call takes.

In this scenario, there are only 2 threads that are relevant: the caller thread & the grpc thread. The grpc thread is responsible for closing spans and generating annotations.

So the counters that are only modified by the grpc thread dont need any special handling. The other counters (for sending msgs & tracking the attempt number) due need some consideration. ~~I marked them as volatile~~. I made them atomic. Thanks for catching that.

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

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

+  private final Span span;
+
+  private long currentAttemptId;
+  private long attemptSentMessages = 0;


So it is very minor but I'm still curious why these are explicitly set to 0? Especially taking into account that the above one (currentAttemptId) is not.

currentAttemptId gets stamped, while the others get incremented.

WIP: implement ApiTracer using opencensus api

eb5531d

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

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

igorbernstein2 added 2 commits January 8, 2019 23:25

add tests

486832c

oops

3c3ccd8

igorbernstein2 changed the title ~~WIP: implement ApiTracer using opencensus api~~ Implement ApiTracer using opencensus api Jan 9, 2019

igorbernstein2 requested a review from vam-google January 9, 2019 04:28

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

igorbernstein2 added 2 commits January 9, 2019 10:33

add more tests

dc0461f

remove unused method

071e28f

igorbernstein2 changed the title ~~Implement ApiTracer using opencensus api~~ WIP: Implement ApiTracer using opencensus api 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: Implement ApiTracer using opencensus api~~ WIP: Opencensus Tracing: Implement ApiTracer using opencensus api Jan 11, 2019

igorbernstein2 added 3 commits January 11, 2019 16:09

Merge remote-tracking branch 'origin/master' into oc-tracer-impl

aba1187

operation cancellation & static imports

bb6fec7

format

c39e967

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: Opencensus Tracing: Implement ApiTracer using opencensus api~~ Opencensus Tracing: Implement ApiTracer using opencensus api Jan 11, 2019

Merge remote-tracking branch 'origin/master' into oc-tracer-impl

b3196e9

vam-google reviewed Jan 17, 2019

View reviewed changes

igorbernstein2 added 5 commits January 17, 2019 16:37

address feedback

90a6f15

Merge remote-tracking branch 'origin/master' into oc-tracer-impl

faf5cd2

update docs

ef22030

typo

a7c8025

Merge remote-tracking branch 'origin/master' into oc-tracer-impl

4d13c86

fix test

89a4777

vam-google approved these changes Jan 28, 2019

View reviewed changes

address feedback

f96e981

igorbernstein2 merged commit b8923fa into googleapis:master Jan 28, 2019

igorbernstein2 deleted the oc-tracer-impl branch January 28, 2019 22:46

This was referenced Jan 31, 2019

Release gax-java v1.38.0 #651

Closed

Release gax-java v1.38.0 #655

Merged


		attributes.put("attempt count", AttributeValue.longAttributeValue(currentAttemptId + 1));

		if (totalAttemptRequests > 0) {

Opencensus Tracing: Implement ApiTracer using opencensus api #641

Opencensus Tracing: Implement ApiTracer using opencensus api #641

Uh oh!

Conversation

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

Uh oh!

Uh oh!

codecov bot commented Jan 9, 2019 • 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!

igorbernstein2 Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

igorbernstein2 commented Jan 17, 2019

Uh oh!

igorbernstein2 commented Jan 23, 2019

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!

vam-google Jan 28, 2019

Choose a reason for hiding this comment

Uh oh!

vam-google Jan 28, 2019

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 Jan 28, 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!

vam-google Jan 28, 2019

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 Jan 28, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

igorbernstein2 commented Jan 8, 2019 •

edited

Loading

codecov bot commented Jan 9, 2019 •

edited

Loading

vam-google left a comment •

edited

Loading

igorbernstein2 Jan 28, 2019 •

edited

Loading