Item 7554: part 2 of log4jupgrade #1423

labkey-ankurj · 2020-07-22T17:19:21Z

Rationale

This is part two of log4j upgrade work, includes changes to custom appenders, event id handling for server javascript console and admin logging UI changes

Related Pull Requests

Item 7490: Upgrade to Log4J 2 - Part 1 #1378

labkey-jeckels · 2020-07-22T17:39:33Z

api/src/org/labkey/api/util/SafeFileAppender.java


+    private void logJobMessage(LogEvent logEvent, @Nullable Throwable t)
+    {
+        if (logEvent.getLevel().compareTo(Level.DEBUG) == 0)


Can these use equals() instead of compareTo()?

And while we seldom use it, this is missing the TRACE level

Should I add the trace level (It wasn't there earlier.)? Are there pipeline jobs that have the trace logging ?

Here is old code in Pipeline.java

OK, but let's make the new code better than the old. :)

I doubt anyone's using trace now (since it doesn't work, apparently), but let's support it. I added trace level logging for some of the recent ODBC work, so I think there are legit use cases within our codebase

sounds good, added it.

labkey-jeckels · 2020-07-22T17:42:55Z

core/src/org/labkey/core/admin/AdminController.java

-            LogEvent[] events = SessionAppender.getLoggingEvents(getViewContext().getRequest());
-            ArrayList<Map<String, Object>> list = new ArrayList<>(events.length);
-            for (LogEvent e : events)
+            Map<LogEvent, String> events = SessionAppender.getLoggingEvents(getViewContext().getRequest());


@labkey-martyp if you haven't already looked at this, can you make sure that it doesn't break the ETL logging scenario that you recently worked on?

labkey-jeckels

Looks good, but let's wait for Marty confirm on the JS console logging for the ETL scenario if you haven't already connected

api/src/org/labkey/api/util/LoggerWriter.java

labkey-jeckels · 2020-07-24T16:10:58Z

api/src/org/labkey/api/util/FileLoggerWriter.java

+    public void writeToFile(File file)
+    {
+        // Make sure that we try to mount the drive (if needed) before using the file
+        NetworkDrive.exists(file);


Since we only have a single use case, I recommend having the constructor open the BufferedWriter, adding a close() method, and calling it after all of the write() calls are done. That will make it so that you don't have to keep opening and closing the file for every line that gets written

labkey-martyp

Sorry, my delay getting to this probably got your branches out of sync with develop. I'm having difficulty getting this to build and looks like its not building on TC. Let me know if I can be any help fixing the build, but assume you can do it a lot quicker for this branch.

labkey-martyp · 2020-07-27T04:33:43Z

api/src/org/labkey/api/util/SessionAppender.java

+@Plugin(name = "SessionAppender", category = Core.CATEGORY_NAME, elementType = Appender.ELEMENT_TYPE, printObject = true)
 public class SessionAppender extends AbstractAppender
 {
+    private static ThreadLocal<WeakHashMap<LogEvent, String>> _eventIds = new ThreadLocal<>();


Is this being used?

labkey-martyp · 2020-07-27T04:37:05Z

api/src/org/labkey/api/util/SessionAppender.java

        final String key;
        boolean on;
-        final List<LogEvent> list = new LinkedList<>();
+        final Map<LogEvent, String> eventIdMap = new WeakHashMap<>();


I think this should probably be a ConcurrentReferenceHashMap. These can be accessed from multiple threads, so that will at least make the map operations thread safe. See appenderInfos implementation of that map type

Possibly something like this? Still not great to just randomly remove index(0) but not sure how to get the eldest entry without using a LinkedHashMap, which does not have a weak reference option.

Suggested change

final Map<LogEvent, String> eventIdMap = new WeakHashMap<>();

final Map<LogEvent, String> eventIdMap = new ConcurrentReferenceHashMap<>(16, ConcurrentReferenceHashMap.ReferenceType.WEAK)

{

@Override

public String put(LogEvent key, String value)

{

// Safeguard against runaway size. ConcurrentReferenceHashMap does not have removeEldestEntry so have

// to randomly remove an entry

if (size() > 1000)

remove(0);

return super.put(key, value);

}

};

labkey-martyp · 2020-07-27T04:40:25Z

api/src/org/labkey/api/util/SessionAppender.java

+            info.eventIdMap.put(event, String.valueOf(++info.eventId));
+            if (info.eventIdMap.size() > 1000)
+                info.eventIdMap.remove(0);


This is something I should have fixed, but would probably be safer to override the put() function of that map with this check.

See suggestion above

labkey-ankurj added 6 commits July 15, 2020 09:31

Item 7554: modify SafeFileAppender for pipeline jobs

c55fd85

Item 7554: OutputLogger level messages

06cf93a

Item 7554: Update eventId handling for server-side JavaScript console

d27f392

Item 7554: remove MXBeanAppender

563f80d

Merge branch 'fb_log4J_upgrade' into fb_log4J_upgrade_2

d2c2e73

Item 7554: Admin Console logging UI changes

31c2f68

labkey-ankurj requested a review from labkey-jeckels July 22, 2020 17:19

labkey-jeckels requested changes Jul 22, 2020

View reviewed changes

This was referenced Jul 22, 2020

Item 7554: NOPLogger update LabKey/DiscvrLabKeyModules#50

Merged

Item 7554: upgrade to log4j2 LabKey/labkey-api-java#9

Merged

Item 7554: LuminexRunAsyncContext test in commonAssays LabKey/commonAssays#217

Merged

labkey-ankurj added 2 commits July 22, 2020 15:07

code review feedback - use equals

dc0e99f

add support for trace

9ee0f9c

labkey-ankurj requested a review from labkey-jeckels July 22, 2020 22:55

labkey-jeckels approved these changes Jul 22, 2020

View reviewed changes

labkey-ankurj added 2 commits July 23, 2020 13:18

Item 7554: code review feedback - custom logger writer

52c9415

cr feedback changes

889396d

labkey-jeckels requested changes Jul 24, 2020

View reviewed changes

labkey-ankurj added 2 commits July 24, 2020 10:32

Item 7554: code review feedback - close writer after dumping threads

752148d

add file name in logs

7fd93a1

labkey-jeckels approved these changes Jul 24, 2020

View reviewed changes

Item 7554: code review feedback - autocloseable logger writer

1e707d5

labkey-martyp requested changes Jul 27, 2020

View reviewed changes

labkey-ankurj merged commit deb307e into fb_log4J_upgrade Jul 27, 2020

labkey-ankurj deleted the fb_log4J_upgrade_2 branch July 27, 2020 18:47

labkey-ankurj mentioned this pull request Jul 28, 2020

Item 7490: Upgrade to Log4J 2 - Part 1 #1378

Merged

-        final Map<LogEvent, String> eventIdMap = new WeakHashMap<>();
+        final Map<LogEvent, String> eventIdMap = new ConcurrentReferenceHashMap<>(16, ConcurrentReferenceHashMap.ReferenceType.WEAK)
+        {
+            @Override
+            public String put(LogEvent key, String value)
+            {
+                // Safeguard against runaway size. ConcurrentReferenceHashMap does not have removeEldestEntry so have
+                // to randomly remove an entry
+                if (size() > 1000)
+                    remove(0);
+                return super.put(key, value);
+            }
+        };

Item 7554: part 2 of log4jupgrade #1423

Item 7554: part 2 of log4jupgrade #1423

Uh oh!

Conversation

labkey-ankurj commented Jul 22, 2020

Rationale

Related Pull Requests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

labkey-ankurj Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

labkey-jeckels left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

labkey-martyp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

labkey-ankurj Jul 22, 2020 •

edited

Loading