core: use new OpenCensus stats/tagging API. #3647

sebright · 2017-10-31T22:31:10Z

This commit updates gRPC core to use io.opencensus:opencensus-api and
io.opencensus:opencensus-contrib-grpc-metrics instead of
com.google.instrumentation:instrumentation-api for stats and tagging. The gRPC
Monitoring Service continues to use instrumentation-api.

The main changes affecting gRPC:

The StatsContextFactory is replaced by three objects, StatsRecorder, Tagger,
and TagContextBinarySerializer.
The StatsRecorder, Tagger, and TagContextBinarySerializer are never null,
but the objects are no-ops when the OpenCensus implementation is not
available.

This commit includes changes written by @songy23 and @sebright.

I added two TODOs for parts that I didn't know how to update. This PR is still using the snapshot of OpenCensus.

/cc @zhangkun83

grpc-kokoro · 2017-10-31T22:31:11Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

thelinuxfoundation · 2017-10-31T22:31:12Z

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

User @sebright isn't covered by a CLA. They will need to complete the form at https://identity.linuxfoundation.org/projects/cncf

Regards,
The Linux Foundation CLA GitHub bot

sebright · 2017-10-31T23:47:52Z

testing Linux Foundation CLA

sebright · 2017-10-31T23:58:07Z

testing Linux Foundation CLA

zhangkun83

Some small-lish comments. The Census API looks good.

Technically this is a breaking behavioral change, because gRPC now handles a different type of Context key. However, it's unlikely that any 3rd-party has done any work on the old Census API. So this should fine.

zhangkun83 · 2017-11-01T00:03:23Z

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

+      StatsRecorder statsRecorder =
+          this.statsRecorder != null ? this.statsRecorder : Stats.getStatsRecorder();
+      // // TODO: How do we check whether stats is enabled, now that the StatsRecorder is always
+      // // non-null? Uncommenting this line causes test failures.


Most tests pass in the fake implementations from StatsTestUtils, but Stats.getState() returns DISABLED. If you don't initialize CensusStatsModule, these tests will fail.

A possibly better approach, is to make CensusStatsModule public, and change the three-argument Census impl setter of the builder to overrideCensusStatsModule(CensusStatsModule). Tests will create CensusStatsModule out of the impls from StatsTestUtils, and here you can do:

if (statsEnabled) { CensusStatsModule censusStats = this.censusStatsOverride; if (censusStats == null && Stats.getStats() == StatsCollectionState.ENABLED) { CensusStatsModule censusStats = new CensusStatsModule(...); } if (censusStats != null) { tracerFactories.add(censusStats.getServerTracerFactory()); } }

@zhangkun83 I refactored the code for overriding the OpenCensus implementation. There are two parts that I'm not sure about.

CensusStatsModule no longer caches the StatsClientInterceptor and ServerTracerFactory. It creates a new object whenever getServerTracerFactory or getClientInterceptor is called. Is that correct?

I'm not completely sure I used the right values for recordStats, especially in CensusModulesTest.

zhangkun83 · 2017-11-01T00:10:40Z

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

-            new CensusStatsModule(statsFactory, GrpcUtil.STOPWATCH_SUPPLIER, true, recordStats);
-        tracerFactories.add(censusStats.getServerTracerFactory());
-      }
+      Tagger tagger = this.tagger != null ? this.tagger : Tags.getTagger();


Just curious, are the three components coupled? For example, does one Tagger implementation only works with a certain StatsRecorder, or can they mix and match?

The Tagger and TagContextBinarySerializer objects are coupled. The StatsRecorder is independent, except that it also uses the gRPC Context key that is used by the tagging classes.

songy23 · 2017-11-01T21:33:57Z

/cc @dinooliva @bogdandrutu

bogdandrutu

Please rebase.

bogdandrutu · 2017-11-02T01:00:17Z

build.gradle

-                opencensus_impl: 'io.opencensus:opencensus-impl:0.7.0',
+                opencensus_api: 'io.opencensus:opencensus-api:0.8.0',
+                opencensus_contrib_grpc_metrics: 'io.opencensus:opencensus-contrib-grpc-metrics:0.8.0',
+                opencensus_impl: 'io.opencensus:opencensus-impl:0.8.0',


Why do you depend on the implementation?

It looks like it is only needed for the interop tests:

grpc-java/interop-testing/build.gradle

Line 29 in 37a67bf

runtime libraries.opencensus_impl

sebright · 2017-11-02T01:22:55Z

I rebased. I didn't squash any commits, so there is still a commit to upgrade to OpenCensus 0.8.0, even though that change was made on master.

ejona86 · 2017-11-02T23:01:49Z

"io.opencensus.contrib.grpc.metrics.RpcMeasureConstants" ☹️ Normally "contrib" implies unsupported. It's sort of like: "Here's a bag of things others found useful, but we don't maintain them and their quality may vary."

ejona86 · 2017-11-02T23:06:50Z

Okay to test.

bogdandrutu · 2017-11-02T23:25:20Z

@ejona86 sorry for the wrong "contrib" name. We used it for "other" artifacts that are not in the core library. We will work in the future to maybe change that name. Thanks for the feedback, we are committed to keep backwards compatibility for "contrib" as well.

ejona86 · 2017-11-02T23:30:25Z

Jenkins, retest this please

zhangkun83

Overall LGTM. Some minor comments.

zhangkun83 · 2017-11-03T17:20:37Z

core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java

@@ -61,15 +67,56 @@
        }
      };

-  private static final StatsContextFactory DUMMY_STATS_FACTORY =
-      new StatsContextFactory() {
+  private static final Tagger DUMMY_TAGGER =


It probably won't hurt if this test uses the fake implementations from StatsTestUtils. Then you can save a lot of lines of the dummy implementations.

zhangkun83 · 2017-11-03T17:20:52Z

core/src/test/java/io/grpc/internal/AbstractServerImplBuilderTest.java

-  private static final StatsContextFactory DUMMY_STATS_FACTORY =
-      new StatsContextFactory() {
+
+  private static final Tagger DUMMY_TAGGER =


zhangkun83 · 2017-11-03T17:30:08Z

testing/src/main/java/io/grpc/internal/TestingAccessor.java

-  public static void setStatsContextFactory(
-      AbstractManagedChannelImplBuilder<?> builder, StatsContextFactory factory) {
-    builder.statsContextFactory(factory);
+  public static void setStatsImplementation(


These two methods should take the form of the corresponding methods in the builders.

zhangkun83 · 2017-11-03T17:38:28Z

testing/src/main/java/io/grpc/internal/testing/StatsTestUtils.java

@@ -93,39 +107,29 @@ public long getMetricAsLongOrFail(MeasurementDescriptor metricName) {
  }

  /**
-   * This tag will be propagated by {@link FakeStatsContextFactory} on the wire.
+   * This tag will be propagated by {@link FakeTagger} on the wire.
   */
  public static final TagKey EXTRA_TAG = TagKey.create("/rpc/test/extratag");

  private static final String EXTRA_TAG_HEADER_VALUE_PREFIX = "extratag:";
  private static final String NO_EXTRA_TAG_HEADER_VALUE_PREFIX = "noextratag";


I wonder if this and its related logic can be removed.
It was needed because the old StatsContext didn't provide a way to tell if it's empty, thus the metadata was always sent. Now that you have implemented the optimization to skip sending metdata if TagContext is empty, we should no longer need the metadata form that represents "no tag".

I'm not sure I understand. I didn't try to change any of the logic for serializing and deserializing tags.

I meant if you remove this field and the if-branches that references this field, tests should not fail. Because of the following if you added to CensusStatsModule, those branches will never be executed now.

if (!module.tagger.empty().equals(parentCtx)) { headers.put(module.statsHeader, parentCtx); }

So the optimization was added before this PR? This PR just changes the comparison to use tagger.empty() instead of statsCtxFactory.getDefault().

I removed NO_EXTRA_TAG_HEADER_VALUE_PREFIX and the branches that used it, and the tests passed. It is the last commit. Shouldn't serialize still handle the case where the TagContext isn't empty, but it doesn't contain EXTRA_TAG?

You are right ... the optimization was added before.

To keep it simple, FakeTagContextBinarySerializer is supposed to only handle EXTRA_TAG, and tests are supposed to only add EXTRA_TAG. Now fromByteArray() will fail if the metadata contains anything other than EXTRA_TAG. toByteArray() will fail too, by throwing NullPointerException -- it's probably better to make it throw UnsupportedOperationException if extraTagValue==null.

I made toByteArray throw an UnsupportedOperationException.

zhangkun83 · 2017-11-03T17:44:04Z

@zhangkun83 I refactored the code for overriding the OpenCensus implementation. There are two parts that I'm not sure about.

CensusStatsModule no longer caches the StatsClientInterceptor and ServerTracerFactory. It creates a new object whenever getServerTracerFactory or getClientInterceptor is called. Is that correct?

Yes it's correct. The caching wasn't necessary because it's only called per channel/server.

I'm not completely sure I used the right values for recordStats, especially in CensusModulesTest.

That sounds right. recordStats should be true in most of the tests.

@songy23

This commit updates gRPC core to use io.opencensus:opencensus-api and io.opencensus:opencensus-contrib-grpc-metrics instead of com.google.instrumentation:instrumentation-api for stats and tagging. The gRPC Monitoring Service continues to use instrumentation-api. The main changes affecting gRPC: - The StatsContextFactory is replaced by three objects, StatsRecorder, Tagger, and TagContextBinarySerializer. - The StatsRecorder, Tagger, and TagContextBinarySerializer are never null, but the objects are no-ops when the OpenCensus implementation is not available. This commit includes changes written by @songy23 and @sebright.

gRPC shouldn't use the state to determine whether to initialize the CensusStatsModule, because the state can be changed at runtime.

This commit adds a method to set the CensusStatsModule on AbstractServerImplBuilder and AbstractManagedChannelImplBuilder. It is simpler to set the whole CensusStatsModule than pass in all necessary OpenCensus implementation objects.

zhangkun83 · 2017-11-06T22:58:40Z

LGTM. Please merge master onto your branch and resolve the conflict.

sebright · 2017-11-07T04:49:14Z

Thanks. I merged my branch.

ejona86 · 2017-11-07T18:23:36Z

okay to test

sebright · 2017-11-07T20:25:45Z

Thanks!

dinooliva · 2017-11-07T22:27:12Z

Excellent!

@songy23

This commit updates gRPC core to use io.opencensus:opencensus-api and io.opencensus:opencensus-contrib-grpc-metrics instead of com.google.instrumentation:instrumentation-api for stats and tagging. The gRPC Monitoring Service continues to use instrumentation-api. The main changes affecting gRPC: - The StatsContextFactory is replaced by three objects, StatsRecorder, Tagger, and TagContextBinarySerializer. - The StatsRecorder, Tagger, and TagContextBinarySerializer are never null, but the objects are no-ops when the OpenCensus implementation is not available. This commit includes changes written by @songy23 and @sebright.

zhangkun83 self-assigned this Oct 31, 2017

zhangkun83 reviewed Nov 1, 2017

View reviewed changes

sebright mentioned this pull request Nov 1, 2017

Test new stats and tagging API with gRPC. census-instrumentation/opencensus-java#720

Closed

sebright force-pushed the update-opencensus branch from ef2fe43 to 404abba Compare November 1, 2017 20:25

bogdandrutu reviewed Nov 2, 2017

View reviewed changes

sebright force-pushed the update-opencensus branch from decfbb0 to 95365c5 Compare November 2, 2017 01:16

sebright force-pushed the update-opencensus branch from 95365c5 to 1b464f6 Compare November 2, 2017 18:34

ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 2, 2017

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 Nov 2, 2017

ejona86 added this to the 1.8 milestone Nov 2, 2017

ejona86 added the TODO:release blocker Issue/PR is important enough to delay the release. Removed after release issues resolved label Nov 2, 2017

zhangkun83 reviewed Nov 3, 2017

View reviewed changes

sebright added 5 commits November 3, 2017 11:56

Update OpenCensus to 0.8.0 release.

4e01775

Remove TODOs about checking stats collection state.

611878c

gRPC shouldn't use the state to determine whether to initialize the CensusStatsModule, because the state can be changed at runtime.

Remove dummy OpenCensus classes, and use fake classes instead.

ca7ad9a

sebright force-pushed the update-opencensus branch from 1b464f6 to ca7ad9a Compare November 3, 2017 19:28

Refactor TestingAccessor.setStatsImplementation methods.

7fe19a2

sebright added 2 commits November 3, 2017 13:09

Remove NO_EXTRA_TAG_HEADER_VALUE_PREFIX.

45419c2

Throw exception when FakeTagContext doesn't contain expected tag.

403c6e3

Merge remote-tracking branch 'origin/master' into update-opencensus

4f0d7c5

ejona86 added kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary labels Nov 7, 2017

sebright mentioned this pull request Nov 7, 2017

Change gRPC to use the newly released tags/stats APIs census-instrumentation/opencensus-java#601

Closed

zhangkun83 approved these changes Nov 7, 2017

View reviewed changes

ejona86 removed kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run labels Nov 7, 2017

zhangkun83 merged commit ef2ec94 into grpc:master Nov 7, 2017

sebright deleted the update-opencensus branch November 7, 2017 20:25

ejona86 added the TODO:backport PR needs to be backported. Removed after backport complete label Nov 7, 2017

sebright mentioned this pull request Nov 10, 2017

Backport OpenCensus update (#3647) onto 1.8 release branch #3712

Closed

zhangkun83 removed TODO:release blocker Issue/PR is important enough to delay the release. Removed after release issues resolved TODO:backport PR needs to be backported. Removed after backport complete labels Nov 10, 2017

lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019

core: use new OpenCensus stats/tagging API. #3647

core: use new OpenCensus stats/tagging API. #3647

Conversation

sebright commented Oct 31, 2017

grpc-kokoro commented Oct 31, 2017

thelinuxfoundation commented Oct 31, 2017

sebright commented Oct 31, 2017

sebright commented Oct 31, 2017

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

songy23 commented Nov 1, 2017

bogdandrutu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebright commented Nov 2, 2017

ejona86 commented Nov 2, 2017

ejona86 commented Nov 2, 2017

bogdandrutu commented Nov 2, 2017

ejona86 commented Nov 2, 2017 • edited Loading

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

zhangkun83 commented Nov 3, 2017

zhangkun83 commented Nov 6, 2017

sebright commented Nov 7, 2017

ejona86 commented Nov 7, 2017

sebright commented Nov 7, 2017

dinooliva commented Nov 7, 2017

ejona86 commented Nov 2, 2017 •

edited

Loading