-
Notifications
You must be signed in to change notification settings - Fork 185
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.
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
Introduce primitives to control internal log level #1199
Conversation
Motivation: ServiceTalk can generate logs internally. These logs are either at a fixed level hard coded by ServiceTalk devs, or taken dynamically at the time of creation based upon the logging configuration. The first approach is too ridged and may not be appropriate for the application's logging level filtering (e.g. trace logs are never logged, or go against logging configuration policies), or take the log level at a snapshot in time which doesn't allow for practical filtering (e.g. log level may start at OFF, and be switched to non-OFF value later when debugging, and ThresholdFilters are not commonly used and introduce performance overhead because isEnabled() pre-checks will succeed resulting in wasted string building). Modifications: - Introduce a way to explicitly configure the logging level for interesting cases (e.g. wire logging, h2 frame logging, zipkin logging reporter) - Introduce a way to avoid logging user data for wire logging and h2 frame logging so it can be enabled in production. Result: Primitives exits such that Applications can more explicitly control the log level used internally by ServiceTalk.
build failure attributed to #1200 test this please |
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/H2ProtocolConfig.java
Outdated
Show resolved
Hide resolved
* data. | ||
* @return {@code this}. | ||
*/ | ||
public abstract HttpServerBuilder enableWireLogging(String loggerName, LogLevel logLevel, boolean logUserData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we can reuse LoggerConfig
here as well to make configuration consistent.
servicetalk-http-api/src/main/java/io/servicetalk/http/api/BaseHttpBuilder.java
Outdated
Show resolved
Hide resolved
...alk-http-netty/src/main/java/io/servicetalk/http/netty/H2ServerParentChannelInitializer.java
Outdated
Show resolved
Hide resolved
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/ServiceTalkHttp2FrameLogger.java
Outdated
Show resolved
Hide resolved
servicetalk-http-api/src/main/java/io/servicetalk/http/api/BaseHttpBuilder.java
Show resolved
Hide resolved
servicetalk-http-api/src/main/java/io/servicetalk/http/api/BaseHttpBuilder.java
Outdated
Show resolved
Hide resolved
...y-internal/src/main/java/io/servicetalk/transport/netty/internal/WireLoggingInitializer.java
Outdated
Show resolved
Hide resolved
fddf5a4
to
17beed7
Compare
17beed7
to
1921d58
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
A few more suggestions, then LGTM:
...alk-http-netty/src/main/java/io/servicetalk/http/netty/H2ServerParentChannelInitializer.java
Outdated
Show resolved
Hide resolved
...nternal/src/main/java/io/servicetalk/logging/slf4j/internal/DefaultUserDataLoggerConfig.java
Outdated
Show resolved
Hide resolved
...nternal/src/main/java/io/servicetalk/logging/slf4j/internal/DefaultUserDataLoggerConfig.java
Show resolved
Hide resolved
...lk-tcp-netty-internal/src/main/java/io/servicetalk/tcp/netty/internal/AbstractTcpConfig.java
Show resolved
Hide resolved
...ty-internal/src/main/java/io/servicetalk/transport/netty/internal/ServiceTalkWireLogger.java
Show resolved
Hide resolved
...etty-internal/src/main/java/io/servicetalk/tcp/netty/internal/AbstractReadOnlyTcpConfig.java
Outdated
Show resolved
Hide resolved
* @return a {@link FixedLevelLogger}. | ||
*/ | ||
public static FixedLevelLogger newLogger(final String loggerName, final LogLevel level) { | ||
final Logger logger = LoggerFactory.getLogger(loggerName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requireNonNull(loggerName)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll let LoggerFactory.getLogger
enforce this.
build failure attributed to #1200 test this please |
Motivation:
ServiceTalk can generate logs internally. These logs are either at a
fixed level hard coded by ServiceTalk devs, or taken dynamically at the
time of creation based upon the logging configuration. The first
approach is too ridged and may not be appropriate for the application's
logging level filtering (e.g. trace logs are never logged, or go against
logging configuration policies), or take the log level at a snapshot in
time which doesn't allow for practical filtering (e.g. log level may
start at OFF, and be switched to non-OFF value later when debugging, and
ThresholdFilters are not commonly used and introduce performance
overhead because isEnabled() pre-checks will succeed resulting in
wasted string building).
Modifications:
interesting cases (e.g. wire logging, h2 frame logging, zipkin logging
reporter)
frame logging so it can be enabled in production.
Result:
Primitives exits such that Applications can more explicitly control the
log level used internally by ServiceTalk.