[JENKINS-68417] Revert `LogRecord` tricks #6643

jglick · 2022-06-09T21:16:01Z

Reverts #6018 and #6449; keeping synchronization simplifications from #6018, the correction to those in #6044, and (slightly amended) test coverage. All “published” log records (INFO and above, or finer if in a custom logger) are retained, but if there are parameters with a message format, that format is expanded at publishing time so that the parameters can be discarded, solving any memory leak issues at the cost of a bit of extra CPU work for custom loggers which the admin never actually looks at. jenkinsci/workflow-cps-plugin#557 seems to suffice.

Proposed changelog entries

Reverting prior attempts to make log records collectible.

Maintainer checklist

Before the changes are marked as ready-for-merge:

There are at least 2 approvals for the pull request and no outstanding requests for change
Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
Changelog entries in the PR title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood
Proper changelog labels are set so that the changelog can be generated automatically
If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

…eference`s

core/src/main/java/hudson/util/RingBufferLogHandler.java

jglick · 2022-06-09T21:18:49Z

core/src/main/java/hudson/util/RingBufferLogHandler.java

+            }
+        }
+        synchronized (this) {
+            int len = records.length;


hide whitespace in diff

…gRecord-JENKINS-68417

…reformatting on agents (jenkinsci#4912)

…in CI, not locally, but anyway Javadoc says a `null` `LogRecord` is permitted, and test should not bypass the interesting logic

core/src/main/java/hudson/util/RingBufferLogHandler.java

basil

Very nice! The original motivation behind #6018 was BOM/PCT failures, so I think we should run BOM/PCT on this to ensure that the partial revert of #6018 does not expose any old BOM/PCT failures again.

test/src/test/java/jenkins/model/JenkinsLogRecordsTest.java

basil · 2022-06-10T21:16:28Z

core/src/main/java/hudson/util/RingBufferLogHandler.java

+        if (record == null) {
+            return;
+        }


A call to super.isLoggable(record) does this null check, and it also checks whether the LogRecord has an appropriate level and whether it satisfies any filter. In practice, the level check is handled by hudson.logging.LogRecorder.Target#matches and subclasses are unlikely to implement a filter, but calling super.isLoggable(record) here results in a slight deduplication of code and makes this subclass of Handler consistent with the subclasses of Handler provided by the Java Platform, so I think this is worth doing.

Suggested change

if (record == null) {

return;

}

if (!super.isLoggable(record)) {

return;

}

I will have to double-check, but I think this is what we do not want to do because this would prevent custom fine loggers from getting anything. Regular handlers either print a message to a sink or do not; handlers which collect all messages need to behave differently. https://github.com/jenkinsci/support-core-plugin/blob/7e75fdc7e9af0abe0d7d659a3e6e591c3451288f/src/main/java/com/cloudbees/jenkins/support/SupportLogHandler.java#L128-L135

This would prevent custom fine loggers from getting anything

I do not see how it could because we never mutate java.util.logging.Handler#logLevel from its default value of ALL in RingBufferLogHandler. Like I wrote earlier, I do not think there is a difference in practice, but I think using the built-in method better adheres to the parent class's extensible design just in case a theoretical subclass of RingBufferLogHandler wanted to use a custom level or filter.

It appears to work. Still, I would rather not do it here because

I am not entirely confident it is correct.

It is not much related to the rest of the change. (I merely added the null check after noticing that tooMuchRecordsShouldNotCrashHandler was not testing anything.)

a theoretical subclass of RingBufferLogHandler

I would rather just make it final than consider such possibilities.

I would rather just make it final than consider such possibilities.

I am OK with making it final.

core/src/main/java/hudson/util/RingBufferLogHandler.java

jglick · 2022-06-10T21:44:53Z

we should run BOM/PCT on this

Yeah, would not hurt. Pretested the known relevant issue—see jenkinsci/support-core-plugin#376.

jglick · 2022-06-12T12:31:19Z

My timebox on this issue is expiring. Considering throwing it all out (just keeping the reversion parts) and instead avoiding large record parameters on a case-by-case basis.

…gRecord-JENKINS-68417

jglick · 2022-06-28T20:21:02Z

Seems I had run into a leak with LogRecord.parameters years ago, acc. to a comment in jenkinsci/workflow-cps-plugin#41.

basil

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process. Thanks!

Co-authored-by: Basil Crow <[email protected]> (cherry picked from commit 75b4b31)

[JENKINS-68417] Eagerly format LogRecord rather than holding `SoftR…

0953300

…eference`s

jglick commented Jun 9, 2022

View reviewed changes

jglick mentioned this pull request Jun 9, 2022

[JENKINS-68417] Revert LogRecord tricks jenkinsci/support-core-plugin#376

Merged

jglick added 3 commits June 10, 2022 13:29

Merge branch 'master' of https://github.com/jenkinsci/jenkins into Lo…

e0a03e1

…gRecord-JENKINS-68417

LogRecorderManagerTest.loggingOnSlaves failure reminds me to skip p…

d4669e0

…reformatting on agents (jenkinsci#4912)

RingBufferLogHandlerTest.tooMuchRecordsShouldNotCrashHandler fails …

e5c3474

…in CI, not locally, but anyway Javadoc says a `null` `LogRecord` is permitted, and test should not bypass the interesting logic

timja reviewed Jun 10, 2022

View reviewed changes

core/src/main/java/hudson/util/RingBufferLogHandler.java Outdated Show resolved Hide resolved

basil approved these changes Jun 10, 2022

View reviewed changes

jglick marked this pull request as draft June 12, 2022 12:29

Merge branch 'master' of https://github.com/jenkinsci/jenkins into Lo…

382ad64

…gRecord-JENKINS-68417

jglick mentioned this pull request Jun 28, 2022

[JENKINS-68417] Use Supplier overloads to avoid leaks from LogRecord.parameters jenkinsci/workflow-cps-plugin#557

Merged

Giving up on preformatting

8e02298

jglick changed the title ~~[JENKINS-68417] Eagerly format LogRecord rather than holding SoftReferences~~ [JENKINS-68417] Revert LogRecord tricks Jun 28, 2022

jglick marked this pull request as ready for review June 28, 2022 21:14

timja requested a review from basil June 28, 2022 21:23

jglick requested a review from a team June 30, 2022 11:42

jtnord approved these changes Jun 30, 2022

View reviewed changes

Merge branch 'master' into LogRecord-JENKINS-68417

64a8fab

basil approved these changes Jul 5, 2022

View reviewed changes

basil added regression-fix Pull request that fixes a regression in one of the previous Jenkins releases ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback labels Jul 5, 2022

Merge branch 'master' into LogRecord-JENKINS-68417

b572e70

timja merged commit 75b4b31 into jenkinsci:master Jul 6, 2022

jglick deleted the LogRecord-JENKINS-68417 branch July 6, 2022 12:40

NotMyFault pushed a commit to NotMyFault/jenkins that referenced this pull request Jul 23, 2022

[JENKINS-68417] Revert LogRecord tricks (jenkinsci#6643)

e334c64

Co-authored-by: Basil Crow <[email protected]> (cherry picked from commit 75b4b31)

jglick mentioned this pull request Oct 24, 2023

Avoid capturing Request in LogRecord parameters jenkinsci/remoting#694

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-68417] Revert `LogRecord` tricks #6643

[JENKINS-68417] Revert `LogRecord` tricks #6643

jglick commented Jun 9, 2022 •

edited

Loading

jglick Jun 9, 2022

basil left a comment

basil Jun 10, 2022

jglick Jun 10, 2022

basil Jun 10, 2022

jglick Jun 28, 2022

jglick Jun 28, 2022

basil Jul 5, 2022

jglick commented Jun 10, 2022

jglick commented Jun 12, 2022

jglick commented Jun 28, 2022

basil left a comment

[JENKINS-68417] Revert LogRecord tricks #6643

[JENKINS-68417] Revert LogRecord tricks #6643

Conversation

jglick commented Jun 9, 2022 • edited Loading

Proposed changelog entries

Maintainer checklist

jglick Jun 9, 2022

Choose a reason for hiding this comment

basil left a comment

Choose a reason for hiding this comment

basil Jun 10, 2022

Choose a reason for hiding this comment

jglick Jun 10, 2022

Choose a reason for hiding this comment

basil Jun 10, 2022

Choose a reason for hiding this comment

jglick Jun 28, 2022

Choose a reason for hiding this comment

jglick Jun 28, 2022

Choose a reason for hiding this comment

basil Jul 5, 2022

Choose a reason for hiding this comment

jglick commented Jun 10, 2022

jglick commented Jun 12, 2022

jglick commented Jun 28, 2022

basil left a comment

Choose a reason for hiding this comment

[JENKINS-68417] Revert `LogRecord` tricks #6643

[JENKINS-68417] Revert `LogRecord` tricks #6643

jglick commented Jun 9, 2022 •

edited

Loading