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.

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

Merged
merged 6 commits into from
Nov 10, 2020

Conversation

Scottmitch
Copy link
Member

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.

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.
@Scottmitch
Copy link
Member Author

build failure attributed to #1200

test this please

* data.
* @return {@code this}.
*/
public abstract HttpServerBuilder enableWireLogging(String loggerName, LogLevel logLevel, boolean logUserData);
Copy link
Member

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.

Copy link
Member

@idelpivnitskiy idelpivnitskiy left a 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:

* @return a {@link FixedLevelLogger}.
*/
public static FixedLevelLogger newLogger(final String loggerName, final LogLevel level) {
final Logger logger = LoggerFactory.getLogger(loggerName);
Copy link
Member

Choose a reason for hiding this comment

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

requireNonNull(loggerName)?

Copy link
Member Author

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.

@Scottmitch
Copy link
Member Author

build failure attributed to #1200

test this please

@Scottmitch Scottmitch merged commit f12492c into apple:main Nov 10, 2020
@Scottmitch Scottmitch deleted the logging_level branch November 10, 2020 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants