57 log snapshot #81

binduwavell · 2017-05-07T01:48:47Z

CHECKLIST

We will not consider a PR until the following items are checked off--thank you!

There aren't existing pull requests attempting to address the issue mentioned here
Submission developed in a feature branch--not master

CONVINCING DESCRIPTION

Adds a Start log Snapshot button to the repo Log4J settings page. When clicked this dynamically creates an appender and adds it to the root logger and any other loggers that additivity disabled. This allows us to siphon off log messages into a temporary file.

The button changes to Stop log Snapshot and an Add log entry button is appended along with a text box and focus is moved to the text box.

If Add log entry is clicked without placing any text in the text box a number is logged to a fake logger supporttools.logsnapshot.Lap. Each time this button is pressed when the text box is empty the number is increased and logged. If there is any text in the text box when this button is pressed the text will be logged and the lap number is not increased.

When Stop log Snapshot is pressed the appender is removed from any loggers it was added to and it (and the underlying file) are closed. Then the contents of this file are streamed down to the user. Finally the button changes back to Start log Snapshot and the Add log entry and associated text box are removed.

If the page is reloaded during a snapshot the reference back to the temporary file is lost so there is no way to stop this instance of snapshotting. Similarly if the user starts a snapshot and then forgets to stop, we don't want an indefinite amount of log duplication. As such a customer appender is used. Each time a log entry is provided it checks if it has been open for more than 20 minutes. If so it will deregister from all loggers and close the appender.

If the user simply forgot to stop the snapshot and the appender is removed, when they click the Stop log Snapshot button, the first 20 minutes of log entries will be downloaded.

While focus is on the log entry (lap) text box if enter is pressed it will trigger the Add log entry button, similarly if the Escape key is pressed while this text box has focus, the Stop log Snapshot button is triggered.

RELATED INFORMATION

We created #80 to track the remaining work around batch log changes during the snapshot.

Fixes #57

…code styling

and triggers log download

We use this so snapshot logs don't get leaked. Also allow downloading of as much of a log as was captured even after it has been deregistered and closed.

binduwavell · 2017-05-07T01:57:58Z

...cripts/org/orderofthebee/support-tools/admin/ootbee-support-tools/log4j-loggers.get.html.ftl

@@ -39,7 +39,7 @@ Copyright (C) 2005-2016 Alfresco Software Limited.
    <div class="column-full">
        <div>${msg("log-settings.column.addLogger")}:
            <form id="addPackageForm" action="${url.service}" method="POST" enctype="multipart/form-data" accept-charset="utf-8">
-                <input name="logger" size="35" placeholder="logger-name"></input>
+                <input type="text" name="logger" size="35" placeholder="logger-name"></input>


I added some CSS so that inputs that explicitly have type="text" will be slightly taller than default (to match surrounding buttons) and will have a little left and right padding.

binduwavell · 2017-05-07T01:59:12Z

...ipts/org/orderofthebee/support-tools/admin/ootbee-support-tools/log4j-loggers.get.properties

@@ -8,6 +8,10 @@ log-settings.reset=Reset
 log-settings.hideUnconfigured=Hide unconfigured Loggers
 log-settings.showUnconfigured=Show unconfigured Loggers
 log-settings.rootLogger=(Root Logger)
+log-settings.startLogSnapshot=Start log Snapshot


We have not localized these strings to languages other than English. Are there any steps we should take to make sure these strings get translated?

During the hack-a-thon I asked Eva and Mikel for direct translations. For any new features outside of a context where we have ready access to foreign language speakers it is perfectly fine to just provide English by default. Afterwards we can ping e.g. @douglascr or @angelborroy for translations (or other people for other languages), though in order to simplify this I would ask for bulk translations similar to #58.

@AFaust My username is wrong there. The right one is @douglascrp
Luckily I was reading the comments and found this.

Strange - I thought I used autocomplete for that...

So I'm adding the english properties to the non english properties files with a comment indicating translation is required. I assume we don't have tools that go through all properties files and figure out which properties need translating. If we have such a tool then I should not have done this. If we don't have such a tool, user experience should be the same and a translator can easily see which items they need to translate.

binduwavell · 2017-05-07T02:00:44Z

.../org/orderofthebee/support-tools/admin/ootbee-support-tools/log4j-snapshot-lap.post.desc.xml

+<webscript>
+    <shortname>Log4J Snapshot Lap</shortname>
+    <description>Log4J Snapshot Log Lap Message</description>
+    <url>/ootbee/admin/log4j-snapshot-lap?message={message}</url>


We could allow an optional log level to be specified here, but as that doesn't seem necessary for the current use case, I left it out and figured it could be added without breaking anything later.

An optional log level only makes sense if someone would re-configure the log level of the snapshot lap logger. And currently you are always resetting that to INFO anyway.

The code could auto-configure the log level based on what is passed in. I don't see this as particularly useful for the UI. However, if folks start using these webscripts for sysadmin activities it might be helpful. In any case I'm not planning to do anything here 😄

binduwavell · 2017-05-07T02:02:20Z

...templates/webscripts/org/orderofthebee/support-tools/admin/ootbee-support-tools/log4j.lib.js

+	logLayout = new Packages.org.apache.log4j.PatternLayout('%d{yyyy-MM-dd} %d{ABSOLUTE} %-5p [%c] [%t] %m%n');		
+	snapshotAppender = new Packages.org.orderofthebee.addons.support.tools.repo.TemporaryFileAppender(logLayout, snapshotLogFile);
+	loggers = getLoggersToSnapshot();
+	loggers.forEach(function(logger) {


@AFaust I couldn't find anywhere where you used this annonymous function syntax so I was not certain how you would format this. I ended up using the syntax that's in the jquery table stuff we are using for support tools. Let me know if you feel strongly that this should be changed.

I would use the same formatting rules as with any other function, which in this case basically amounts to "braces on the next line". I would also name such functions as a rule since that would include useful information in Rhino stacktraces in case of errors, especially when scripts are merged using the import tag (which we use abundantly in this addon) and line numbers no longer align.

binduwavell · 2017-05-07T02:04:05Z

...templates/webscripts/org/orderofthebee/support-tools/admin/ootbee-support-tools/log4j.lib.js

+
+    root = Packages.org.apache.log4j.Logger.getRootLogger();
+    level = Packages.org.apache.log4j.Level.INFO;
+    // Fake logger that produces a good log message with the Alfresco default log format


The console logger just shows the last 3 segments of the logger class "supporttools.logsnapshot.Lap" which is why I removed some periods. Since this is a fake logger I hope that is acceptable. The snapshot appender itself includes the fully qualified class name so we can make this more accurate if folks would prefer.

I would prefer using our base Java package name for consistent logger names (in this case ´org.orderofthebee.addons.support.tools.repo´ so we end up with e.g. ´org.orderofthebee.addons.support.tools.repo.logSnapshotLap'). I feel we should not worry about how a specific, pre-defined appender will output the package name - it is more important that the last segment (which should always be present) is sufficiently meaningful. E.g. I may re-route Log4J via SLF4J to Logback and this will only include the initial character of each package level, e.g. `o.o.a.s.t.repo.logSnapshotLap'
We could (should?) include the default INFO level in our log4j.properties.
Why are we not using the explicit info() log method instead?

I will make the suggested change to the fake logger.

Since I want to make sure this logger has the right log level I need the level variable. Given that I figured I'd use the generic log() method. That way we could change the level reference and the code would continue to work. Otherwise if the level is changed to say error, if we used the info() method our log statements would not show up.

I don't feel super strongly about this, but have not made a change given the above justification. If you have a strong feeling about this let me know and I'll change it.

binduwavell · 2017-05-07T02:05:28Z

repository/src/main/amp/web/ootbee-support-tools/js/log-settings.js

+          method : 'GET',
+          fnSuccess : function startLogSnapshot_success(res)
+          {
+              if (res.responseText)


Reviewing this file, this condition should probably be if (res.responseJSON).

AFaust

I left various remarks - none are significant but some should be corrected since they would otherwise be forgotten about in the long run.

AFaust · 2017-05-08T12:30:45Z

...templates/webscripts/org/orderofthebee/support-tools/admin/ootbee-support-tools/log4j.lib.js

+
+    root = Packages.org.apache.log4j.Logger.getRootLogger();
+    level = Packages.org.apache.log4j.Level.INFO;
+    // Fake logger that produces a good log message with the Alfresco default log format


I would prefer using our base Java package name for consistent logger names (in this case ´org.orderofthebee.addons.support.tools.repo´ so we end up with e.g. ´org.orderofthebee.addons.support.tools.repo.logSnapshotLap'). I feel we should not worry about how a specific, pre-defined appender will output the package name - it is more important that the last segment (which should always be present) is sufficiently meaningful. E.g. I may re-route Log4J via SLF4J to Logback and this will only include the initial character of each package level, e.g. `o.o.a.s.t.repo.logSnapshotLap'
We could (should?) include the default INFO level in our log4j.properties.
Why are we not using the explicit info() log method instead?

AFaust · 2017-05-08T12:36:54Z

repository/src/main/amp/web/ootbee-support-tools/js/log-settings.js

+
+    AdminLS.stopLogSnapshot = function stopLogSnapshot()
+    {
+        window.open(serviceContext + '/ootbee/admin/log4j-snapshot-complete/'+snapshotLogFile+'?a=true','_blank');


I wonder - is an explicit window.open really necessary (compared to a simple navigate/href update)? I understand there could be issues due to the text/plain mimetype which a browser might display inline instead, but shouldn't the a=true already handle that via a proper content-disposition in the response? I know we discussed this during the hack-a-thon and I think Ana mentioned that there were some limitations with some browser(s).

I don't know the answer to this question. Given that I tested this and it works. Do object to leaving it as is?

AFaust · 2017-05-08T12:38:30Z

repository/src/main/java/org/orderofthebee/addons/support/tools/repo/TemporaryFileAppender.java

+        {
+            super.append(event);
+        } else {
+            if (!this.closed) {


Minor thing: Inconsistent brace style - "next line" vs. "same line"

Damn tools, another format thing I don't like and my tooling fights me all the way trying to do the thing I tell it not to allow ;) .. In any case I fixed this and the previous constructor that had the same issue.

AFaust · 2017-05-08T12:42:47Z

...ipts/org/orderofthebee/support-tools/admin/ootbee-support-tools/log4j-snapshot-create.get.js

+
+/**
+ * Copyright (C) 2016 Axel Faust / Markus Joos / Bindu Wavell
+ * Copyright (C) 2016 Order of the Bee


Note: It is 2017 alredy so new files can already be created with the correct date. I am far from knowledgable with regards to Copyright attributions to people, but so far I have used the default policy of including only the names of people that have worked on a particular new feature (package). The default "Axel Faust / Markus Joos" was used during the Global Virtual hack-a-thon to simplify things. On new tools that I add I usually only add my name (in addition to OOTBee which should be on every file). For this PR it should probably be the names of all the hack-a-thon participants that contributed.

In this case I worked on this file so I'll just add my name. I'll go through the other files with copyright info and make sure the appropriate folks are listed.

AFaust · 2017-05-08T12:45:33Z

...ipts/org/orderofthebee/support-tools/admin/ootbee-support-tools/log4j-loggers.get.properties

@@ -8,6 +8,10 @@ log-settings.reset=Reset
 log-settings.hideUnconfigured=Hide unconfigured Loggers
 log-settings.showUnconfigured=Show unconfigured Loggers
 log-settings.rootLogger=(Root Logger)
+log-settings.startLogSnapshot=Start log Snapshot


During the hack-a-thon I asked Eva and Mikel for direct translations. For any new features outside of a context where we have ready access to foreign language speakers it is perfectly fine to just provide English by default. Afterwards we can ping e.g. @douglascr or @angelborroy for translations (or other people for other languages), though in order to simplify this I would ask for bulk translations similar to #58.

AFaust · 2017-05-08T12:46:36Z

...cripts/org/orderofthebee/support-tools/admin/ootbee-support-tools/log4j-loggers.get.html.ftl

+				    <@button id="stopLogSnapshot" style="display:none" label=msg("log-settings.stopLogSnapshot") onclick=("AdminLS.stopLogSnapshot();")/>
+				    <@button id="lapLogSnapshot" style="display:none" label=msg("log-settings.lapLogSnapshot") onclick=("AdminLS.lapLogSnapshot();")/>
+				    <input id="lapMessageLogSnapshot" type="text" size="35" style="display:none" placeholder="${msg("log-settings.lapMessageLogSnapshot")}" onkeyup="return AdminLS.handleLogMessageLogSnapshotKeyUp(event);"></input>
+			    </div>


Minor thing: The indent seems off

File has a strange mix of tabs and spaces, I converted tabs to 4 spaces as that seems to make the file look right. I wonder if we should add a lint type check for mixed tabs/spaces.

This may make the file look like we changed stuff we didn't 😢

AFaust · 2017-05-08T12:48:32Z

.../org/orderofthebee/support-tools/admin/ootbee-support-tools/log4j-snapshot-lap.post.desc.xml

+<webscript>
+    <shortname>Log4J Snapshot Lap</shortname>
+    <description>Log4J Snapshot Log Lap Message</description>
+    <url>/ootbee/admin/log4j-snapshot-lap?message={message}</url>


An optional log level only makes sense if someone would re-configure the log level of the snapshot lap logger. And currently you are always resetting that to INFO anyway.

AFaust · 2017-05-08T12:51:28Z

...templates/webscripts/org/orderofthebee/support-tools/admin/ootbee-support-tools/log4j.lib.js

+	var snapshotLogFile, logLayout, snapshotAppender, loggers;
+
+	snapshotLogFile = Packages.org.alfresco.util.TempFileProvider.createTempFile("ootbee-support-tools-snapshot", ".log");
+	logLayout = new Packages.org.apache.log4j.PatternLayout('%d{yyyy-MM-dd} %d{ABSOLUTE} %-5p [%c] [%t] %m%n');		


I wonder if the log pattern should be configurable. Might not be in this iteration but for future improvement. E.g. I might want to check components that use MDC data and need to include the specific keys in the pattern. This is irrelevant for default Alfresco which unfortunately does not use MDC, but might be relevant for custom solutions.

Since I'm not sure how you'd want to pass this configuration in, I'm going to leave it fixed like this. Happy to work on another issue if you write up how you'd like to have that done (although I expect writing it up would take more time than doing it yourself ;) ... of course if you have another CL showing the approach you'd like to use you could just reference that...)

I created #82 to track the idea of a configurable log pattern.

Thanks Axel!

AFaust · 2017-05-08T12:52:11Z

repository/src/main/amp/web/ootbee-support-tools/js/log-settings.js


+    var serviceContext;
+    var snapshotLogFile;
+    var snapshotLapNumber;


Minor thing: not consistent with "one var per fn" style

I hate that convention... However, I agree we should be consistent. I merged serviceContext, snapshotLogFile and snapshotLapNumber into a single var. I left KEYCODE_ENTER and KEYCODE_ESC standalone given the value assignments.

Let me know if you'd prefer this be done differently.

AFaust · 2017-05-08T12:52:48Z

repository/src/main/java/org/orderofthebee/addons/support/tools/repo/TemporaryFileAppender.java

@@ -0,0 +1,97 @@
+package org.orderofthebee.addons.support.tools.repo;


Missing copyright header

AFaust

All things that I do care about have been addressed. We should consider some of the raised styling issues in #83

binduwavell · 2017-05-14T03:09:51Z

Thanks a ton for the reviews Axel!

binduwavell and others added 12 commits April 26, 2017 22:14

First version of log4j-snapshot-create webscript

b416c3b

Spaces to tab

0d25c38

Minor fix, removed a faulty comma in JSON-response

56b14bb

First version of log4j-snapshot-complete webscript

25a6013

Impl. GUI for log snapshot

738d044

Log snapshot is directly downloaded instead of opened in a new tab + …

765a680

…code styling

Initial snapshot lap feature

c03b3fb

Allow keyboard control of lap triggering

a83efd7

Escape in lap text box completes snapshot

294f883

and triggers log download

Improved layout for text boxes on log settings tab

864dc70

Fix a jslint issue that missed previous commit

669b92f

TemporaryFileAppender lives for 20m or less

84898d9

We use this so snapshot logs don't get leaked. Also allow downloading of as much of a log as was captured even after it has been deregistered and closed.

binduwavell requested a review from AFaust May 7, 2017 01:48

binduwavell commented May 7, 2017

View reviewed changes

AFaust requested changes May 8, 2017

View reviewed changes

binduwavell added 2 commits May 13, 2017 14:30

Merge branch 'master' into 57-log-snapshot

bf6f777

Address PR comments

3cc38be

AFaust mentioned this pull request May 13, 2017

Include / improve code style checks #83

Open

AFaust approved these changes May 13, 2017

View reviewed changes

binduwavell merged commit b76a5c4 into OrderOfTheBee:master May 14, 2017

binduwavell deleted the 57-log-snapshot branch May 14, 2017 03:11

		@@ -0,0 +1,97 @@
		package org.orderofthebee.addons.support.tools.repo;

57 log snapshot #81

57 log snapshot #81

Conversation

binduwavell commented May 7, 2017 • edited Loading

CHECKLIST

CONVINCING DESCRIPTION

RELATED INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AFaust left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AFaust left a comment

Choose a reason for hiding this comment

binduwavell commented May 14, 2017

binduwavell commented May 7, 2017 •

edited

Loading