Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,11 +172,11 @@ protected final int maxInboundMessageSize() {
return maxInboundMessageSize;
}

private boolean statsEnabled = true;
private boolean statsEnabled = CensusStatsModule.OPENCENSUS_IMPL_PRESENT;
private boolean recordStartedRpcs = true;
private boolean recordFinishedRpcs = true;
private boolean recordRealTimeMetrics = false;
private boolean tracingEnabled = true;
private boolean tracingEnabled = CensusTracingModule.OPENCENSUS_IMPL_PRESENT;

@Nullable
private CensusStatsModule censusStatsOverride;
Expand Down
4 changes: 2 additions & 2 deletions core/src/main/java/io/grpc/internal/AbstractServerImplBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,11 @@ public static ServerBuilder<?> forPort(int port) {
CompressorRegistry compressorRegistry = DEFAULT_COMPRESSOR_REGISTRY;
long handshakeTimeoutMillis = DEFAULT_HANDSHAKE_TIMEOUT_MILLIS;
@Nullable private CensusStatsModule censusStatsOverride;
private boolean statsEnabled = true;
private boolean statsEnabled = CensusStatsModule.OPENCENSUS_IMPL_PRESENT;
private boolean recordStartedRpcs = true;
private boolean recordFinishedRpcs = true;
private boolean recordRealTimeMetrics = false;
private boolean tracingEnabled = true;
private boolean tracingEnabled = CensusTracingModule.OPENCENSUS_IMPL_PRESENT;
@Nullable BinaryLog binlog;
TransportTracer.Factory transportTracerFactory = TransportTracer.getDefaultFactory();
InternalChannelz channelz = InternalChannelz.instance();
Expand Down
9 changes: 9 additions & 0 deletions core/src/main/java/io/grpc/internal/CensusStatsModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,15 @@ public final class CensusStatsModule {
private static final Logger logger = Logger.getLogger(CensusStatsModule.class.getName());
private static final double NANOS_PER_MILLI = TimeUnit.MILLISECONDS.toNanos(1);

static final boolean OPENCENSUS_IMPL_PRESENT;

static {
ClassLoader loader = io.opencensus.stats.StatsComponent.class.getClassLoader();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if this can be CensusStatsModule.class.getClassLoader(), and people who don't want census can remove runtime dependency of census completely, namely making census api as a compile-time-only dependency for grpc library.

Copy link
Contributor Author

@njhill njhill Mar 29, 2019 •
edited by sanjaypujare
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dapengzhang0 you mean also remove the existing opencensus-api dependency? I don't think that would be possible without the options discussed in #5510 which both require some kind of breakage, at a minimum would need existing census users to amend their builds.

Copy link
Member

@dapengzhang0 dapengzhang0 Mar 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean not removing opencensus-api transparenttransitive dependency from grpc library, but about the ability for people who don't want census to explicitly remove that dependency in their own project's classpath.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I see what you mean, yes I think that should be possible, it would require a couple more changes than this though, for example io.opencensus.trace.Tracing is also referenced directly in the channel and server builder classes.

OPENCENSUS_IMPL_PRESENT =
GrpcUtil.classIsFound("io.opencensus.impl.stats.StatsComponentImpl", loader)
|| GrpcUtil.classIsFound("io.opencensus.impllite.stats.StatsComponentImplLite", loader);
}

private final Tagger tagger;
private final StatsRecorder statsRecorder;
private final Supplier<Stopwatch> stopwatchSupplier;
Expand Down
7 changes: 7 additions & 0 deletions core/src/main/java/io/grpc/internal/CensusTracingModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@
final class CensusTracingModule {
private static final Logger logger = Logger.getLogger(CensusTracingModule.class.getName());

static final boolean OPENCENSUS_IMPL_PRESENT;

@Nullable private static final AtomicIntegerFieldUpdater<ClientCallTracer> callEndedUpdater;

@Nullable private static final AtomicIntegerFieldUpdater<ServerTracer> streamClosedUpdater;
Expand All @@ -84,6 +86,11 @@ final class CensusTracingModule {
}
callEndedUpdater = tmpCallEndedUpdater;
streamClosedUpdater = tmpStreamClosedUpdater;

ClassLoader loader = io.opencensus.trace.TraceComponent.class.getClassLoader();
OPENCENSUS_IMPL_PRESENT =
GrpcUtil.classIsFound("io.opencensus.impl.trace.TraceComponentImpl", loader)
|| GrpcUtil.classIsFound("io.opencensus.impllite.trace.TraceComponentImplLite", loader);
}

private final Tracer censusTracer;
Expand Down
13 changes: 13 additions & 0 deletions core/src/main/java/io/grpc/internal/GrpcUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -781,5 +781,18 @@ static <T> boolean iterableContains(Iterable<T> iterable, T item) {
return false;
}

/**
* Returns true if the specified class is found on the classpath, false otherwise.
*/
public static boolean classIsFound(String className, ClassLoader classLoader) {
try {
Class.forName(className, /* initialize = */ false, classLoader);
return true;
} catch (ClassNotFoundException e) {
// no problem
}
return false;
}

private GrpcUtil() {}
}
32 changes: 16 additions & 16 deletions core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,43 +296,43 @@ public void makeTargetStringForDirectAddress_scopedIpv6() throws Exception {
public void getEffectiveInterceptors_default() {
builder.intercept(DUMMY_USER_INTERCEPTOR);
List<ClientInterceptor> effectiveInterceptors = builder.getEffectiveInterceptors();
assertEquals(3, effectiveInterceptors.size());
assertThat(effectiveInterceptors.get(0))
.isInstanceOf(CensusTracingModule.TracingClientInterceptor.class);
assertThat(effectiveInterceptors.get(1))
.isInstanceOf(CensusStatsModule.StatsClientInterceptor.class);
assertThat(effectiveInterceptors.get(2)).isSameAs(DUMMY_USER_INTERCEPTOR);
assertThat(effectiveInterceptors).containsExactly(DUMMY_USER_INTERCEPTOR);
}

@Test
public void getEffectiveInterceptors_disableStats() {
public void getEffectiveInterceptors_enableStats() {
builder.intercept(DUMMY_USER_INTERCEPTOR);
builder.setStatsEnabled(false);
builder.setStatsEnabled(true);
List<ClientInterceptor> effectiveInterceptors = builder.getEffectiveInterceptors();
assertEquals(2, effectiveInterceptors.size());
assertThat(effectiveInterceptors.get(0))
.isInstanceOf(CensusTracingModule.TracingClientInterceptor.class);
.isInstanceOf(CensusStatsModule.StatsClientInterceptor.class);
assertThat(effectiveInterceptors.get(1)).isSameAs(DUMMY_USER_INTERCEPTOR);
}

@Test
public void getEffectiveInterceptors_disableTracing() {
public void getEffectiveInterceptors_enableTracing() {
builder.intercept(DUMMY_USER_INTERCEPTOR);
builder.setTracingEnabled(false);
builder.setTracingEnabled(true);
List<ClientInterceptor> effectiveInterceptors = builder.getEffectiveInterceptors();
assertEquals(2, effectiveInterceptors.size());
assertThat(effectiveInterceptors.get(0))
.isInstanceOf(CensusStatsModule.StatsClientInterceptor.class);
.isInstanceOf(CensusTracingModule.TracingClientInterceptor.class);
assertThat(effectiveInterceptors.get(1)).isSameAs(DUMMY_USER_INTERCEPTOR);
}

@Test
public void getEffectiveInterceptors_disableBoth() {
public void getEffectiveInterceptors_enableBoth() {
builder.intercept(DUMMY_USER_INTERCEPTOR);
builder.setStatsEnabled(false);
builder.setTracingEnabled(false);
builder.setStatsEnabled(true);
builder.setTracingEnabled(true);
List<ClientInterceptor> effectiveInterceptors = builder.getEffectiveInterceptors();
assertThat(effectiveInterceptors).containsExactly(DUMMY_USER_INTERCEPTOR);
assertEquals(3, effectiveInterceptors.size());
assertThat(effectiveInterceptors.get(0))
.isInstanceOf(CensusTracingModule.TracingClientInterceptor.class);
assertThat(effectiveInterceptors.get(1))
.isInstanceOf(CensusStatsModule.StatsClientInterceptor.class);
assertThat(effectiveInterceptors.get(2)).isSameAs(DUMMY_USER_INTERCEPTOR);
}

@Test
Expand Down
29 changes: 14 additions & 15 deletions core/src/test/java/io/grpc/internal/AbstractServerImplBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,44 +49,43 @@ public void getTracerFactories_default() {
builder.addStreamTracerFactory(DUMMY_USER_TRACER);

List<? extends ServerStreamTracer.Factory> factories = builder.getTracerFactories();

assertEquals(3, factories.size());
assertThat(factories.get(0)).isInstanceOf(CensusStatsModule.ServerTracerFactory.class);
assertThat(factories.get(1)).isInstanceOf(CensusTracingModule.ServerTracerFactory.class);
assertThat(factories.get(2)).isSameAs(DUMMY_USER_TRACER);
assertThat(factories).containsExactly(DUMMY_USER_TRACER);
}

@Test
public void getTracerFactories_disableStats() {
public void getTracerFactories_enableStats() {
builder.addStreamTracerFactory(DUMMY_USER_TRACER);
builder.setStatsEnabled(false);
builder.setStatsEnabled(true);

List<? extends ServerStreamTracer.Factory> factories = builder.getTracerFactories();

assertEquals(2, factories.size());
assertThat(factories.get(0)).isInstanceOf(CensusTracingModule.ServerTracerFactory.class);
assertThat(factories.get(0)).isInstanceOf(CensusStatsModule.ServerTracerFactory.class);
assertThat(factories.get(1)).isSameAs(DUMMY_USER_TRACER);
}

@Test
public void getTracerFactories_disableTracing() {
public void getTracerFactories_enableTracing() {
builder.addStreamTracerFactory(DUMMY_USER_TRACER);
builder.setTracingEnabled(false);
builder.setTracingEnabled(true);

List<? extends ServerStreamTracer.Factory> factories = builder.getTracerFactories();

assertEquals(2, factories.size());
assertThat(factories.get(0)).isInstanceOf(CensusStatsModule.ServerTracerFactory.class);
assertThat(factories.get(0)).isInstanceOf(CensusTracingModule.ServerTracerFactory.class);
assertThat(factories.get(1)).isSameAs(DUMMY_USER_TRACER);
}

@Test
public void getTracerFactories_disableBoth() {
public void getTracerFactories_enableBoth() {
builder.addStreamTracerFactory(DUMMY_USER_TRACER);
builder.setTracingEnabled(false);
builder.setStatsEnabled(false);
builder.setTracingEnabled(true);
builder.setStatsEnabled(true);
List<? extends ServerStreamTracer.Factory> factories = builder.getTracerFactories();
assertThat(factories).containsExactly(DUMMY_USER_TRACER);
assertEquals(3, factories.size());
assertThat(factories.get(0)).isInstanceOf(CensusStatsModule.ServerTracerFactory.class);
assertThat(factories.get(1)).isInstanceOf(CensusTracingModule.ServerTracerFactory.class);
assertThat(factories.get(2)).isSameAs(DUMMY_USER_TRACER);
}

static class Builder extends AbstractServerImplBuilder<Builder> {
Expand Down