fix: correct log timestamps, thread attribution and config discovery - #7705
Merged
Conversation
Vest
force-pushed
the
fix/logging-formatter
branch
from
August 17, 2026 15:58
34237ef to
cb1c0be
Compare
logging.properties is what wires up SourceLogFormatter and the LoggingRecorder behind the Debug dialog, but it was only ever looked for in the working directory. A packaged launch leaves user.dir as "/", so the file was never found and PCGen silently fell back to the JDK default logging configuration, losing the formatter, the pcgen.log recorder and the contents of the Debug dialog. The working directory is still tried first, then the install directory derived from java.home, mirroring how the bundled data directories are located. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SourceLogFormatter took its timestamp from LocalDateTime.now() at the moment of formatting rather than from the record, so the time reported was not the time the event occurred, and two handlers formatting the same record disagreed. It also printed bare UTC with no offset, which silently differs from the wall clock and from surrounding system log lines. It now uses record.getInstant(), rendered as local time with an explicit offset and a fixed width. The thread was reported as Thread.currentThread().getName(), which names whichever thread happened to format the record rather than the one that logged it. The originating thread is now reported as name#id: the id comes from LogRecord.getLongThreadID(), which is the real Thread.threadId captured when the record was created and matches thread dumps and debuggers, while the name is only added when this is still the logging thread, since LogRecord does not carry it. Unnamed virtual threads and records formatted elsewhere print as #id rather than being misattributed. Replaces the per-record new Throwable().getStackTrace() used to find the caller with a lazy StackWalker that stops at the first frame outside the logging plumbing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Converts the nine remaining Logging.log(Logging.ERROR/Level.SEVERE, ...) call sites to Logging.errorPrint(...), which is the same code path and level and the idiom used by the other ~794 call sites. Also documents the logging conventions (errorPrint idiom, logging.properties wiring, SourceLogFormatter behavior) in AGENTS.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Vest
force-pushed
the
fix/logging-formatter
branch
from
August 17, 2026 16:02
cb1c0be to
dd4ab60
Compare
The formatter rendered the record's instant as local time with an explicit offset (name#id timestamps like 2026-08-18T10:03:04.872+02:00). For a single-user desktop app the offset is noise at best, and it leaks the user's approximate location into logs that get pasted into public bug reports. Drop the offset and print bare local wall-clock time, which is what the user sees on their own clock. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
findLoggingConfig and candidateConfigDirs walked the filesystem with java.io.File. Switch them to java.nio.file.Path/Files, the modern API: Path.resolve for child paths, Path.getParent for the upward walk, and Files.isRegularFile for the existence check. Behavior is unchanged; the String inputs from SystemUtils are kept at the call boundary. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The " (Source: <uri>)" suffix was built by string concatenation and duplicated across five log call sites (SonarLint S1192, and S3457 for concatenating into a format-aware Logger.log message). Extract the literal into a single SOURCE_FORMAT constant and build the message with MessageFormat.format, passing the message and URI as arguments. MessageFormat does not recursively parse argument values, so error messages containing braces or quotes are unaffected. This also drops the stray space before the closing paren that four of the sites had, so all of them now render "(Source: <uri>)" consistently. These warnings are pre-existing, not introduced by this branch; fixing them here while the file is already being changed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Vest
force-pushed
the
fix/logging-formatter
branch
from
August 18, 2026 08:28
8bc5d6e to
e9fd0c3
Compare
Rewrite the formatter tests to guard functional guarantees rather than the exact layout: the timestamp derives from the record's own instant (same instant -> same stamp, different -> different, compared as a field not a pattern), the logging thread is attributed by name and id, a record formatted on another thread keeps its origin id without claiming the formatting thread, and a thrown stack trace is rendered. The exact timestamp pattern and thread rendering can now change without touching these tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Trim the findLoggingConfig javadoc to a one-line what-and-why, and reword the java.home walk comment to make clear it is layout-agnostic (macOS is only cited as the deepest nesting that sets MAX_ANCESTORS) rather than reading as a macOS special case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Vest
force-pushed
the
fix/logging-formatter
branch
from
August 18, 2026 08:53
a4d8cf8 to
75ae1ad
Compare
Contributor
🚧 PCGen Code Coverage
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
logging.propertieswas only searched in the working directory; a packaged launch leavesuser.diras/so the file was never found and PCGen silently fell back to the JDK default, losingSourceLogFormatter, theLoggingRecorder(Debug dialog), and pcgen.log. The fix also tries the install directory derived fromjava.home, mirroring how bundled data is located.SourceLogFormatterstamped log records withLocalDateTime.now()at format time rather thanrecord.getInstant(), so the reported time was wrong and two handlers formatting the same record disagreed. Timestamps now use the record's instant, rendered as local wall-clock time (no zone offset).Thread.currentThread().getName()— the formatting thread, not the originating one. The fix reports the originating thread asname#idusingLogRecord.getLongThreadID(), which matches thread dumps and debuggers. Unnamed/virtual threads and records formatted off-thread print as#id.new Throwable().getStackTrace()caller-finder with a lazyStackWalkerthat stops at the first frame outside logging plumbing.Logging.log(Logging.ERROR/Level.SEVERE, ...)call sites toLogging.errorPrint(...), consistent with the other ~794 call sites.java.io.Filetojava.nio.file.Path, and tidies the(Source: <uri>)log suffix (SonarLint S1192/S3457).Test Plan
./gradlew :test --tests "pcgen.util.LoggingConfigTest"— verifies config-file discovery fallback logic./gradlew :test --tests "pcgen.util.SourceLogFormatterTest"— verifies timestamp, thread attribution, and caller functionalitypcgen.logis written — verified on Linux (SUSE VM) and via the macOSuser.dir=/java.home fallback