OpenCensus Support for Cloud Pub/Sub #4240

dinooliva · 2018-12-18T20:16:09Z

Adds OpenCensus context propagation to the CPS Publisher and Subscriber. This pr is initially meant for discussion between OpenCensus and CPS.

sduskis · 2019-01-10T14:11:55Z

@dinooliva, did @igorbernstein2 reach out to you yet about his work in GAX? That work should supersede this PR.

igorbernstein2 · 2019-01-10T15:13:36Z

we are in touch

sduskis

Please add a some unit tests for this

sduskis · 2019-01-11T14:03:14Z

google-cloud-clients/google-cloud-pubsub/pom.xml

+    <dependency>
+      <groupId>io.opencensus</groupId>
+      <artifactId>opencensus-api</artifactId>
+      <version>${opencensus.version}</version>


This version should be inherited from google-cloud-clients

This still needs to be changed. Please remove <version>${opencensus.version}</version>

Seems this dependency is inherited from google-cloud-clients so I can remove it altogether.

sduskis · 2019-01-25T04:30:41Z

...oud-clients/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/OpenCensusUtil.java

+        .spanBuilderWithExplicitParent(name, tracer.getCurrentSpan())
+        .setRecordEvents(true)
+        // TODO(dpo): set to default.
+        .setSampler(Samplers.alwaysSample())


@dinooliva, does this need to be fixed before merging?

Yes, as per the TODO() - done now.

sduskis · 2019-01-25T04:45:17Z

google-cloud-clients/google-cloud-pubsub/pom.xml

  </parent>
  <properties>
    <site.installationModule>google-cloud-pubsub</site.installationModule>
+    <opencensus.version>0.15.1</opencensus.version>


Please remove this variable.

I've moved the variable to google-cloud-clients.

igorbernstein2 · 2019-01-25T15:42:58Z

...oud-clients/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/OpenCensusUtil.java

+      String encodedSpanContext = message.getAttributesOrDefault(TRACE_CONTEXT_KEY, "");
+      try (
+          Scope spanScope = createScopedSpan("receiver");
+          Scope statsScope = createScopedTagContext(encodedTagContext)) {


Shouldn't the creation of the span be conditional on the attributes being present?

igorbernstein2 · 2019-01-25T15:55:27Z

...oud-clients/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/OpenCensusUtil.java

+  @MustBeClosed
+  private static Scope createScopedSpan(String name) {
+    return tracer
+        .spanBuilderWithExplicitParent(name, tracer.getCurrentSpan())


What would be the current span on the receiver side? Shouldn't it start a new root span? Also, shouldn't recordEvents setting be copied from the publisher's span config?

On the receiver side, we create a root span and set the publisher's span as a parent link.

igorbernstein2 · 2019-01-25T15:58:16Z

...oud-clients/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/OpenCensusUtil.java

+      String encodedTagContext = message.getAttributesOrDefault(TAG_CONTEXT_KEY, "");
+      String encodedSpanContext = message.getAttributesOrDefault(TRACE_CONTEXT_KEY, "");
+      try (
+          Scope spanScope = createScopedSpan("receiver");


If I'm not mistaken, there is a good probability that this will be a top level span. Since this is a continuation of the publisher span, maybe it should be prefixed with the publisher span's name?

We don't transfer the Publisher's span name over the wire so it's not available. The link contains the Publisher's trace and span id's so users can find them through that.

igorbernstein2 · 2019-01-25T16:49:30Z

...le-cloud-clients/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/Publisher.java

      throw new IllegalStateException("Cannot publish on a shut-down publisher.");
    }
-
+    PubsubMessage message = OpenCensusUtil.putOpenCensusAttributes(originalMessage);


Is there any way to hook into OpenCensus' sampling logic here and avoid increasing request size if the span will not be published?

sduskis · 2019-02-08T13:21:36Z

google-cloud-clients/google-cloud-pubsub/pom.xml

+    <dependency>
+      <groupId>io.opencensus</groupId>
+      <artifactId>opencensus-api</artifactId>
+      <version>${opencensus.version}</version>


This still needs to be changed. Please remove <version>${opencensus.version}</version>

sduskis · 2019-02-08T13:22:31Z

...oud-clients/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/OpenCensusUtil.java

@@ -0,0 +1,193 @@
+/* Copyright 2018 Google Inc.


s/2018/2019

sduskis · 2019-02-08T13:22:43Z

...oud-clients/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/OpenCensusUtil.java

+import java.util.logging.Level;
+import java.util.logging.Logger;
+
+final class OpenCensusUtil {


class comments are need

Done.

Are class comments necessary even though the class isn't public?

Will we have to make this class public in the new scheme?

sduskis · 2019-02-08T13:25:01Z

google-cloud-clients/pom.xml

    <!-- test dependency versions -->
    <easymock.version>3.4</easymock.version>
    <objenesis.version>2.6</objenesis.version>
+    <opencensus.version>0.16.0</opencensus.version>


There is already an <opencensus.version> above. Please remove this duplicate entry.

sduskis · 2019-02-08T13:26:26Z

...oud-clients/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/OpenCensusUtil.java

+  private static final Tracer tracer = Tracing.getTracer();
+
+  // Used in Publisher.
+  // TODO: consider adding configuration support to control adding these attributes.


I believe we need the configuration control before merging this. This should be off by default, I would think.

Sure, that makes sense - do you have any links to how configuration is handled in the Google Cloud Java client libraries?

I don't know offhand. I'm just concerned that this will change user behavior without user selecting to turn this on.

The library only has a dependency on the OpenCensus API library, which means this code is a no-op. To enable propagation, an application will need to explicitly add a dependency on an OpenCensus Impl library. This is how configuration is done for other Java cloud client libraries (e.g. cloud bigtable and spanner).

sduskis · 2019-02-08T13:27:36Z

...oud-clients/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/OpenCensusUtil.java

+    }
+
+    @Override
+    public void receiveMessage(PubsubMessage message, AckReplyConsumer consumer) {


Do we need configuration here as well, so that the user can define whether or not they want this?

I think that makes sense too.

sduskis · 2019-02-12T23:39:57Z

...oud-clients/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/OpenCensusUtil.java

+  private static final Tracer tracer = Tracing.getTracer();
+
+  // Used in Publisher.
+  // TODO: consider adding configuration support to control adding these attributes.


I don't know offhand. I'm just concerned that this will change user behavior without user selecting to turn this on.

sduskis · 2019-02-12T23:41:39Z

...oud-clients/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/OpenCensusUtil.java

+    }
+    return "traceid=" + traceId.toLowerBase16()
+        + "&spanid=" + spanId.toLowerBase16()
+        + "&traceopt=" + (traceOpts.isSampled() ? "t&" : "f&");


should this line now be "&traceopt=f&"?

I've changed the text encoding of span to use our standard TraceContext text format.

codecov · 2019-02-14T16:49:53Z

Codecov Report

Merging #4240 into master will increase coverage by 0.47%.
The diff coverage is 70.42%.

@@             Coverage Diff              @@
##             master    #4240      +/-   ##
============================================
+ Coverage     49.19%   49.66%   +0.47%     
- Complexity    21958    22705     +747     
============================================
  Files          2076     2158      +82     
  Lines        207416   213518    +6102     
  Branches      24111    24206      +95     
============================================
+ Hits         102033   106043    +4010     
- Misses        97207    99182    +1975     
- Partials       8176     8293     +117

Impacted Files	Coverage Δ	Complexity Δ
...ain/java/com/google/cloud/pubsub/v1/Publisher.java	`80.97% <62.5%> (-0.68%)`	`26 <0> (ø)`
...ava/com/google/cloud/pubsub/v1/OpenCensusUtil.java	`71.42% <71.42%> (ø)`	`5 <5> (?)`
...mpute/v1/stub/HttpJsonDiskTypeCallableFactory.java	`60% <0%> (-15%)`	`3% <0%> (ø)`
...ute/v1/stub/HttpJsonAutoscalerCallableFactory.java	`60% <0%> (-15%)`	`3% <0%> (ø)`
...b/HttpJsonInstanceGroupManagerCallableFactory.java	`60% <0%> (-15%)`	`3% <0%> (ø)`
...HttpJsonInterconnectAttachmentCallableFactory.java	`60% <0%> (-15%)`	`3% <0%> (ø)`
...compute/v1/stub/HttpJsonRegionCallableFactory.java	`60% <0%> (-15%)`	`3% <0%> (ø)`
...1/stub/HttpJsonRegionOperationCallableFactory.java	`60% <0%> (-15%)`	`3% <0%> (ø)`
...b/HttpJsonRegionBackendServiceCallableFactory.java	`60% <0%> (-15%)`	`3% <0%> (ø)`
...compute/v1/stub/HttpJsonUrlMapCallableFactory.java	`60% <0%> (-15%)`	`3% <0%> (ø)`
... and 268 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 9ef0c0e...b2a41f3. Read the comment docs.

…CPS.

…ned annotations.

…g spec has been finalized (census-instrumentation/opencensus-specs#65).

dinooliva · 2019-02-26T17:49:43Z

Note: I developed the design of the OpenCensus Pub/Sub integration in conjunction with the Cloud Pub/Sub team.

Also, as previously noted, similar to Google Cloud Bigtable and Spanner (and other libraries e.g. gRPC), this feature is inactive unless a dependency on an OpenCensus implementation is added to the application.

sduskis · 2019-03-01T02:05:00Z

google-cloud-clients/google-cloud-pubsub/pom.xml

      <artifactId>grpc-google-iam-v1</artifactId>
      <scope>test</scope>
    </dependency>
+    <dependency>


Do we really need to add opencensus-impl here?

We should not, only the final app should do this.

These are the test dependencies rather than the library dependencies. I use the implementation to test OpenCensusUtil.

google-cloud-clients/google-cloud-pubsub/pom.xml

  </parent>
  <properties>
    <site.installationModule>google-cloud-pubsub</site.installationModule>
+    <opencensus.version>0.15.1</opencensus.version>


bogdandrutu · 2019-03-01T02:07:58Z

google-cloud-clients/google-cloud-pubsub/pom.xml

      <artifactId>grpc-google-iam-v1</artifactId>
      <scope>test</scope>
    </dependency>
+    <dependency>


We should not, only the final app should do this.

sduskis · 2019-03-01T02:05:28Z

...oud-clients/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/OpenCensusUtil.java

+import java.util.logging.Level;
+import java.util.logging.Logger;
+
+final class OpenCensusUtil {


Will we have to make this class public in the new scheme?

sduskis · 2019-03-01T02:06:22Z

...oud-clients/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/OpenCensusUtil.java

+  private static final TagContextBinarySerializer serializer =
+      Tags.getTagPropagationComponent().getBinarySerializer();
+
+  private static final TraceOptions SAMPLED = TraceOptions.builder().setIsSampled(true).build();


SAMPLED isn't used anymore. Please remove it.

…cloud-java into opencensus-cps

dinooliva · 2019-03-20T15:41:35Z

Thanks!

dinooliva requested a review from a team as a code owner December 18, 2018 20:16

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

chingor13 added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Dec 19, 2018

sduskis suggested changes Jan 25, 2019

View reviewed changes

sduskis added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 25, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 25, 2019

igorbernstein2 reviewed Jan 25, 2019

View reviewed changes

dinooliva changed the title ~~DO NOT MERGE: OpenCensus Support for Cloud Pub/Sub~~ OpenCensus Support for Cloud Pub/Sub Feb 5, 2019

JustinBeckwith added the 🚨 This issue needs some love. label Feb 7, 2019

yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Feb 7, 2019

sduskis suggested changes Feb 8, 2019

View reviewed changes

sduskis added status: blocked Resolving the issue is dependent on other work. kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed 🚨 This issue needs some love. labels Feb 11, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 12, 2019

sduskis reviewed Feb 12, 2019

View reviewed changes

sduskis requested a review from kolea2 February 14, 2019 23:34

Dino Oliva added 6 commits February 21, 2019 10:05

Adds OpenCensus context propagation to Publisher and Subscriber.

f093caf

Updates OpenCensus attribute keys so that they will be propagated by …

69af4b8

…CPS.

Addresses reviewer comments by fixing build files and using only defi…

8b37de8

…ned annotations.

Updates build dependencies and copyright date.

a951585

Fixes typo.

1e8fbce

Removes encoding of OpenCensus tags. Will re-enable once text encodin…

290fb81

…g spec has been finalized (census-instrumentation/opencensus-specs#65).

sduskis added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 28, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 28, 2019

sduskis mentioned this pull request Feb 28, 2019

Adding a pubsub publisher message transformer #4599

Closed

sduskis reviewed Mar 1, 2019

View reviewed changes

bogdandrutu reviewed Mar 1, 2019

View reviewed changes

Updates OpenCensus integration to use a generic MessageTransform.

4dc8425

sduskis reviewed Mar 7, 2019

View reviewed changes

Dino Oliva and others added 2 commits March 11, 2019 09:50

Removes now-unused private constant.

2f11262

Update pom.xml

6ea2f70

sduskis added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 18, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 18, 2019

Marking setTransform as BetaApi

67961f3

yoshi-automation added the 🚨 This issue needs some love. label Mar 19, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 19, 2019

Dino Oliva added 2 commits March 19, 2019 17:24

Fixes for formatting issues.

8135358

Merge branch 'opencensus-cps' of https://github.com/dinooliva/google-…

b2a41f3

…cloud-java into opencensus-cps

sduskis added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 20, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 20, 2019

sduskis approved these changes Mar 20, 2019

View reviewed changes

sduskis merged commit e3e812b into googleapis:master Mar 20, 2019

This was referenced Mar 20, 2019

Release google-cloud-java v0.84.0 #4710

Closed

Release google-cloud-java v0.84.0 #4713

Merged

OpenCensus Support for Cloud Pub/Sub #4240

OpenCensus Support for Cloud Pub/Sub #4240

Uh oh!

Conversation

dinooliva commented Dec 18, 2018

Uh oh!

sduskis commented Jan 10, 2019

Uh oh!

igorbernstein2 commented Jan 10, 2019

Uh oh!

sduskis left a comment

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!

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

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

igorbernstein2 Jan 25, 2019 •

edited

Loading

codecov bot commented Feb 14, 2019 •

edited

Loading