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

Jump to bottom

api, core: make channel logger accessible through NameResolver.Args #6430

Merged
merged 4 commits into from Nov 19, 2019
Merged

api, core: make channel logger accessible through NameResolver.Args #6430

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a18eca8
Added API for accessing channel logger on NameResolver.Args.
voidzcy Nov 15, 2019
11ba017
Make ChannelLogger required in builder, but would only throw exceptio…
voidzcy Nov 18, 2019
495c4ce
Added ExperimentalApi annotation.
voidzcy Nov 18, 2019
1f0d13c
Removed requirement for channel logger in builder.
voidzcy Nov 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 43 additions & 1 deletion api/src/main/java/io/grpc/NameResolver.java
View file
Open in desktop
Expand Up @@ -408,22 +408,39 @@ public ConfigOrError parseServiceConfig(Map<String, ?> rawServiceConfig) {
*/
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/1770")
public static final class Args {
private static final ChannelLogger NOOP_CHANNEL_LOGGER = new ChannelLogger() {
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 a bit concerned about a logger that does nothing. I wonder if it would be better to just have it be null.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, because the javadoc says "Information logged here goes to Channelz, and to the Java logger of this class as well."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was something I hesitated on, as said in #6430 (comment).

@Override
public void log(ChannelLogLevel level, String message) {
}

@Override
public void log(ChannelLogLevel level, String messageFormat, Object... args) {
}
};

private final int defaultPort;
private final ProxyDetector proxyDetector;
private final SynchronizationContext syncContext;
private final ServiceConfigParser serviceConfigParser;
private final ChannelLogger channelLogger;
@Nullable private final Executor executor;

Args(
Integer defaultPort,
ProxyDetector proxyDetector,
SynchronizationContext syncContext,
ServiceConfigParser serviceConfigParser,
@Nullable ChannelLogger channelLogger,
@Nullable Executor executor) {
this.defaultPort = checkNotNull(defaultPort, "defaultPort not set");
this.proxyDetector = checkNotNull(proxyDetector, "proxyDetector not set");
this.syncContext = checkNotNull(syncContext, "syncContext not set");
this.serviceConfigParser = checkNotNull(serviceConfigParser, "serviceConfigParser not set");
if (channelLogger == null) {
this.channelLogger = NOOP_CHANNEL_LOGGER;
} else {
this.channelLogger = channelLogger;
}
this.executor = executor;
}

Expand Down Expand Up @@ -466,6 +483,15 @@ public ServiceConfigParser getServiceConfigParser() {
return serviceConfigParser;
}

/**
* Returns the {@link ChannelLogger} for the Channel served by this NameResolver.
*
* @since 1.26.0
*/
public ChannelLogger getChannelLogger() {
return channelLogger;
}

/**
* Returns the Executor on which this resolver should execute long-running or I/O bound work.
* Null if no Executor was set.
Expand All @@ -485,6 +511,7 @@ public String toString() {
.add("proxyDetector", proxyDetector)
.add("syncContext", syncContext)
.add("serviceConfigParser", serviceConfigParser)
.add("channelLogger", channelLogger)
.add("executor", executor)
.toString();
}
Expand All @@ -500,6 +527,7 @@ public Builder toBuilder() {
builder.setProxyDetector(proxyDetector);
builder.setSynchronizationContext(syncContext);
builder.setServiceConfigParser(serviceConfigParser);
builder.setChannelLogger(channelLogger);
builder.setOffloadExecutor(executor);
return builder;
}
Expand All @@ -523,6 +551,7 @@ public static final class Builder {
private ProxyDetector proxyDetector;
private SynchronizationContext syncContext;
private ServiceConfigParser serviceConfigParser;
private ChannelLogger channelLogger;
private Executor executor;

Builder() {
Expand Down Expand Up @@ -568,6 +597,16 @@ public Builder setServiceConfigParser(ServiceConfigParser parser) {
return this;
}

/**
* See {@link Args#getChannelLogger}. This is an optional field.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to be optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like the requirement is too strong for user that must provide a ChannelLogger. So if it is not set, it will just default to a no-op channel logger. Do you think it's better to make this required?

Copy link
Contributor

Choose a reason for hiding this comment

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

gRPC is the provider of the Args in most, if not all of the cases. Users of this API are on the consumer side, and they will appreciate a guaranteed non-null ChannelLogger, rather than having to check for null (or wonder whether it's a no-op) every time they use it.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to call out it is an optional field. We will always set it and if their test doesn't they will just get a NPE. I don't think we should have validation in the builder to guarantee it is there, but that doesn't mean consumers of it need to check for nullness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You two have different opinions... So should we make this required in API?

I would prefer this being required (given that we do not default to no-op channel logger if not set) and provides a non-null guarantee to users. In general, I don't like @Nullable APIs and would like to avoid as much as possible.

Copy link
Contributor Author

@voidzcy voidzcy Nov 18, 2019 •
edited

Choose a reason for hiding this comment

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

As discussed offline, we are trying to make a "soft requirement" for ChannelLogger in NameResolver.Args. That is, it is only required for NameResolver.Args users that ChannelLogger matters to them.

We do not call out if it is required or optional in builder. Implementors that care about this field know what to do.

*
* @since 1.26.0
*/
public Builder setChannelLogger(ChannelLogger channelLogger) {
this.channelLogger = checkNotNull(channelLogger);
return this;
}

/**
* See {@link Args#getOffloadExecutor}. This is an optional field.
*
Expand All @@ -585,7 +624,10 @@ public Builder setOffloadExecutor(Executor executor) {
* @since 1.21.0
*/
public Args build() {
return new Args(defaultPort, proxyDetector, syncContext, serviceConfigParser, executor);
return
new Args(
defaultPort, proxyDetector, syncContext, serviceConfigParser,
channelLogger, executor);
}
}
}
Expand Down
4 changes: 4 additions & 0 deletions api/src/test/java/io/grpc/NameResolverTest.java
View file
Open in desktop
Expand Up @@ -45,6 +45,7 @@ public class NameResolverTest {
private final SynchronizationContext syncContext =
new SynchronizationContext(mock(UncaughtExceptionHandler.class));
private final ServiceConfigParser parser = mock(ServiceConfigParser.class);
private final ChannelLogger channelLogger = mock(ChannelLogger.class);
private final Executor executor = Executors.newSingleThreadExecutor();
private URI uri;
private final NameResolver nameResolver = mock(NameResolver.class);
Expand All @@ -61,13 +62,15 @@ public void args() {
assertThat(args.getProxyDetector()).isSameInstanceAs(proxyDetector);
assertThat(args.getSynchronizationContext()).isSameInstanceAs(syncContext);
assertThat(args.getServiceConfigParser()).isSameInstanceAs(parser);
assertThat(args.getChannelLogger()).isSameInstanceAs(channelLogger);
assertThat(args.getOffloadExecutor()).isSameInstanceAs(executor);

NameResolver.Args args2 = args.toBuilder().build();
assertThat(args2.getDefaultPort()).isEqualTo(defaultPort);
assertThat(args2.getProxyDetector()).isSameInstanceAs(proxyDetector);
assertThat(args2.getSynchronizationContext()).isSameInstanceAs(syncContext);
assertThat(args2.getServiceConfigParser()).isSameInstanceAs(parser);
assertThat(args2.getChannelLogger()).isSameInstanceAs(channelLogger);
assertThat(args2.getOffloadExecutor()).isSameInstanceAs(executor);

assertThat(args2).isNotSameInstanceAs(args);
Expand Down Expand Up @@ -251,6 +254,7 @@ private NameResolver.Args createArgs() {
.setProxyDetector(proxyDetector)
.setSynchronizationContext(syncContext)
.setServiceConfigParser(parser)
.setChannelLogger(channelLogger)
.setOffloadExecutor(executor)
.build();
}
Expand Down
13 changes: 7 additions & 6 deletions core/src/main/java/io/grpc/internal/ManagedChannelImpl.java
View file
Open in desktop
Expand Up @@ -561,6 +561,12 @@ ClientStream newSubstream(ClientStreamTracer.Factory tracerFactory, Metadata new
final TimeProvider timeProvider) {
this.target = checkNotNull(builder.target, "target");
this.logId = InternalLogId.allocate("Channel", target);
this.timeProvider = checkNotNull(timeProvider, "timeProvider");
maxTraceEvents = builder.maxTraceEvents;
channelTracer = new ChannelTracer(
logId, builder.maxTraceEvents, timeProvider.currentTimeNanos(),
"Channel for '" + target + "'");
channelLogger = new ChannelLoggerImpl(channelTracer, timeProvider);
this.nameResolverFactory = builder.getNameResolverFactory();
ProxyDetector proxyDetector =
builder.proxyDetector != null ? builder.proxyDetector : GrpcUtil.DEFAULT_PROXY_DETECTOR;
Expand All @@ -581,6 +587,7 @@ ClientStream newSubstream(ClientStreamTracer.Factory tracerFactory, Metadata new
builder.maxRetryAttempts,
builder.maxHedgedAttempts,
loadBalancerFactory))
.setChannelLogger(channelLogger)
.setOffloadExecutor(
// Avoid creating the offloadExecutor until it is first used
new Executor() {
Expand All @@ -591,12 +598,6 @@ public void execute(Runnable command) {
})
.build();
this.nameResolver = getNameResolver(target, nameResolverFactory, nameResolverArgs);
this.timeProvider = checkNotNull(timeProvider, "timeProvider");
maxTraceEvents = builder.maxTraceEvents;
channelTracer = new ChannelTracer(
logId, builder.maxTraceEvents, timeProvider.currentTimeNanos(),
"Channel for '" + target + "'");
channelLogger = new ChannelLoggerImpl(channelTracer, timeProvider);
this.executorPool = checkNotNull(builder.executorPool, "executorPool");
this.balancerRpcExecutorPool = checkNotNull(balancerRpcExecutorPool, "balancerRpcExecutorPool");
this.balancerRpcExecutorHolder = new ExecutorHolder(balancerRpcExecutorPool);
Expand Down