core: Update OpenCensus version. #4494

songy23 · 2018-05-22T23:25:07Z

Related to census-instrumentation/opencensus-java#1207.

OpenCensus has updated the RPC constants in Specs recently, and in OpenCensus-Java these new constants are added since v0.13.0. This PR updates gRPC to use the latest OpenCensus version, ~~and record stats against the new RPC constants.~~

~~Note that gRPC also needs to record stats against the deprecated constants, to keep backwards compatibility.~~

/cc @bogdandrutu @dinooliva @sebright

sebright

Do we also need to update filtering for serialized tags?

sebright · 2018-05-23T23:03:51Z

interop-testing/src/main/java/io/grpc/testing/integration/AbstractInteropTest.java

@@ -1891,22 +1897,36 @@ private void assertServerStatsTrace(String method, Status.Code code,
    }
  }

-  private static void checkStartTags(MetricsRecord record, String methodName) {
+  private static void checkStartTags(MetricsRecord record, String methodName, boolean isClient) {


Suggestion: Since there are many calls to checkStartTags and checkEndTags that are far from the definition, it might help to use an enum with CLIENT and SERVER values instead of a boolean.

Make sense to me, though there's other method in this class that already used a similar boolean check...

songy23

We don't have filtering for serialized tags before - I'm not sure if this would be a problem after we added the new tags (client_method, server_method, etc.). @zhangkun83 WDYT?

songy23 · 2018-05-24T00:05:04Z

interop-testing/src/main/java/io/grpc/testing/integration/AbstractInteropTest.java

@@ -1891,22 +1897,36 @@ private void assertServerStatsTrace(String method, Status.Code code,
    }
  }

-  private static void checkStartTags(MetricsRecord record, String methodName) {
+  private static void checkStartTags(MetricsRecord record, String methodName, boolean isClient) {


Make sense to me, though there's other method in this class that already used a similar boolean check...

zhangkun83

We don't have filtering for serialized tags before - I'm not sure if this would be a problem after we added the new tags (client_method, server_method, etc.). @zhangkun83 WDYT?

Sorry I don't understand your question. Can you elaborate the problem?

zhangkun83 · 2018-05-24T16:03:01Z

core/src/main/java/io/grpc/internal/CensusStatsModule.java

@@ -329,7 +330,9 @@ public void outboundMessage(int seqNo) {
      this.parentCtx = checkNotNull(parentCtx);
      this.startCtx =
          module.tagger.toBuilder(parentCtx)
-          .put(RpcMeasureConstants.RPC_METHOD, TagValue.create(fullMethodName)).build();
+          .put(RpcMeasureConstants.RPC_METHOD, TagValue.create(fullMethodName))
+          .put(RpcMeasureConstants.GRPC_CLIENT_METHOD, TagValue.create(fullMethodName))


You probably want to create the TagValue once and re-use it.

zhangkun83 · 2018-05-24T16:03:36Z

core/src/main/java/io/grpc/internal/CensusStatsModule.java

@@ -410,6 +424,9 @@ void callEnded(Status status) {
              .tagger
              .toBuilder(startCtx)
              .put(RpcMeasureConstants.RPC_STATUS, TagValue.create(status.getCode().toString()))
+              .put(
+                  RpcMeasureConstants.GRPC_CLIENT_STATUS,
+                  TagValue.create(status.getCode().toString()))


zhangkun83 · 2018-05-24T16:03:57Z

core/src/main/java/io/grpc/internal/CensusStatsModule.java

@@ -609,6 +634,9 @@ public void streamClosed(Status status) {
              .tagger
              .toBuilder(parentCtx)
              .put(RpcMeasureConstants.RPC_STATUS, TagValue.create(status.getCode().toString()))
+              .put(
+                  RpcMeasureConstants.GRPC_SERVER_STATUS,
+                  TagValue.create(status.getCode().toString()))


zhangkun83 · 2018-05-24T16:08:58Z

core/src/main/java/io/grpc/internal/CensusTracingModule.java

@@ -211,9 +211,10 @@ private static EndSpanOptions createEndSpanOptions(
  }

  private static void recordNetworkEvent(
-      Span span, NetworkEvent.Type type,
+      Span span, io.opencensus.trace.NetworkEvent.Type type,


Is it not the time yet to switch to the new class MessageEvent?

I would prefer to do that in a separate PR since it's unrelated to RPC constants. Also, I assume we will still need to record for both NetworkEvent and MessageEvent to be backwards compatible? @bogdandrutu @HailongWen

The API converts between the two classes, so only one should be recorded. I think that the only reason to not migrate to MessageEvent is to keep compatibility with older versions of opencensus-java.

@sebright, do you mean old versions of opencensus-java implementations? So presumably, the new Census impl will treat NetworkEvent and MessageEvent as equivalent, then gRPC can switch to MessageEvent after all your customers has upgraded to the new impl.

@sebright, do you mean old versions of opencensus-java implementations?

I meant old versions of opencensus-api. I don't think that the version of the OpenCensus implementation should matter to gRPC, as long as it is also compatible with the version of opencensus-api that gRPC uses.

The opencensus-java implementations still only reference NetworkEvent, and the API does the conversion.

zhangkun83 · 2018-05-24T16:41:14Z

core/src/main/java/io/grpc/internal/CensusStatsModule.java

@@ -389,6 +392,7 @@ void callEnded(Status status) {
        tracer = BLANK_CLIENT_TRACER;
      }
      MeasureMap measureMap = module.statsRecorder.newMeasureMap()
+          // TODO(songya): remove the deprecated measure constants once they are completed removed.


How is the migration handled on the consumer side in the Census implementation? For not to break the interoperability between gRPC and Census inside Google, it seems necessary for Census to read both the old and the new measurements, giving the new ones precedence. If that's the case, we could just remove the old measurements here.

Unfortunately, inside Google the new measurements are not completely consistent with Census Java, and only the old measurements will be read at this moment. Even for open source, users could still export and read the old measurements so I think it's important to continue recording stats for the old ones.

There are some ongoing discussion about making the new measurements consistent with Census Java. /cc @dinooliva

The artifact providing the gRPC views has separate methods for registering the old and new views, so users can choose when to upgrade them. It might be easier for users to upgrade if they could get data for both sets of views at the same time.

Is it possible to first change Census impls to convert the old measurements to the new ones, then switch gRPC to record the new measurements after all your customers have switched to the new impl and the new measurements? Then the migration of the measurements can go in line with the migration from NetworkEvent to MessageEvent (see my comment on that). Plus the gRPC code (esp. the tests) would be much cleaner if it ever has to deal with one API.

I'm not sure, but I thought that we wanted users to explicitly switch to the new views, since the change isn't transparent like the switch to MessageEvent. For example, the stats may be exported to a service that uses the view or measure names.

/cc @bogdandrutu

Yes, like @sebright said, OpenCensus users could be exporting the old RPC constants to other backends and using them for alerting rules. Since the new and old RPC constants have different names, they will be different metrics on other monitoring systems (Stackdriver, Prometheus). We would like to allow users to explicitly choose old or new RPC constants and update their monitoring accordingly, as in our APIs (registerAllGrpcViews vs registerAllViews). So I think it's import to have stats for both of them.

On top of that, as I mentioned in the previous comments, currently it's particularly difficult to map the old RPC constants to the new ones inside Google. For example, there are no *_count measures in the new constants.

I see you don't have the "started count" measurements in the new constants, but it is a required metric inside Google.

We removed started_count, finished_count and error_count in the new specs. There are some rationale behind why error_count and finished_count gets removed, but I cannot remember why we removed started_count.

@bogdandrutu @Ramonza should we consider putting back the started_count measure since it's required?

I added *_started_rpcs constants to OpenCensus v0.14.0, and upgraded the version here. Now the started_rpcs are recorded. PTAL at the two latest commits.

songy23

Since we're adding more tags to the context, @sebright proposed maybe we need filtering on serialized tags when serializing contexts. We didn't do this before and wondered if the additional tags could be a problem.

songy23 · 2018-05-24T17:12:16Z

core/src/main/java/io/grpc/internal/CensusStatsModule.java

@@ -329,7 +330,9 @@ public void outboundMessage(int seqNo) {
      this.parentCtx = checkNotNull(parentCtx);
      this.startCtx =
          module.tagger.toBuilder(parentCtx)
-          .put(RpcMeasureConstants.RPC_METHOD, TagValue.create(fullMethodName)).build();
+          .put(RpcMeasureConstants.RPC_METHOD, TagValue.create(fullMethodName))
+          .put(RpcMeasureConstants.GRPC_CLIENT_METHOD, TagValue.create(fullMethodName))


songy23 · 2018-05-24T17:24:26Z

core/src/main/java/io/grpc/internal/CensusStatsModule.java

@@ -389,6 +392,7 @@ void callEnded(Status status) {
        tracer = BLANK_CLIENT_TRACER;
      }
      MeasureMap measureMap = module.statsRecorder.newMeasureMap()
+          // TODO(songya): remove the deprecated measure constants once they are completed removed.


Unfortunately, inside Google the new measurements are not completely consistent with Census Java, and only the old measurements will be read at this moment. Even for open source, users could still export and read the old measurements so I think it's important to continue recording stats for the old ones.

There are some ongoing discussion about making the new measurements consistent with Census Java. /cc @dinooliva

songy23 · 2018-05-24T17:24:34Z

core/src/main/java/io/grpc/internal/CensusStatsModule.java

@@ -410,6 +424,9 @@ void callEnded(Status status) {
              .tagger
              .toBuilder(startCtx)
              .put(RpcMeasureConstants.RPC_STATUS, TagValue.create(status.getCode().toString()))
+              .put(
+                  RpcMeasureConstants.GRPC_CLIENT_STATUS,
+                  TagValue.create(status.getCode().toString()))


songy23 · 2018-05-24T17:24:41Z

core/src/main/java/io/grpc/internal/CensusStatsModule.java

@@ -609,6 +634,9 @@ public void streamClosed(Status status) {
              .tagger
              .toBuilder(parentCtx)
              .put(RpcMeasureConstants.RPC_STATUS, TagValue.create(status.getCode().toString()))
+              .put(
+                  RpcMeasureConstants.GRPC_SERVER_STATUS,
+                  TagValue.create(status.getCode().toString()))


songy23 · 2018-05-24T17:26:48Z

core/src/main/java/io/grpc/internal/CensusTracingModule.java

@@ -211,9 +211,10 @@ private static EndSpanOptions createEndSpanOptions(
  }

  private static void recordNetworkEvent(
-      Span span, NetworkEvent.Type type,
+      Span span, io.opencensus.trace.NetworkEvent.Type type,


I would prefer to do that in a separate PR since it's unrelated to RPC constants. Also, I assume we will still need to record for both NetworkEvent and MessageEvent to be backwards compatible? @bogdandrutu @HailongWen

songy23 · 2018-05-24T20:33:09Z

Just found I forgot to update repositories.bzl. @ericgribkoff could you please run the kokoro build again?

zhangkun83 · 2018-05-25T23:05:38Z

Since we're adding more tags to the context, @sebright proposed maybe we need filtering on serialized tags when serializing contexts. We didn't do this before and wondered if the additional tags could be a problem.

Oh, I thought the method name and status tags were not propagated on the via in the first place. Is it not the case?

songy23

Oh, I thought the method name and status tags were not propagated on the via in the first place. Is it not the case?

Server-method, server-status and client-status are non-propagating. The only thing we're not sure is client-method. From our interop testing it looks like client-method will be propagated to server.

songy23 · 2018-05-29T15:03:51Z

core/src/main/java/io/grpc/internal/CensusStatsModule.java

@@ -389,6 +392,7 @@ void callEnded(Status status) {
        tracer = BLANK_CLIENT_TRACER;
      }
      MeasureMap measureMap = module.statsRecorder.newMeasureMap()
+          // TODO(songya): remove the deprecated measure constants once they are completed removed.


Yes, like @sebright said, OpenCensus users could be exporting the old RPC constants to other backends and using them for alerting rules. Since the new and old RPC constants have different names, they will be different metrics on other monitoring systems (Stackdriver, Prometheus). We would like to allow users to explicitly choose old or new RPC constants and update their monitoring accordingly, as in our APIs (registerAllGrpcViews vs registerAllViews). So I think it's import to have stats for both of them.

songy23 · 2018-05-29T15:11:02Z

Besides, it looks like the failed Kokoro Macos build is because the testing machine cannot connect to gradle host:

./gradlew clean -PtargetArch=x86_64 -Pcheckstyle.ignoreFailures=false -PfailOnWarnings=true -PerrorProne=true -Dorg.gradle.parallel=true

Downloading https://services.gradle.org/distributions/gradle-4.7-bin.zip

Exception in thread "main" java.net.NoRouteToHostException: No route to host (Host unreachable)
...

zhangkun83 · 2018-05-29T23:59:30Z

Server-method, server-status and client-status are non-propagating. The only thing we're not sure is client-method. From our interop testing it looks like client-method will be propagated to server.

What's the benefit of propagating client-method? Presumably it would be the same as server-method.

songy23 · 2018-05-30T00:39:15Z

What's the benefit of propagating client-method? Presumably it would be the same as server-method.

Oh sorry I got this wrong at first. Client-method is actually non-propagating, but server would have method in the context. From Dino's comments in census-ecosystem/opencensus-experiments#18 (comment):

The grpc client doesn't need to propagate the method tag but the grpc server does need to add it to the context where the rpc is being processed so there should be a method tag in scope on the server side.

Also, if the grpc server subsequently makes a call as a grpc client, then the method tag will be propagated since it is in scope (I don't believe gRPC has any logic for explicitly removing the method tag).

And we're wondering if ServerTracerFactory.filterContext() needs to be updated after using the new TagKeys.

zhangkun83 · 2018-05-30T23:48:45Z

Oh sorry I got this wrong at first. Client-method is actually non-propagating, but server would have method in the context. From Dino's comments in census-ecosystem/opencensus-experiments#18 (comment):

The grpc client doesn't need to propagate the method tag but the grpc server does need to add it to the context where the rpc is being processed so there should be a method tag in scope on the server side.

Also, if the grpc server subsequently makes a call as a grpc client, then the method tag will be propagated since it is in scope (I don't believe gRPC has any logic for explicitly removing the method tag).

And we're wondering if ServerTracerFactory.filterContext() needs to be updated after using the new TagKeys.

gRPC puts RPC_METHOD (also GRPC_SERVER_METHOD as added in this PR) in the parentCtx in CensusStatsModule.ServerTracerFactory.newServerStreamTracer. This is already doing what @dinooliva was describing. filterContext() doesn't need to be changed.

songy23 · 2018-06-04T22:45:20Z

#4530 has updated gRPC to use MessageEvent instead of NetworkEvent, and has been submitted. Rebased against that PR (upgrade from 0.12.3 to 0.13.2).

Please hold off this PR until I add started_rpcs to OC Java (census-instrumentation/opencensus-java#1230) and make a 0.14.0 release. I'll update this PR after that.

sebright · 2018-06-05T18:08:59Z

LGTM

Remove SuppressWarnings from other classes.

songy23 · 2018-11-09T18:21:39Z

So sorry I didn't update this thread for a long time. The most recent benchmarks still showed performance regression if we were to record both the new and old constants. For now I remove the new constants from this PR and just update the OpenCensus version (and fix deprecation warnings). Please help me submit this PR first to avoid blocking census-instrumentation/opencensus-java#1563. @zhangkun83

Moving forward about the new constants, I think at some point we should just switch to the new ones and completely get rid of the old constants, both internally and in open source. That will be a breaking change, but even with our initial plan (which is to record both new and old constants for some time and then remove the old constants) there will be a point when we need to make the same breaking change.

zhangkun83 · 2018-11-09T23:26:46Z

interop-testing/src/main/java/io/grpc/testing/integration/AbstractInteropTest.java

@@ -1918,22 +1926,27 @@ private void assertServerStatsTrace(String method, Status.Code code,
    }
  }

-  private static void checkStartTags(MetricsRecord record, String methodName) {
+  private static void checkStartTags(MetricsRecord record, String methodName, Type type) {


The type is used, can you remove it?

Correct, removed.

zhangkun83 · 2018-11-09T23:26:52Z

interop-testing/src/main/java/io/grpc/testing/integration/AbstractInteropTest.java

  }

  private static void checkEndTags(
-      MetricsRecord record, String methodName, Status.Code status) {
+      MetricsRecord record, String methodName, Status.Code status, Type type) {


zhangkun83

LGTM

songy23 · 2018-11-10T01:07:26Z

Thanks!

songy23 force-pushed the opencensus-rpc-constants branch 4 times, most recently from 5be264f to 636d393 Compare May 23, 2018 16:31

sebright reviewed May 23, 2018

View reviewed changes

songy23 commented May 24, 2018

View reviewed changes

zhangkun83 reviewed May 24, 2018

View reviewed changes

songy23 commented May 24, 2018

View reviewed changes

songy23 force-pushed the opencensus-rpc-constants branch from ea3d425 to 20c37be Compare May 24, 2018 17:38

ericgribkoff added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 24, 2018

kokoro-team removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 24, 2018

ericgribkoff mentioned this pull request May 24, 2018

build.gradle: update to latest opencensus #4453

Closed

ericgribkoff added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 24, 2018

kokoro-team removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 24, 2018

ericgribkoff added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 25, 2018

songy23 commented May 29, 2018

View reviewed changes

ericgribkoff added kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary and removed kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run labels May 29, 2018

kokoro-team removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label May 29, 2018

songy23 mentioned this pull request Jun 4, 2018

Add started_rpcs measures and views to RPC constants. census-instrumentation/opencensus-java#1230

Merged

songy23 force-pushed the opencensus-rpc-constants branch from 364672e to 5d56f59 Compare June 4, 2018 22:45

songy23 force-pushed the opencensus-rpc-constants branch from dfbe60e to 243962e Compare June 4, 2018 23:43

ericgribkoff added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Sep 1, 2018

kokoro-team removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Sep 1, 2018

songy23 added 14 commits November 9, 2018 09:51

Update OpenCensus version.

45be3ca

Record stats for new RPC measure constants.

5c3cdfb

Fix tests related to RPC constants.

b1711d0

Work around deprecation warning.

769b22b

Use an enum instead of boolean for a test helper method.

948f81a

Extract a few common tag values.

060dff8

Update OpenCensus version for Bazel.

1903e0e

Remove one unnecessary SuppressWarnings(deprecation).

1b60c83

Upgrade OpenCensus version to 0.14.0

6275d31

Record *_started_rpcs measurements.

2a2e16d

Put deprecated Census constants in a holder class.

b214f77

Remove SuppressWarnings from other classes.

Make the constructor of DeprecatedCensusConstants private.

bd052a4

Update to latest oc version.

cbde7a6

Temporarily remove the new constants.

20f8de3

songy23 force-pushed the opencensus-rpc-constants branch from f10a2e6 to 20f8de3 Compare November 9, 2018 18:15

songy23 changed the title ~~core: Update OpenCensus version and record against the new RPC constants.~~ core: Update OpenCensus version. Nov 9, 2018

Suppress one more deprecation warning.

2fea929

songy23 force-pushed the opencensus-rpc-constants branch from d903c19 to 2fea929 Compare November 9, 2018 19:14

zhangkun83 reviewed Nov 9, 2018

View reviewed changes

Remove an unused enum.

d1c9dff

zhangkun83 approved these changes Nov 10, 2018

View reviewed changes

zhangkun83 added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Nov 10, 2018

kokoro-team removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Nov 10, 2018

zhangkun83 merged commit 09b13fe into grpc:master Nov 10, 2018

songy23 deleted the opencensus-rpc-constants branch November 10, 2018 01:07

lock bot locked as resolved and limited conversation to collaborators Feb 8, 2019

core: Update OpenCensus version. #4494

core: Update OpenCensus version. #4494

Conversation

songy23 commented May 22, 2018 • edited by zhangkun83 Loading

sebright left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

songy23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangkun83 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

songy23 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

songy23 commented May 24, 2018

zhangkun83 commented May 25, 2018

songy23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

songy23 commented May 29, 2018

zhangkun83 commented May 29, 2018

songy23 commented May 30, 2018

zhangkun83 commented May 30, 2018

songy23 commented Jun 4, 2018 • edited Loading

sebright commented Jun 5, 2018

songy23 commented Nov 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangkun83 left a comment

Choose a reason for hiding this comment

songy23 commented Nov 10, 2018

songy23 commented May 22, 2018 •

edited by zhangkun83

Loading

songy23 left a comment •

edited

Loading

songy23 commented Jun 4, 2018 •

edited

Loading