Grpc-java: There should be a way to disable OpenCensus stats and tracing

[njhill]

Currently, out-of-the-box client and server config includes a very expensive "no-op" in the form of the enabled-by-default opencensus tracing and stats interceptors. This is the case even if no implementation library is on the classpath, and disabling them is difficult/discouraged/impossible (without reflection) depending on the transport.

Benchmark                         (direct)  (statsAndTracing)  (transport)  Mode  Cnt      Score      Error  Units
TransportBenchmark.unaryCall1024      true               true    INPROCESS  avgt   10   3313.595 ±  107.360  ns/op
TransportBenchmark.unaryCall1024      true              false    INPROCESS  avgt   10   2142.610 ±   99.678  ns/op
TransportBenchmark.unaryCall1024      true               true  NETTY_LOCAL  avgt   10  64780.414 ± 1564.422  ns/op
TransportBenchmark.unaryCall1024      true              false  NETTY_LOCAL  avgt   10  60969.867 ± 1447.113  ns/op
TransportBenchmark.unaryCall1024      true               true        NETTY  avgt   10  85561.145 ± 3765.635  ns/op
TransportBenchmark.unaryCall1024      true              false        NETTY  avgt   10  81556.086 ± 2136.174  ns/op
TransportBenchmark.unaryCall1024      true               true  NETTY_EPOLL  avgt   10  79758.331 ± 3005.046  ns/op
TransportBenchmark.unaryCall1024      true              false  NETTY_EPOLL  avgt   10  75667.486 ± 2734.508  ns/op
TransportBenchmark.unaryCall1024      true               true       OKHTTP  avgt   10  74714.350 ± 2848.833  ns/op
TransportBenchmark.unaryCall1024      true              false       OKHTTP  avgt   10  70527.535 ± 2945.292  ns/op

There's also significantly more garbage produced when enabled.

It would be great to enable these only when the impl library is detected, however:

• There are circumstances when it might be there but not used
• It may be the case that only one of stats or tracing is being used
• Even if it is being used, instrumentation overhead of this magnitude may be considered undesirable (and these numbers are just the api plumbing without any additional cost from the impl itself)

Could these be changed to be disabled by default? I'm unsure why such settings are considered internal when they pertain to external functionality. There's not much danger of anyone "accidentally" leaving it disabled since it would be immediately apparent that the stats aren't working but the converse is of course true and I would assume most common.

[dapengzhang0]

There is no public API to disable it because there was no strong demand from users. However, there is a hack: enabling retry will disable census as a side effect for the current version of grpc library (Note: This behavior is subject to change in future releases).

[njhill]

Thanks @dapengzhang0

There is no public API to disable it because there was no strong demand from users.

Strong demand for the associated functionality or strong demand to disable these flags? If the former wouldn't that be all the more reason for them to be off by default? and if the latter I expect that's because most aren't aware of them or the fact a free performance boost is available by switching them off :)

there is a hack: enabling retry will disable census as a side effect for the current version of grpc library

This only applies to client-side, there's no way to do the equivalent on the server side. Interestingly if you also don't want retry then channelBuilder.enableRetry().disableRetry() will work too :) again only for client side though. If you are lucky enough to be using netty there are the InternalNetty(Channel|Server)Builder classes, but these also come with similar warning/discouragement.

[zhangkun83]

As we revisit the decision to include the OpenCensus into grpc-core, we regret it. That's why we don't expose public API to disable it. We are considering moving that of grpc-core and make its own artifact grpc-census. There may be two ways to approach it:

1. Let the core find classes of grpc-census and load then if available. The user controls whether to have stats/tracing by having or not having grpc-census in their class path.
2. Ditch dynamic loading, and have a utility class in grpc-census to install interceptors/tracers on the channel/server builders.

Option 1 allows current stats/tracing-dependent users to migrate without any code change. Option 2 is cleaner as it gives the user the maximum control, while it requires code changes to migrate. I personally prefer Option 2 but I am not sure whether the churn brought to users is acceptable. @ejona86 may want to chime in.

[njhill]

Thanks @zhangkun83. My vote would be for both! i.e. have the default settings determined by presence of the grpc-census classes, but also provide a means of explicit/finer-grained control. Wouldn't that give the best of both worlds? i.e. existing census users could upgrade without requiring any code/config changes while those who don't use it would benefit from it being turned off automatically.

Maybe a third option to consider at some point (also not mutually exclusive) is investigating the reason for the overhead; maybe there's some low-hanging optimizations which could cut that down.

[dapengzhang0]

@zhangkun83 I think Option 1 might require current stats/tracing-dependent users to migrate with dependency change.

[sanjaypujare]

I agree with @dapengzhang0 - option 1 is a breaking change for users using stats/tracing. So option 2 is better - except have the utility class in grpc-core instead of grpc-census (if possible).

[njhill]

@dapengzhang0 @sanjaypujare I don't follow why 1 can't be done without breaking existing users?

  static boolean classFound(String name) {
    try {
      Class.forName(name, /* init = */ false, StatsComponent.class.getClassLoader());
      return true;
    } catch (ClassNotFoundException e) { }
    return false;
  }

  boolean enableStats = classFound("io.opencensus.impl.stats.StatsComponentImpl")
      || classFound("io.opencensus.impllite.stats.StatsComponentImplLite");

  boolean enableTracing = classFound("io.opencensus.impl.trace.TraceComponentImpl")
      || classFound("io.opencensus.impllite.trace.TraceComponentImplLite");

[dapengzhang0]

@njhill Option 1 will be depending on a new artifact grpc-census for existing census users. I agree that your solution is not breaking existing users, but it seems different from Option 1.

[sanjaypujare]

@njhill So people who do want to use stats/tracing will need to add the new dependency grpc-census in their build to maintain existing functionality. Without that change it breaks, isn't that right?

[njhill]

@dapengzhang0 @sanjaypujare ok sorry I think I didn't properly digest @zhangkun83's comment which is specifically talking about how to proceed with splitting the opencensus-specific logic into a separate module.

I was actually more interested in something which could hopefully be a simpler and non-breaking first step - to control whether the interceptors are enabled based on presence of the opencensus impl classes themselves.

[njhill]

I've opened #5520 for consideration.

[njhill]

Has this now been addressed by ~#5510~ #6577?

[ejona86]

Yes, I think #6577 (not #5510 :smile:) addressed this. I don't think all the discussions are done, but the major issue should be addressed now.

Closing. @njhill, if things still don't look right we can reopen.