Census tracing integration for Firestore. by danchia · Pull Request #2729 · googleapis/google-cloud-java

danchia · 2017-12-20T23:11:50Z

@bogdandrutu PTAL. The dependency management was in particular quite nasty, and ideas / feedback around that area greatly appreciated.

google-cloud-firestore/pom.xml

+    <dependency>
+      <groupId>io.opencensus</groupId>
+      <artifactId>opencensus-api</artifactId>
+      <version>0.10.1</version>


danchia · 2017-12-21T00:44:48Z

I updated the PR to use census from BOM instead.

google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java


+            numResponses++;
+            if (numResponses == 1) {
+              Tracing.getTracer()


google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java

  private static final String AUTO_ID_ALPHABET =
      "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";

+  public static final Tracer tracer = Tracing.getTracer();


google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java

+                  span);
            } else {
+              span.setStatus(
+                  io.opencensus.trace.Status.ABORTED.withDescription("too many retries"));


google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java


          @Override
          public void onError(Throwable throwable) {
+            Tracing.getTracer().getCurrentSpan().addAnnotation("Firestore.BatchGet: Error");


google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java

+            numResponses++;
+            if (numResponses == 1) {
+              Tracing.getTracer()
+                  .getCurrentSpan()


danchia

Thanks for the feedback, hope this addresses things.

google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java

  private static final String AUTO_ID_ALPHABET =
      "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";

+  public static final Tracer tracer = Tracing.getTracer();


google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java


+            numResponses++;
+            if (numResponses == 1) {
+              Tracing.getTracer()


google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java

+            numResponses++;
+            if (numResponses == 1) {
+              Tracing.getTracer()
+                  .getCurrentSpan()


google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java


          @Override
          public void onError(Throwable throwable) {
+            Tracing.getTracer().getCurrentSpan().addAnnotation("Firestore.BatchGet: Error");


google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java

+                  span);
            } else {
+              span.setStatus(
+                  io.opencensus.trace.Status.ABORTED.withDescription("too many retries"));


google-cloud-firestore/src/main/java/com/google/cloud/firestore/TraceUtil.java

+import io.opencensus.trace.Status;
+
+/** Census tracing utilities. */
+class TraceUtil {


google-cloud-firestore/src/main/java/com/google/cloud/firestore/TraceUtil.java

+import io.opencensus.trace.Status;
+
+/** Census tracing utilities. */
+class TraceUtil {


google-cloud-firestore/src/main/java/com/google/cloud/firestore/TraceUtil.java

@@ -0,0 +1,35 @@
+/*
+ * Copyright 2017 Google Inc. All Rights Reserved.


google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java

      final Transaction.Function<T> transactionCallback,
      final SettableApiFuture<T> resultFuture,
      final TransactionOptions options) {
+    Span span = tracer.spanBuilder("CloudFirestore.Transaction").startSpan();


google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java

+                  span);
            } else {
+              span.setStatus(TOO_MANY_RETRIES_STATUS);
+              span.end();


google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java


    ApiStreamObserver<BatchGetDocumentsResponse> responseObserver =
        new ApiStreamObserver<BatchGetDocumentsResponse>() {
+          int numResponses;


danchia · 2018-03-06T18:04:49Z

@pongad do you have any other feedback?

pongad · 2018-04-19T18:36:10Z

@danchia Sorry for the late response. I was transferring and dropped a bunch of things. This LGTM. @garrettjonesgoogle Do you have any other comments?

garrettjonesgoogle

LGTM

danchia requested a review from garrettjonesgoogle as a code owner December 20, 2017 23:11

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

garrettjonesgoogle reviewed Dec 20, 2017

View reviewed changes

danchia force-pushed the census branch from 94d81cc to 232597d Compare December 21, 2017 00:39

bogdandrutu reviewed Feb 9, 2018

View reviewed changes

danchia force-pushed the census branch from 232597d to 5dad898 Compare February 20, 2018 19:35

danchia requested a review from pongad as a code owner February 20, 2018 19:35

danchia commented Feb 20, 2018

View reviewed changes

Census tracing integration for Firestore.

3975e78

danchia force-pushed the census branch from 5dad898 to 3975e78 Compare February 20, 2018 20:28

bogdandrutu reviewed Feb 26, 2018

View reviewed changes

Address review comments.

26265e0

bogdandrutu approved these changes Feb 28, 2018

View reviewed changes

garrettjonesgoogle approved these changes Apr 25, 2018

View reviewed changes

pongad merged commit e7d5250 into googleapis:master Apr 26, 2018

		@@ -0,0 +1,35 @@
		/*
		* Copyright 2017 Google Inc. All Rights Reserved.

Conversation

danchia commented Dec 20, 2017

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!

danchia commented Dec 21, 2017

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!

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

danchia commented Mar 6, 2018

Uh oh!

pongad commented Apr 19, 2018

Uh oh!

garrettjonesgoogle left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!