feat: add opencensus tracing/stats support by suraj-qlogic · Pull Request #130 · googleapis/java-datastore

suraj-qlogic · 2020-05-19T06:20:49Z

Fixes #109

codecov · 2020-05-19T06:25:42Z

Codecov Report

Merging #130 into master will decrease coverage by 1.12%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master     #130      +/-   ##
============================================
- Coverage     58.30%   57.17%   -1.13%     
  Complexity      384      384              
============================================
  Files            47       48       +1     
  Lines          2331     2377      +46     
  Branches        261      264       +3     
============================================
  Hits           1359     1359              
- Misses          865      911      +46     
  Partials        107      107

Impacted Files	Coverage Δ	Complexity Δ
...java/com/google/cloud/datastore/DatastoreImpl.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...ain/java/com/google/cloud/datastore/TraceUtil.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...oogle/cloud/datastore/spi/v1/HttpDatastoreRpc.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`

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 d572efb...9d85b03. Read the comment docs.

skuruppu · 2020-05-20T03:50:30Z

@mayurkale22, would you be able to take a quick look at this PR to see if OpenCensus is being used correctly? Thanks heaps.

mayurkale22 · 2020-05-20T05:39:56Z

pom.xml

    <guava.version>29.0-android</guava.version>
    <threeten.version>1.4.4</threeten.version>
    <javax.annotations.version>1.3.2</javax.annotations.version>
+    <opencensus.version>0.24.0</opencensus.version>


I would use latest version: 0.26.0.

Recently google-cloud-shared-dependencies was added in this client which is pull latest version automatically.

mayurkale22 · 2020-05-20T06:37:37Z

google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreImpl.java

  com.google.datastore.v1.BeginTransactionResponse beginTransaction(
      final com.google.datastore.v1.BeginTransactionRequest requestPb) {
+    Span span = traceUtil.startSpan(TraceUtil.SPAN_NAME_BEGINTRANSACTION);
+    Scope scope = traceUtil.getTracer().withSpan(span);


Personally, I prefer to use try-with-resources for scope. It implements AutoClosable, so it'll be closed when the try block ends. Ex:

try (Scope s = tracer.withSpan(span)) { ... rest of the logic }

elharo · 2020-05-20T11:13:28Z

google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreImpl.java

    this.datastoreRpc = options.getDatastoreRpcV1();
    retrySettings =
        MoreObjects.firstNonNull(options.getRetrySettings(), ServiceOptions.getNoRetrySettings());
+    traceUtil = TraceUtil.getInstance();


just set this in the declaration on line 57

elharo · 2020-05-20T11:14:18Z

google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreImpl.java

@@ -131,29 +136,41 @@ public T call() throws DatastoreException {
  @Override
  public <T> T runInTransaction(final TransactionCallable<T> callable) {
    final DatastoreImpl self = this;


this self variable seems pointless

elharo · 2020-05-20T11:14:34Z

google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreImpl.java

  @Override
  public <T> T runInTransaction(
      final TransactionCallable<T> callable, TransactionOptions transactionOptions) {
    final DatastoreImpl self = this;


elharo · 2020-05-20T11:16:29Z

google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreImpl.java

      final TransactionCallable<T> callable, TransactionOptions transactionOptions) {
    final DatastoreImpl self = this;
+    Span span = traceUtil.startSpan(TraceUtil.SPAN_NAME_TRANSACTION);
+    Scope scope = traceUtil.getTracer().withSpan(span);


You can use try with resources for the scope

elharo · 2020-05-20T11:18:08Z

google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreImpl.java

  com.google.datastore.v1.RunQueryResponse runQuery(
      final com.google.datastore.v1.RunQueryRequest requestPb) {
+    Span span = traceUtil.startSpan(TraceUtil.SPAN_NAME_RUNQUERY);
+    Scope scope = traceUtil.getTracer().withSpan(span);


You can use try with resources for the scope (unless the order of closing the span would be wrong?)

elharo · 2020-05-20T11:18:59Z

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

+ * Helper class for tracing utility. It is used for instrumenting {@link HttpDatastoreRpc} with Open
+ * Census APIs.
+ *
+ * <p>TraceUtil Instance are created by the {@link TraceUtil#getInstance()} method.


elharo · 2020-05-20T11:19:15Z

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

+ */
+public class TraceUtil {
+  private final Tracer tracer = Tracing.getTracer();
+  private static TraceUtil traceUtil;


elharo · 2020-05-20T11:19:54Z

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

+   *
+   * @return An instance of {@link TraceUtil}
+   */
+  public static TraceUtil getInstance() {


no need, just create a TraceUtil on the field

elharo · 2020-05-20T11:20:35Z

google-cloud-datastore/src/main/java/com/google/cloud/datastore/spi/v1/HttpDatastoreRpc.java

+    // Open Census initialization
+    CensusHttpModule censusHttpModule =
+        new CensusHttpModule(TraceUtil.getInstance().getTracer(), true);
+    delegate = censusHttpModule.getHttpRequestInitializer(delegate);


try to avoid reassigning local variables

suraj-qlogic · 2020-05-21T05:42:53Z

@mayurkale22 PTAL

mayurkale22

Added one comment, overall LGTM.

mayurkale22 · 2020-05-21T20:44:13Z

google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreImpl.java

          EXCEPTION_HANDLER,
          getOptions().getClock());
    } catch (RetryHelperException e) {
+      span.setStatus(Status.UNKNOWN.withDescription(e.getMessage()));


Any specific reason for Status.UNKNOWN instead of status.INTERNAL? Generally, an example of (UNKNOWN) where this error may be returned is if a Status value received from another address space belongs to an error-space that is not known in this address space. On the other side, Internal errors mean some invariants expected by underlying system has been broken.

Also, DEADLINE_EXCEEDED can be useful in case of retries exceeded.

Having said that, I am not familiar with datastore codebase. Feel free to ignore if you disagree.

suraj-qlogic · 2020-05-22T16:15:40Z

@elharo PTAL

suraj-qlogic · 2020-05-28T05:28:26Z

@elharo gentle ping

suraj-qlogic · 2020-06-04T06:56:20Z

@BenWhitehead @elharo PTAL

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

googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 19, 2020

suraj-qlogic requested a review from BenWhitehead May 19, 2020 06:26

suraj-qlogic force-pushed the java-datastore-109 branch from d16bb7e to da7fead Compare May 20, 2020 06:18

mayurkale22 reviewed May 20, 2020

View reviewed changes

elharo reviewed May 20, 2020

View reviewed changes

suraj-qlogic requested a review from elharo May 21, 2020 05:37

mayurkale22 approved these changes May 21, 2020

View reviewed changes

suraj-qlogic force-pushed the java-datastore-109 branch from 26986bd to 0c18852 Compare May 29, 2020 11:37

suraj-qlogic self-assigned this May 29, 2020

suraj-qlogic added 2 commits June 4, 2020 12:14

feat: add opencensus tracing/stats support

c6806ab

feat: fix review changes

9d85b03

suraj-qlogic force-pushed the java-datastore-109 branch from 0c18852 to 9d85b03 Compare June 4, 2020 06:49

BenWhitehead requested changes Jun 4, 2020

View reviewed changes

google-cloud-datastore/src/main/java/com/google/cloud/datastore/TraceUtil.java Show resolved Hide resolved

suraj-qlogic requested a review from BenWhitehead June 8, 2020 03:05

BenWhitehead approved these changes Jun 8, 2020

View reviewed changes

BenWhitehead merged commit 5aee52f into googleapis:master Jun 8, 2020

Conversation

suraj-qlogic commented May 19, 2020

Uh oh!

codecov bot commented May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

skuruppu commented May 20, 2020

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!

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!

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!

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!

suraj-qlogic commented May 21, 2020

Uh oh!

mayurkale22 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suraj-qlogic commented May 22, 2020

Uh oh!

suraj-qlogic commented May 28, 2020

Uh oh!

suraj-qlogic commented Jun 4, 2020

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented May 19, 2020 •

edited

Loading