-
Notifications
You must be signed in to change notification settings - Fork 160
AP-25788: Dump log buffer in shutdown hook if not yet initialized #80
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -64,6 +64,19 @@ | |||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||
| final class LogBuffer { | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||
| * Drained buffer contents and any overflow metadata. | ||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||
| * @param messages buffered messages in insertion order | ||||||||||||||||||||||||||||||||||||
| * @param evictedEntries total number of messages evicted from the buffer | ||||||||||||||||||||||||||||||||||||
| * @param evictionMessageLevel log level to use for an overflow notice | ||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||
| record BufferContents(BufferedLogMessage[] messages, long evictedEntries, Level evictionMessageLevel) { | ||||||||||||||||||||||||||||||||||||
| boolean isEmpty() { | ||||||||||||||||||||||||||||||||||||
| return messages.length == 0; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
enplotz marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| private final CircularFiFoBuffer m_logBuffer; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| LogBuffer(final int bufferSize) { | ||||||||||||||||||||||||||||||||||||
|
|
@@ -187,26 +200,48 @@ private int size() { | |||||||||||||||||||||||||||||||||||
| * @param consumer consumer for buffered log messages | ||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||
| void drainTo(final Consumer<BufferedLogMessage> consumer) { | ||||||||||||||||||||||||||||||||||||
| if (!m_logBuffer.isEmpty()) { | ||||||||||||||||||||||||||||||||||||
| final var contents = drain(); | ||||||||||||||||||||||||||||||||||||
| if (!contents.isEmpty()) { | ||||||||||||||||||||||||||||||||||||
| // we expect no NodeContext to be available at this point anyway | ||||||||||||||||||||||||||||||||||||
| final var omitCtx = true; | ||||||||||||||||||||||||||||||||||||
| final var logger = KNIMELogger.getLogger(KNIMELogger.class); | ||||||||||||||||||||||||||||||||||||
| final var current = m_logBuffer.size(); | ||||||||||||||||||||||||||||||||||||
| final var evicted = m_logBuffer.getNumberOfEvictedEntries(); | ||||||||||||||||||||||||||||||||||||
| final var total = current + evicted; | ||||||||||||||||||||||||||||||||||||
| final var total = contents.messages().length + contents.evictedEntries(); | ||||||||||||||||||||||||||||||||||||
| final var countMessages = total > 1 ? "%d messages were".formatted(total) : "1 message was"; | ||||||||||||||||||||||||||||||||||||
| logger.log(Level.DEBUG, () -> "%s logged before logging was initialized; see below..." | ||||||||||||||||||||||||||||||||||||
| .formatted(countMessages), omitCtx, null); | ||||||||||||||||||||||||||||||||||||
| if (evicted > 0) { | ||||||||||||||||||||||||||||||||||||
| logger.log(m_logBuffer.m_levelForEvictionMessage, | ||||||||||||||||||||||||||||||||||||
| if (contents.evictedEntries() > 0) { | ||||||||||||||||||||||||||||||||||||
| logger.log(contents.evictionMessageLevel(), | ||||||||||||||||||||||||||||||||||||
| () -> "[*** Log incomplete: log buffer did wrap around -- " | ||||||||||||||||||||||||||||||||||||
| + "%d messages were evicted from buffer in total ***]".formatted(evicted), omitCtx, null); | ||||||||||||||||||||||||||||||||||||
| + "%d messages were evicted from buffer in total ***]" | ||||||||||||||||||||||||||||||||||||
| .formatted(contents.evictedEntries()), | ||||||||||||||||||||||||||||||||||||
| omitCtx, null); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| for (final var message : contents.messages()) { | ||||||||||||||||||||||||||||||||||||
| consumer.accept(message); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| m_logBuffer.drainingIterator().forEachRemaining(consumer); | ||||||||||||||||||||||||||||||||||||
| logger.log(Level.DEBUG, "End of buffered log messages", omitCtx, null); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||
| * Drains and returns the current buffer contents. | ||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||
| * <p> | ||||||||||||||||||||||||||||||||||||
| * <b>Note:</b> this method is <i>not thread-safe</i>. Callers must synchronize on the parent | ||||||||||||||||||||||||||||||||||||
| * {@link LogBuffer} instance before invoking it. | ||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||
| * @return drained buffer contents in insertion order | ||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||
| BufferContents drain() { | ||||||||||||||||||||||||||||||||||||
| final var current = m_logBuffer.size(); | ||||||||||||||||||||||||||||||||||||
| final var messages = new BufferedLogMessage[current]; | ||||||||||||||||||||||||||||||||||||
| final var iter = m_logBuffer.drainingIterator(); | ||||||||||||||||||||||||||||||||||||
| for (var i = 0; i < current; i++) { | ||||||||||||||||||||||||||||||||||||
| messages[i] = iter.next(); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| return new BufferContents(messages, m_logBuffer.getNumberOfEvictedEntries(), m_logBuffer.m_levelForEvictionMessage); | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+236
to
+242
|
||||||||||||||||||||||||||||||||||||
| final var current = m_logBuffer.size(); | |
| final var messages = new BufferedLogMessage[current]; | |
| final var iter = m_logBuffer.drainingIterator(); | |
| for (var i = 0; i < current; i++) { | |
| messages[i] = iter.next(); | |
| } | |
| return new BufferContents(messages, m_logBuffer.getNumberOfEvictedEntries(), m_logBuffer.m_levelForEvictionMessage); | |
| synchronized (m_logBuffer) { | |
| final var current = m_logBuffer.size(); | |
| final var messages = new BufferedLogMessage[current]; | |
| final var iter = m_logBuffer.drainingIterator(); | |
| for (var i = 0; i < current; i++) { | |
| messages[i] = iter.next(); | |
| } | |
| return new BufferContents(messages, m_logBuffer.getNumberOfEvictedEntries(), | |
| m_logBuffer.m_levelForEvictionMessage); | |
| } |
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.
drainBufferedMessagesTointroduces non-trivial behavior (level filtering, eviction notice gating, and single-shot dumping viaisEmergencyBufferDumped) but there are no tests covering it. Since this module already has JUnit coverage forKNIMELogger(seeorg.knime.core.tests/.../KNIMELoggerTest), it would be good to add tests that (a) buffer some messages before init, (b) invoke the dump with aPrintStreambacked by aByteArrayOutputStream, and (c) assert output respectsEMERGENCY_BUFFER_DUMP_MIN_LEVELand eviction notice behavior.