Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the logging during image import (see #11020) #1251

Merged
merged 12 commits into from Jun 18, 2013

Conversation

bpindelski
Copy link

This PR extends the contents of the import log file created during the upload/import phase. The file can be accessed after the import finishes in Insight (using the View button).

The logback.xml configuration modifications might slightly affect Blitz-0.log and friends (after local comparison, the contents of those log files should be roughly the same before this PR and after).

To test - upload images (different types) and view the import log file - contents should be quite verbose (messages of levels: DEBUG, INFO, WARN and ERROR - latter two not always present).

@ximenesuk
Copy link
Contributor

I've checked this against gretzky and I am getting nicely full logs for a handful of image and SPW formats. The one thing I noticed is that the import log contains only DEBUG level messages. I don't know if this means the filter is too strict or if messages levels are being rewritten or if the logs genuinely do only have that data in them.

@bpindelski
Copy link
Author

@ximenesuk Currently logback.xml is set up with the LevelFilter which does exact matching. I can change it to ThresholdFilter and ask it to catch everything in the import file log that falls into INFO and below. @joshmoore is it worth a test?

@bpindelski
Copy link
Author

Changed the filter settings so that it catches both INFO and DEBUG.

@joshmoore
Copy link
Member

Interesting. I would think we want DEBUG and higher always. I can't think of a case where we would want, say, INFO but not WARN. So across the board we should be using thresholding, I'd say, though in some cases not starting at DEBUG.

@bpindelski
Copy link
Author

@joshmoore So we want DEBUG, INFO, WARN and ERROR in the import log file, yes? If so, then the ThresholdFilter should be able to do it when set to DEBUG.

@joshmoore
Copy link
Member

Sounds like a plan.

@joshmoore
Copy link
Member

Apologies, @bpindelski . I didn't get to this today. Re-adding for tomorrow.

@joshmoore
Copy link
Member

This looks great. Tested via:

$ touch 1251.fake
$ bin/omero import 1251.fake
...
2013-06-12 08:31:05,780 2966       [l.Client-0] INFO   ormats.importer.cli.LoggingImportMonitor  - OBJECTS_RETURNED Step: 5 of 5  Logfile: 2348
2013-06-12 08:31:05,870 3056       [l.Client-1] INFO   ormats.importer.cli.LoggingImportMonitor  - IMPORT_DONE Imported file: /home/omero/slave/workspace/OMERO-merge-develop/src/dist/1251.fake
Imported pixels:
771
Other imported objects:
Fileset:5
Image:771
2013-06-12 08:31:05,871 3057       [l.Client-1] INFO      ome.formats.importer.cli.ErrorHandler  - Number of errors: 0
# Find log id in the output, e.g. 2348
$ bin/omero download 2348 1251.log

The following categories look to be a bit much at DEBUG and could be limited to INFO:

  • ome.annotations
  • ome.logic
  • ome.security
  • ome.tools

@melissalinkert, do you have any thoughts on what we want/need from ome.scifio.* and loci.*. Location is at DEBUG which might be too much. Are there any formats where DEBUG for those package hierarchies would be complete overkill?

@@ -52,10 +53,18 @@ public Object invoke(final MethodInvocation arg0) throws Throwable {
final Map<String, String> ctx = current.ctx;
if (ctx != null && ctx.size() > 0) {
cd.setContext(ctx);
if (ctx.containsKey("omero.logfilename")) {
MDC.put("fileset", ctx.get("omero.logfilename"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need some ideas on how to limit the scope of this, since at the moment anyone will be able to write to whatever log file they like just by passing a value.

@joshmoore
Copy link
Member

Final question, perhaps for @ximenesuk & another PR: what if any of the lines are being duplicated to Blitz-0.log and what options do we have to limit that?

@melissalinkert
Copy link
Member

Location at DEBUG in general is too much - for single files it might be fine, but for larger multi-file datasets it will make a bit of a mess.

Having loci.* (and specifically `loci.formats.tiff.*) atDEBUG`` is also likely to be overkill for pretty much anything TIFF-based, which is the majority of supported formats. Looking at the logs for any non-fake plate should give you an idea of what to expect.

@joshmoore
Copy link
Member

@bpindelski : could you lower to INFO then and we'll gather several logs there?

@bpindelski
Copy link
Author

@joshmoore Updated logback.xml. Tried importing bd-pathway and indeed lowering to INFO in some cases was necessary, as the log file grew in size.

@joshmoore
Copy link
Member

Looking through the logs from today's auto-import:

omero@gretzky:/OMERO/ManagedRepository$ find . -name "*.log"
./user-4_5/2013-06/17/08-37-37.065.log
./user-4_5/2013-06/17/09-24-24.350.log
./user-4_5/2013-06/17/09-21-30.475.log
./user-4_5/2013-06/17/08-38-56.485.log
./user-4_5/2013-06/17/08-40-47.930.log
./user-4_5/2013-06/17/09-24-27.120.log
./user-4_5/2013-06/17/09-20-31.203.log
./user-4_5/2013-06/17/08-38-30.724.log
./user-4_5/2013-06/17/09-23-08.408.log
./user-4_5/2013-06/17/08-39-35.286.log
./user-4_5/2013-06/17/08-41-33.666.log
./user-4_5/2013-06/17/08-41-13.694.log
./user-4_5/2013-06/17/09-24-21.979.log
./user-4_5/2013-06/17/08-39-53.243.log
./user-4_5/2013-06/17/08-39-16.936.log
./user-4_5/2013-06/17/08-47-44.015.log
./user-4_5/2013-06/17/08-40-13.664.log
./user-10_13/2013-06/17/09-52-30.087.log
./user-9_2/2013-06/17/09-46-36.071.log
./user-2_10/2013-06/17/09-29-19.637.log
./user-2_10/2013-06/17/09-29-25.596.log
./user-2_10/2013-06/17/09-29-16.746.log
./user-2_10/2013-06/17/09-58-02.582.log
./user-2_10/2013-06/17/09-29-22.482.log
./user-2_10/2013-06/17/09-58-06.197.log

the files look not overly full with DEBUG but generally usable. There was at least one exception which it would be interesting to look into further (i.e. good data). It's nice that we now have the timestamps for all the actions, which we could optionally start parsing out at some point. 👍

@@ -19,6 +19,12 @@
<pattern>%date %-5level [%40.40logger{40}] \(%10.10thread\) %msg%n</pattern>
</encoder>

<filter class="ch.qos.logback.classic.filter.LevelFilter">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bpindelski: sorry, I missed this while focusing on the import logs. Was this supposed to be changed to a ThresholdFilter?

@joshmoore
Copy link
Member

Tested on gretzky and locally. Both DEBUG and WARN messages are again appearing. This looks great, thanks @bpindelski.

joshmoore added a commit that referenced this pull request Jun 18, 2013
Improve the logging during image import (see #11020)
@joshmoore joshmoore merged commit 605db3f into ome:develop Jun 18, 2013
@@ -219,6 +219,7 @@
import omero.util.TempFileManager;

import org.apache.commons.io.FilenameUtils;
import org.apache.log4j.MDC;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be:

import org.slf4j.MDC;

A bit late for this PR but I'll reference this minor change to an ongoing ticket so it can be included in a suitable PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants