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

Enable kv logs in log4j configuration #3986

Merged
merged 3 commits into from
Aug 29, 2023

Conversation

hangc0276
Copy link
Contributor

@hangc0276 hangc0276 commented Jun 14, 2023

Motivation

We introduced DirectIO since BookKeeper 4.16.0, and the DirectIO classes use KV-based Slf4jSlogger. https://github.com/apache/bookkeeper/blob/master/bookkeeper-slogger/slf4j/src/main/java/org/apache/bookkeeper/slogger/slf4j/Slf4jSlogger.java

In order to print the KV out, we need to add %X flag in log4j2.yaml, otherwise the log will miss the detailed key and values.

Modifications

In order to reduce the impact on current logger appenders, I add MDC-based appenders for CONSOLE, TRACEFILE, and ROLLINGFILE by adding %X for log4j2.yaml Appender PatternLayout.
Only classes under org.apache.bookkeeper.bookie.storage.directentrylogger will enable MDC-based appender logger.

If the key value has items, the log will look like:

2023-06-19T16:01:56,685+0800 [main] INFO  org.apache.bookkeeper.bookie.storage.directentrylogger.EntryLogIdsImpl {dirs=[data/bookkeeper/ledgers/current], durationMs=0, event=ENTRYLOG_IDS_CANDIDATES_SELECTED, maxId=21474836
47, nextId=0} - ENTRYLOG_IDS_CANDIDATES_SELECTED
2023-06-19T16:01:56,721+0800 [main] INFO  org.apache.bookkeeper.bookie.storage.directentrylogger.DirectEntryLogger {directory=data/bookkeeper/ledgers/current, event=ENTRYLOGGER_CREATED, maxCachedReaders=32, maxCachedReader
sPerThread=4, maxFileSize=1073741824, maxSaneEntrySize=5252620, perThreadBufferSize=33554432, readBufferSize=8388608, singleWriteBufferSize=33554432, totalReadBufferSize=268435456, totalWriteBufferSize=268435456} - ENTRYLO
GGER_CREATED

@dlg99
Copy link
Contributor

dlg99 commented Jun 17, 2023

What are the performance implications of this?

@hangc0276
Copy link
Contributor Author

What are the performance implications of this?

@dlg99 In order to reduce the impaction on current logger appenders, I add an extra MDC-based appenders for classes under org.apache.bookkeeper.bookie.storage.directentrylogger. Please help take a look, thanks.

Copy link
Contributor

@dlg99 dlg99 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@zymap zymap merged commit da1d8eb into apache:master Aug 29, 2023
zymap pushed a commit that referenced this pull request Aug 29, 2023
### Motivation
We introduced DirectIO since BookKeeper 4.16.0, and the DirectIO classes use KV-based Slf4jSlogger. https://github.com/apache/bookkeeper/blob/master/bookkeeper-slogger/slf4j/src/main/java/org/apache/bookkeeper/slogger/slf4j/Slf4jSlogger.java

In order to print the KV out, we need to add `%X` flag in log4j2.yaml, otherwise the log will miss the detailed key and values.

<!-- Explain here the context, and why you're making that change. What is the problem you're trying to solve. -->

### Modifications
In order to reduce the impact on current logger appenders, I add MDC-based appenders for `CONSOLE`, `TRACEFILE`, and `ROLLINGFILE` by adding `%X` for log4j2.yaml Appender PatternLayout.
Only classes under `org.apache.bookkeeper.bookie.storage.directentrylogger` will enable MDC-based appender logger.

If the key value has items, the log will look like:
```
2023-06-19T16:01:56,685+0800 [main] INFO  org.apache.bookkeeper.bookie.storage.directentrylogger.EntryLogIdsImpl {dirs=[data/bookkeeper/ledgers/current], durationMs=0, event=ENTRYLOG_IDS_CANDIDATES_SELECTED, maxId=21474836
47, nextId=0} - ENTRYLOG_IDS_CANDIDATES_SELECTED
2023-06-19T16:01:56,721+0800 [main] INFO  org.apache.bookkeeper.bookie.storage.directentrylogger.DirectEntryLogger {directory=data/bookkeeper/ledgers/current, event=ENTRYLOGGER_CREATED, maxCachedReaders=32, maxCachedReader
sPerThread=4, maxFileSize=1073741824, maxSaneEntrySize=5252620, perThreadBufferSize=33554432, readBufferSize=8388608, singleWriteBufferSize=33554432, totalReadBufferSize=268435456, totalWriteBufferSize=268435456} - ENTRYLO
GGER_CREATED
```

(cherry picked from commit da1d8eb)
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
### Motivation
We introduced DirectIO since BookKeeper 4.16.0, and the DirectIO classes use KV-based Slf4jSlogger. https://github.com/apache/bookkeeper/blob/master/bookkeeper-slogger/slf4j/src/main/java/org/apache/bookkeeper/slogger/slf4j/Slf4jSlogger.java 

In order to print the KV out, we need to add `%X` flag in log4j2.yaml, otherwise the log will miss the detailed key and values.

<!-- Explain here the context, and why you're making that change. What is the problem you're trying to solve. -->

### Modifications
In order to reduce the impact on current logger appenders, I add MDC-based appenders for `CONSOLE`, `TRACEFILE`, and `ROLLINGFILE` by adding `%X` for log4j2.yaml Appender PatternLayout. 
Only classes under `org.apache.bookkeeper.bookie.storage.directentrylogger` will enable MDC-based appender logger.

If the key value has items, the log will look like:
```
2023-06-19T16:01:56,685+0800 [main] INFO  org.apache.bookkeeper.bookie.storage.directentrylogger.EntryLogIdsImpl {dirs=[data/bookkeeper/ledgers/current], durationMs=0, event=ENTRYLOG_IDS_CANDIDATES_SELECTED, maxId=21474836
47, nextId=0} - ENTRYLOG_IDS_CANDIDATES_SELECTED
2023-06-19T16:01:56,721+0800 [main] INFO  org.apache.bookkeeper.bookie.storage.directentrylogger.DirectEntryLogger {directory=data/bookkeeper/ledgers/current, event=ENTRYLOGGER_CREATED, maxCachedReaders=32, maxCachedReader
sPerThread=4, maxFileSize=1073741824, maxSaneEntrySize=5252620, perThreadBufferSize=33554432, readBufferSize=8388608, singleWriteBufferSize=33554432, totalReadBufferSize=268435456, totalWriteBufferSize=268435456} - ENTRYLO
GGER_CREATED
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants