Repository navigation
Decide how to support logging errors and warnings #13
Description
Many ops in the old imagej-ops framework, such as this one or this one attempt to log errors or warnings. Sometimes this is in place of throwing some RuntimeException (as in the case of the former) or other times it is just saying something about the data (as in the case of the latter). We need to find a consistent way to deal with the situations in which we would like to pass on messages/errors.
Regardless of the option, no messages should be allowed as secondary outputs.
Potential solutions:
- Use Scijava's
Logger - Create dedicated error subclasses (although this will only solve the issues with errors, not with the logging)
- Not allow any sort of logging
Activity
ctrueden commentedon May 15, 2019
Current thinking is:
OpExecutionException(subclass ofRuntimeException).Loggerclass. But we will wait and use the scijava-v3-log module once it exists. For now, we can make a temporary staticorg.scijava.ops.Logutility class, which will be easy to migrate away from later.ctrueden commentedon May 16, 2019
See also scijava/scijava-common#319 (comment)
gselzer commentedon May 13, 2021
Time commitment: 1 week
gselzer commentedon May 19, 2021
Condensed from a lengthy Gitter discussion with @ctrueden :
Ops should not be bloated by the need to pass a
Loggerthrough theOpEnvironment, to individual Ops, etc. This would require longer signatures, require more significant signature changes in the event we change API, etc. Furthermore, we need to be able to support more than a singleLoggerclass across different Ops/OpEnvironments.Therefore, Ops should rely on a static
LoggingObject able to return aLoggerfor a givenObject(for our purposes it would be an Op, but there is no reason not to be more general here) using a syntax along the lines ofLogger log = Logging.getMyLogger(this). The underlying structure should be some sort ofConcurrentHashMap<Object, Logger>able to return a defaultLoggingObject forObjects not in theMap. Ideally, theMapwould consist ofWeakReferences, clearing entries for Objects no longer in scope. We need to make sure that different instances of the same Op return different Logger`s, for all types of Op.OpFieldis most likely to present issues; what happens if two different procedures want to call the same OpFieldusing differentLoggers? What if those procedures use the sameOpEnvironment?MapMakerseems like it is what we'd want to use, but that would require a Guava dependency. When this logic is moved toorg.scijava.v3.logor whatever that module becomes, we'd ideally want all scijava projects to use it. This Guava dependency would then be a major con. Maybe we can useMapMakeras inspiration...We also noted that it would be ideal for
OpHints(see scijava/incubator#26) to function in a similar "quasi-static" manner. We should probably create some sort ofStateobject that can extensibly provide state information to an Op (including logging behavior,Hints, etc.)gselzer commentedon Jul 25, 2023
I'm going to close this issue with #106, as that PR introduces a static utility class
Loggingthat allows us to direct all logging output through a sharedLoggerimplementation of the user's choice. That solves the majority of this issue.Without any more context I can't think of a situation where we'd want a different
Loggerfor differentObjects (not that I'm opposed to implementing that if a use case arises). Can you think of a reason we might want this @ctrueden? If not, I say we close this as not planned.ctrueden commentedon Jul 25, 2023
Yeah, logging is definitely one of those areas that people tend to overengineer. There are valid reasons for wanting to log, for example, only a single op at debug, but everything else at info or warn, because you really just want to debug that one op implementation. Being able to do this without changing/rebuilding any code can be really important in some deployment scenarios. There are also valid reasons for wanting to have a different logger used per
OpEnvironment, in case you have some complicated server-side JVM running multiple separate contexts. The design of SciJava Common accommodated this, at the expense of everything needing to pass the Context object around all the time—when you use static, you are always JVM-wide unless you get clever with class loaders, which you can't always do.From an architecture standpoint, I can't say that I love having a static Logging utility class—I think it limits the utility of logging—and we might be better off just going with a "standard best practice" logging approach that doesn't try to be too clever about reducing boilerplate. E.g. just suck it up and put the SLF4J Logger declaration at the top of each class that wants to do logging, I dunno...
Can we punt on this one till a little later? I'd like to resolve this before 1.0.0, but we don't need to resolve it urgently before doing things like beta testing.
hinerm commentedon Aug 11, 2023
Agreed. It looks like this was originally suggested here.
@ctrueden note the questions here. Let's just keep their discussion in this issue.