core, census: move census dependency out of grpc-core #6577

voidzcy · 2019-12-28T01:21:59Z

Major changes:

Created a grpc-census module:
- Moved CensusStatsModule.java and CensusTracingModule.java into grpc-census from grpc-core. CensusModuleTests.java is also moved. They now belong to io.grpc.census package.
- Moved DeprecatedCensusConstants.java into io.grpc.census.internal (is this necessary?) in grpc-census.
- Created CensusStatsAccessor.java and CensusTracingAccessor.java, which are used to create census ClientInterceptor and ServerStreamTracer.Factory.
- Everything in grpc-census are package private, except the accessor classes. They only publicly expose ClientInterceptor and ServerStreamTracer.Factory, no Census specific types are exposed.
grpc-core's integration of census is done at runtime by reflection.
Removed special APIs on AbstractManagedChannelImplBuilder and AbstractServerImplBuilder for overriding census module. They are only used for testing. Now we changed tests to apply Census ClientInterceptor and ServerStreamTracer.Factory just as normal interceptor/stream tracer factory. Test writer is responsible for taking care of the ordering concerns of interceptors and stream tracer factories.

…s into grpc-census package.

…ensusServerStreamTracerFactory to eliminate dependencies on method signature.

… tracer factory with custom census stats module.

…in core.

ejona86

Looks fair. I think we'll just want to reduce the API surface before merging.

census/build.gradle

ejona86 · 2020-01-02T22:39:52Z

census/src/main/java/io/grpc/census/CensusStatsModule.java

@@ -67,6 +68,7 @@
 * starts earlier than the ServerCall.  Therefore, only one tracer is created per stream/call and
 * it's the tracer that reports the summary to Census.
 */
+@VisibleForTesting


It seems this should be marked @Internal or similar? @VisibleForTesting does not really limit users.

Changed to @Internal.

CensusStatsModule and CensusTracingModule are now package-private.

census/src/test/java/io/grpc/census/CensusModulesTest.java

census/src/main/java/io/grpc/census/CensusStatsAccessor.java

ejona86 · 2020-01-02T22:49:16Z

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

+                      null,
+                      GrpcUtil.STOPWATCH_SUPPLIER,
+                      true,
+                      recordStartedRpcs,


Doesn't have to be now, but we'll probably want to re-think how this configuration in tests is done. Could be as simple as having a method on the builder that disables the census interceptors and then requiring they be added back manually by the test code.

Previously this is done with AbstractManagedChannelImplBuilder#overrideCensusStatsModule(CensusStatsModule) by injecting a CensusStatsModule with custom implementation for testing. Now, I changed it to AbstractManagedChannelImplBuilder#setCensusStatsInterceptor(ClientInterceptor). Same idea, directly use the injected interceptor to replace the default census interceptor.

census/src/main/java/io/grpc/census/CensusStatsModule.java

…_census_dep_out_of_core

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

census/src/main/java/io/grpc/census/CensusStatsAccessor.java

census/src/main/java/io/grpc/census/CensusTracingModule.java

ejona86 · 2020-01-08T22:38:50Z

census/src/main/java/io/grpc/census/CensusStatsAccessor.java

+  /**
+   * Returns a {@link ClientInterceptor} with custom stats implementation.
+   */
+  public static ClientInterceptor getClientInterceptor(CensusStatsModule censusStats) {


This method doesn't seem like it adds much. I'd expect to just make CensusStatsModule.getClientInterceptor() public instead of this. The benefit of a method like this is if we are able to hide CensusStatsModule, but this method isn't doing that.

But even better, it seems the only users of the Module construct it to just get the interceptor. Let's instead make this almost the same as the first getClientInterceptor method, just with more arguments. At that point we should make CensusStatsModule package-private.

Ditto for server-side.

ejona86 · 2020-01-08T22:43:50Z

census/src/main/java/io/grpc/census/CensusStatsAccessor.java

+ * Accessor for getting {@link ClientInterceptor} or {@link ServerStreamTracer.Factory} with
+ * default Census stats implementation.
+ */
+public final class CensusStatsAccessor {


Let's rename this to InternalCensusStatsAccessor and mark it @Internal. It looks like we'll need to do a resonable amount of work before we can use zero-arg (or almost zero) methods to construct the objects here. Once we get to zero (or almost zero) we can make something here public or use the package-private+public-inner-class trick.

ejona86 · 2020-01-08T22:57:47Z

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

   */
  @VisibleForTesting
-  protected final T overrideCensusStatsModule(CensusStatsModule censusStats) {
-    this.censusStatsOverride = censusStats;
+  protected final T setCensusStatsInterceptor(ClientInterceptor statsInterceptor) {


I think we need to consider just adding an interceptor like normal, instead of having a special method for it. We would no longer need testing's TestingAccessor.

Changed tests to apply census interceptor/stream tracer factory as normal ones. This eliminates the special methods on channel/server builder for overriding census interceptor/stream tracer factory. With this approach, we would still need accessors for disabling default census interceptor/stream tracer factory.

…private.

…nterceptor/tracer factory that were used for testing. Tests directly apply census interceptor/tracer factory as applying normal interceptor/tracer factory.

…file implementation for the specific test.

…sting tracer factory for interop test.

ejona86

That cleaned up quite a bit.

ejona86 · 2020-01-11T00:58:16Z

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

+                    recordRealTimeMetrics);
+      } catch (ClassNotFoundException | NoSuchMethodException | IllegalAccessException
+          | InvocationTargetException e) {
+        // Do nothing.


You may consider logging this at FINE/DEBUG level. Especially because it includes IllegalAccessException and InvocationTargetException. Ditto below and for server.

Sure. Done.

gfelbing · 2020-03-05T17:49:32Z

Hello @voidzcy ,

this broke our monitoring.

After updating from 1.24.1 to 1.27.2, the maven dependency 'io.grpc:grpc-core:1.27.2' did not include the new grpc-census artifact as transitive dependency and we had to include it explicitly.

While this was probably the intention of the PR, we were still quite surprised by this breaking change.

ejona86 · 2020-03-05T17:55:39Z

@gfelbing, if you were able to add grpc-census artifact and it work, then it wasn't a "breaking change." Any time when changing dependencies you may need to make sure they are correct. I'm sorry it surprised you, but there doesn't seem much more we can do about it, unless you have a suggestion for something that would have helped.

… census dependency out of grpc-core (grpc#6577) Decouples grpc-core with census, while still preserve the default integration of census in grpc-core. Users wishing to enable census needs to add grpc-census to their runtime classpath. - Created a grpc-census module: - Moved CensusStatsModule.java and CensusTracingModule.java into grpc-census from grpc-core. CensusModuleTests.java is also moved. They now belong to io.grpc.census package. Moved DeprecatedCensusConstants.java into io.grpc.census.internal (is this necessary?) in grpc-census. - Created CensusStatsAccessor.java and CensusTracingAccessor.java, which are used to create census ClientInterceptor and ServerStreamTracer.Factory. - Everything in grpc-census are package private, except the accessor classes. They only publicly expose ClientInterceptor and ServerStreamTracer.Factory, no Census specific types are exposed. - Use runtime reflection to load and apply census stats/tracing to channel/server builders, if grpc-census is found in runtime classpath. - Removed special APIs on AbstractManagedChannelImplBuilder and AbstractServerImplBuilder for overriding census module. They are only used for testing. Now we changed tests to apply Census ClientInterceptor and ServerStreamTracer.Factory just as normal interceptor/stream tracer factory. Test writer is responsible for taking care of the ordering concerns of interceptors and stream tracer factories.

voidzcy added 7 commits December 27, 2019 17:20

Created grpc-census package and moved census stats and tracing module…

1724783

…s into grpc-census package.

Replaced overrideCensusStats with setCensusClientInterceptor and setC…

dd11d99

…ensusServerStreamTracerFactory to eliminate dependencies on method signature.

Use reflection to access census modules in channel/server builder.

d6edad4

Add overloading methods for creating client interceptor/server stream…

192705c

… tracer factory with custom census stats module.

Expose some internal types for testing.

6d59e66

Remove census modules in grpc-core and use grpc-census for testing.

5f8bb19

Remove census dependencies from build files, delete census constants …

d9cfe78

…in core.

voidzcy force-pushed the refactor/move_census_dep_out_of_core branch 2 times, most recently from 322ff09 to b43535d Compare December 28, 2019 03:43

Clean up build files.

7ff29e7

voidzcy force-pushed the refactor/move_census_dep_out_of_core branch from b43535d to 7ff29e7 Compare December 28, 2019 03:53

voidzcy marked this pull request as ready for review December 30, 2019 05:21

voidzcy requested review from ejona86 and zhangkun83 December 30, 2019 22:04

ejona86 reviewed Jan 2, 2020

View reviewed changes

voidzcy added 4 commits January 3, 2020 11:18

Fix Javadoc configuration for census module.

9f368ab

Add @internal annotation for census stats/tracing modules.

30bb278

Add missing final for class definition.

d407590

Revert whitespace change.

49ad4d1

ejona86 reviewed Jan 3, 2020

View reviewed changes

census/src/main/java/io/grpc/census/CensusStatsModule.java Outdated Show resolved Hide resolved

Merge branch 'master' of github.com:grpc/grpc-java into refactor/move…

2a6f789

…_census_dep_out_of_core

ejona86 reviewed Jan 8, 2020

View reviewed changes

voidzcy added 4 commits January 8, 2020 16:27

Eliminate hard-coded arguments for creating Census module via accessors.

b9a61cd

Remove direct dependency on Census module classes, make them package-…

342474c

…private.

Remove special APIs on channel/server builder for overriding census i…

1d035d9

…nterceptor/tracer factory that were used for testing. Tests directly apply census interceptor/tracer factory as applying normal interceptor/tracer factory.

Minor styling fixes.

c549c8d

voidzcy force-pushed the refactor/move_census_dep_out_of_core branch from d2c7256 to c549c8d Compare January 9, 2020 21:28

Revert making ServerCallInfoImpl public. Instead, create a simple in-…

35ebeda

…file implementation for the specific test.

voidzcy force-pushed the refactor/move_census_dep_out_of_core branch from 89a56ad to 35ebeda Compare January 9, 2020 21:40

voidzcy added 5 commits January 9, 2020 13:44

Fix bazel dependencies for grpc-testing.

42084bf

Suppress error for no public classes.

03db456

Disable default census stats tracer factory on server side and use te…

0dff1ac

…sting tracer factory for interop test.

Add generic type for ServerCallInfo impl.

fc37246

Add grpc-census dependency for android-interop-testing.

fdf94bf

ejona86 approved these changes Jan 11, 2020

View reviewed changes

Add logging for failures in loading runtime methods.

ba6d40d

voidzcy closed this Jan 13, 2020

voidzcy reopened this Jan 13, 2020

voidzcy merged commit b7ccc0d into grpc:master Jan 13, 2020

This was referenced Jan 15, 2020

alts: do not exclude opencensus-api from google-oauth2's transitive dependencies #6607

Merged

alts: do not exclude opencensus-api from google-oauth2's transitive dependencies (1.27.x backport) #6609

Merged

This was referenced Jan 16, 2020

core: disable stats and tracing when OpenCensus impls aren't present #5520

Closed

There should be a way to disable OpenCensus stats and tracing #5510

Closed

sanjaypujare mentioned this pull request Mar 30, 2020

Envoy protos should not have a dependency on opencensus proto envoyproxy/envoy#10576

Closed

ejona86 mentioned this pull request Apr 20, 2020

Remove Census dependency from grpc-core #5076

Closed

voidzcy mentioned this pull request May 7, 2020

Unable to activate real-time metrics to OpenCensus #5624

Open

codefromthecrypt mentioned this pull request Oct 15, 2020

deprecate "grpc-trace-bin" openzipkin/brave#1178

Open

igorbernstein2 mentioned this pull request Nov 9, 2020

need to add grpc_census now that its split googleapis/java-bigtable#517

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jun 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core, census: move census dependency out of grpc-core #6577

core, census: move census dependency out of grpc-core #6577

voidzcy commented Dec 28, 2019 •

edited by sanjaypujare

Loading

ejona86 left a comment

ejona86 Jan 2, 2020

voidzcy Jan 3, 2020

voidzcy Jan 10, 2020

ejona86 Jan 2, 2020

voidzcy Jan 3, 2020

ejona86 Jan 8, 2020

ejona86 Jan 8, 2020

ejona86 Jan 8, 2020

voidzcy Jan 9, 2020

ejona86 left a comment

ejona86 Jan 11, 2020

voidzcy Jan 13, 2020

gfelbing commented Mar 5, 2020

ejona86 commented Mar 5, 2020

core, census: move census dependency out of grpc-core #6577

core, census: move census dependency out of grpc-core #6577

Conversation

voidzcy commented Dec 28, 2019 • edited by sanjaypujare Loading

ejona86 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

ejona86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfelbing commented Mar 5, 2020

ejona86 commented Mar 5, 2020

voidzcy commented Dec 28, 2019 •

edited by sanjaypujare

Loading