Repository navigation
Conversation
The "Rotating Debug Logs" section did not say that the suffix must match the rotation settings (milliseconds for size-based rotation), so a common configuration was silently rejected. It also claimed a default suffix that does not exist and that changes apply without a restart. Document the suffix rules, a working example, the startup error message, the restart requirement, and that old files are never deleted. Fixes OpenIdentityPlatform#1180
maximthomas
left a comment
maximthomas
left a comment
There was a problem hiding this comment.
praise: The rewrite corrects two long-standing errors in the section and quotes the real failure message.
- The sample stderr text matches
DebugConfigurationFromProperties.validate()(:146-147) word for word. - "Changes take effect immediately" is gone; the file is loaded once, in the
DebugFileProviderImplconstructor (:45). - The invented default suffix
-MM.dd.yyyy-kk.mmis gone;DefaultDebugConfiguration.getDebugSuffix()returns""(:45-46).
issue (blocking): "The OpenAM .war file does not include this file" is false: the WAR ships WEB-INF/classes/debugconfig.properties.
openam-documentation/openam-doc-source/src/main/asciidoc/admin-guide/chap-monitoring.adoc:395
openam-server-only/openam-server-prepare-war.xml:248-255
The pom's <exclude>config/**</exclude> applies only to Maven resources. openam-server-prepare-war.xml copies config/*.properties into target/classes, and both OpenAM-16.1.4-SNAPSHOT.war and OpenAM-ServerOnly-16.1.4-SNAPSHOT.war contain WEB-INF/classes/debugconfig.properties. In that file the prefix, suffix and rotation are empty, and there is no rotation.maxsize line. The old "edit" was right. The first bullet of the PR description repeats the claim.
By default OpenAM does not rotate debug logs. To rotate debug logs, edit `WEB-INF/classes/debugconfig.properties` where OpenAM is deployed. The file shipped with OpenAM sets the prefix, the suffix and the rotation interval to empty values and has no `org.forgerock.openam.debug.rotation.maxsize` line; add that line to enable rotation based on log file size.issue (blocking): The suffix entry says the suffix is needed only when rotation is on, but if the suffix line is missing, debug logging throws NullPointerException and does not fall back.
openam-documentation/openam-doc-source/src/main/asciidoc/admin-guide/chap-monitoring.adoc:403, :431
openam-shared/src/main/java/com/sun/identity/shared/debug/file/impl/DebugConfigurationFromProperties.java:62, :138, :160
openam-shared/src/main/java/com/sun/identity/shared/debug/file/impl/DebugFileImpl.java:108
openam-shared/src/main/java/com/sun/identity/shared/debug/impl/DebugImpl.java:280
For a missing key, getProperty returns null. With rotation on, validate() calls getDebugSuffix().isEmpty(). DebugFileProviderImpl:47 catches only InvalidDebugConfigurationException, so the NPE escapes Debug's static initialiser, and the "isn't valid, the default configuration will be used" message from line 431 never appears. We reproduced this against openam-shared/target/classes. With rotation off, for example in a prefix-only file, validate() passes. DebugFileImpl:108 then throws on the first write of every debug log, called from DebugImpl.writeIt outside any try. The new "create the file" wording makes a file without this line likely; the shipped file has suffix=.
The property has no default. It is required when rotation is enabled. Keep the `org.forgerock.openam.debug.suffix` line in the file even when rotation is disabled, with an empty value. If the line is missing, debug logging fails with a `NullPointerException` instead of falling back to the default configuration.Or: fix the code instead, and the sentence is not needed — DebugConfigurationFromProperties.java:62: debugSuffix = rotationConfig.getProperty(DebugConstants.CONFIG_DEBUG_LOGFILE_SUFFIX, "");
issue (non-blocking): The failure paragraph says every mismatching suffix is rejected, and it leaves out other values that also make OpenAM ignore the whole file.
openam-documentation/openam-doc-source/src/main/asciidoc/admin-guide/chap-monitoring.adoc:431
openam-shared/src/main/java/com/sun/identity/shared/debug/file/impl/DebugConfigurationFromProperties.java:132-135, :154-157, :185-193
validateSuffix checks the suffix at a single point: it compares the suffix at epoch 0 with the suffix one interval later, in the JVM default time zone. In Asia/Kolkata, -yyyy.MM.dd-HH with rotation=30 passes the check (05:30 → 06:00). Yet files started at 09:00 and 09:30 get the same name, the second interval is appended to the first, and rotation silently becomes hourly. Three other cases are not listed: rotation ≤ 0, rotation.maxsize ≤ 0 and an unparseable pattern all make OpenAM ignore the whole file, prefix included. "Set this to a value greater than zero to enable" reads as if 0 just disables rotation.
If rotation is enabled with an empty suffix, a suffix that OpenAM detects as too coarse for the rotation settings, or a suffix pattern it cannot parse, or if `org.forgerock.openam.debug.rotation` or `org.forgerock.openam.debug.rotation.maxsize` is set to 0 or less, OpenAM ignores the whole file, including the prefix, and does not rotate debug logs at all. To disable a kind of rotation, leave its value empty. OpenAM checks the suffix against a single interval in the time zone of the server, so it does not catch every pattern that is too coarse; follow the rules above.The WAR ships debugconfig.properties, so the section tells the administrator to edit it and describes its contents again. Keep the suffix line even when rotation is disabled (a missing line causes a NullPointerException, OpenIdentityPlatform#1182), state that zero and negative rotation values are not valid, list the other values that make OpenAM ignore the whole file, and note that the suffix check does not catch every suffix that is too coarse.
|
All three points are addressed in 6b1997f. WAR ships Missing suffix line → NPE. Confirmed from the code ( Other ways the file is rejected. Added: a suffix pattern that cannot be parsed and a rotation value of zero also make OpenAM ignore the whole file, prefix included; the property descriptions now say that zero and negative values are not valid and that an empty value disables that kind of rotation. Also added a paragraph that the suffix is checked against a single interval in the server time zone, so a too coarse suffix can pass and the next interval is then appended to the same file. One deviation from the suggested text: I did not write that every value ≤ 0 makes OpenAM ignore the file. |
maximthomas
left a comment
maximthomas
left a comment
There was a problem hiding this comment.
praise: The rotation section now matches what the loader and the shipped file actually do.
chap-monitoring.adoc:395describes the shippedopenam-server-only/src/main/resources/config/debugconfig.propertiesexactly: empty prefix, suffix and rotation, and noorg.forgerock.openam.debug.rotation.maxsizeline.chap-monitoring.adoc:403tells readers to keep theorg.forgerock.openam.debug.suffixline with an empty value, which avoids theNullPointerExceptionthatDebugConfigurationFromPropertieshits when the key is absent.chap-monitoring.adoc:438matchesvalidateSuffix, which checks a single interval in the server's default time zone.
`DebugConfigurationFromProperties` read the prefix and suffix with `Properties.getProperty(key)`, without a default. A `debugconfig.properties` without the `org.forgerock.openam.debug.suffix` line therefore left the suffix `null`: - with rotation enabled, `validate()` threw a `NullPointerException` instead of `InvalidDebugConfigurationException`, so `DebugFileProviderImpl` did not fall back to the default configuration; - with rotation disabled, the configuration was accepted and `DebugFileImpl` threw on the first write of every debug log. Changes in `openam-shared`: - A missing prefix or suffix line now reads as an empty string. `DebugConfiguration` documents that both are never `null`, and `DebugFileImpl` drops its null check on the prefix. - Values are trimmed. A trailing space used to make the rotation value unparseable, which silently disabled rotation, and ended up in the file name as part of the prefix. - `rotation=-1` is rejected like any other value `<= 0`. It used to collide with the internal "not configured" marker and silently disabled time rotation. The `<= 0` checks for both `rotation` and `rotation.maxsize` now run while parsing. - The error for a rotation value `<= 0` now says how to disable that rotation: "Leave the value empty to disable time rotation." or "Leave the value empty to disable file size rotation." OpenAM drops the whole file in that case, and this one line on stderr is all an administrator sees. The file size message names its unit: "File size rotation = '-2' MB." - A file that `Properties.load()` cannot read (a malformed `\u` escape), or any other runtime failure while loading it, becomes an `InvalidDebugConfigurationException`, so OpenAM falls back to the default configuration. - The next rotation time is computed in `long`. A rotation interval above 35791 minutes overflowed `int`, and the file rotated on every write. Tests: - New: a file without the prefix and suffix lines is accepted, and `debug.error()` writes to a file named exactly after the debug log (`DebugTest.shouldWriteWithoutPrefixAndSuffixLines`); a missing suffix line with time or size rotation is rejected; trailing whitespace; `rotation=-1`; a malformed `\u` escape; a 30-day rotation interval does not rotate after one minute (`DebugRotationTest.longTimeRotation`). - New: `rotation=0` and `rotation.maxsize=0` are rejected, and `rotation.maxsize=2048` does not overflow (`zeroRotation`, `zeroSizeRotation`, `largeSizeRotation`). They pin the `<= 0` bounds and the `(long)` cast before the shift, which no test covered. - Three existing invalid-configuration tests referred to files that do not exist and passed on "Can't find the configuration file". `cantParseSuffix` used `w`, which is a valid `SimpleDateFormat` letter. The tests now point at the existing files, `cantParseSuffix` uses an illegal letter, and every invalid-configuration test asserts the exception message; `negativeSizeRotation` asserts it in full, unit included. - Each new test failed before its fix. The three edge tests fail when either `<= 0` check becomes `< 0` or the `(long)` cast is dropped. `mvn -pl openam-shared clean test`: 1263 tests, 0 failures. Upgrade note: The release notes list only PR titles, so the note is in the upgrade guide (`chap-upgrade-servers.adoc`), as a "Starting with this release" `[IMPORTANT]` block: ```text debugconfig.properties is validated more strictly. A file with org.forgerock.openam.debug.rotation=-1, or with a trailing space after a rotation value next to a suffix that is empty, too coarse or cannot be parsed, is now rejected as a whole: OpenAM prints "'/debugconfig.properties' isn't valid, the default configuration will be used instead" to the standard error output and uses no prefix and no rotation. Leave a rotation value empty to turn that rotation off. Trailing spaces are also removed from the prefix and the suffix, so they no longer appear in debug log file names. ``` The documentation side is #1181, now merged. Its advice to keep the suffix line is no longer needed after this fix, but stays correct for released versions. This PR changes one sentence of it in `chap-monitoring.adoc`: OpenAM ignores the whole file when a rotation property is set to zero or a negative value. The guide named only zero, because `rotation=-1` used to load. It also adds the upgrade note above to `chap-upgrade-servers.adoc`. Fixes #1182
2 participants
The "Rotating Debug Logs" section of the Administration Guide (
chap-monitoring.adoc,#log-rotate-debug) left out the rules thatDebugConfigurationFromPropertiesenforces. In #1180 a configuration withsuffix=-yyyy.MM.dd-HH.mmandrotation.maxsize=1was rejected at startup, and OpenAM fell back to the default configuration with no rotation at all.Changes to the section, checked against
DebugConfigurationFromProperties,DebugFileProviderImplandDebugFileImpl:.warfile shipsWEB-INF/classes/debugconfig.propertieswith an empty prefix, suffix and rotation interval, and without arotation.maxsizeline. The section now describes that file and says to add therotation.maxsizeline for rotation based on file size.-MM.dd.yyyy-kk.mm, butDefaultDebugConfigurationreturns an empty suffix. The suffix is required when rotation is enabled, it must change at every interval for time-based rotation, and it must include milliseconds (SSS) for size-based rotation.NullPointerException(Debug logging throws NullPointerException when debugconfig.properties has no suffix line #1182).Debug.initialize()→DebugProviderImpl→DebugFileProviderImpl). The section claimed that changes take effect immediately; it now says to restart.Documentation only. The section renders without warnings in Asciidoctor.js 3.0.4.
Fixes #1180