osdir.com


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [imaging] IMAGING-154 remove Debug class


What I've seen done when trying to avoid a logging API dependency is to
create a minimal logging API purely for framework use. This can have a
default System.err style implementation, but the idea is to make it easy
(and performant hopefully) to bridge into the end user's choice of
framework and configuration without actually requiring a real logging
framework (at least until you want to use it in production). While it seems
overkill, the problem is that neither JUL nor System.err are sufficient for
logging. Even a simple API like Android's logging API can be good enough to
abstract it in a small library.

For a look at the very simplest route, you can see how Log4j2 handles
logging before any logging classes have been initialized. It's basically a
configurable wrapper around System.err:
https://github.com/apache/logging-log4j2/blob/master/log4j-api/src/main/java/org/apache/logging/log4j/util/LowLevelLogUtil.java


On Sun, 12 Aug 2018 at 09:12, Gary Gregory <garydgregory@xxxxxxxxx> wrote:

> You could also log to a pluggable print stream which could be sys err by
> default. Kind of like what JDBC allows. My bias is to Log4j 2 as well :-)
>
> Gary
>
> On Sun, Aug 12, 2018, 08:00 Remko Popma <remko.popma@xxxxxxxxx> wrote:
>
> > There’s a couple of considerations about doing logging in a library, but
> > I’ll just mention a few:
> >
> > * dependencies
> > * performance
> > * ease of use
> >
> > * Dependencies*
> > Will the library be less attractive to users if it requires an external
> > dependency? Then don’t introduce one (so: use system err or JUL). On the
> > other hand, if the vast majority of usages is in a context with many
> other
> > external libraries (like in a web container) you have more freedom.
> >
> > *Performance*
> > Please take a look at the log4j 2 performance page (
> > https://logging.apache.org/log4j/2.x/performance.html#tradeoffs).
> Console
> > logging is 50x (yes fifty times) slower than file logging.
> > That’s a strong argument against system err logging. I’m not a fan of
> JUL,
> > but if you need to avoid dependencies you’re better off using JUL, that’s
> > only 5x slower than log4j. Also depends on how much logging you expect to
> > do in the worst case.
> >
> > *Ease of use*
> > I’m biased and would say that Log4j 2 has the nicest and richest API.
> > Console logging (System.err.printf) probably has the poorest API. Other
> > libraries sit in the middle.
> >
> > *Final note*
> > I would never log to System out, always use system err instead. This
> > allows programs using your library to pipe output to other programs
> without
> > their output getting mixed with your library’s diagnostic output.
> >
> > Hope this helps,
> >
> > Remko
> >
> > (Shameless plug) Every java main() method deserves http://picocli.info
> >
> > > On Aug 12, 2018, at 21:21, Gilles <gilles@xxxxxxxxxxxxxxxxxxxxx>
> wrote:
> > >
> > > Hello Bruno.
> > >
> > >> On Sun, 12 Aug 2018 08:56:37 +0000 (UTC), Bruno P. Kinoshita wrote:
> > >> Hi all,
> > >>
> > >>
> > >> I commented on IMAGING-154, but copying the last comment here as it
> > >> contains the approach I would like to follow to fix the last change in
> > >> the project blocking
> > >> a 1.0 release:
> > >>
> > >>
> > >> ---
> > >>
> > >> So went ahead to re-design the Debug class, in a way users could
> > >> still enable/disable debugging, and also use a PrintStream so that
> > >> other thing rather than System.out could be used.
> > >>
> > >>
> > >>
> > >> Then, realized removing System.out was the natural next step. But
> > >> alas, the library uses System.out for debugging, but sometimes it uses
> > >> it for writing to System.out in a "verbose mode". What is more
> > >> complicated, is that sometimes classes methods like `toString()` are
> > >> calling debug methods that receive a PrintStream already.
> > >>
> > >>
> > >>
> > >> So I spent some more time quickly comparing what other libraries I've
> > >> seen being used / or used for image processing:
> > >>
> >
> https://kinoshita.eti.br/2018/08/12/use-of-logging-in-java-image-processing-libraries/
> > .
> > >> Turns out only very low level libraries, such as the JNI bridge for
> > >> OpenCV, im4java, and Java's ImageIO can do with just throwing
> > >> Exception's.
> > >>
> > >>
> > >>
> > >> All other libraries have one way or another of logging. Be it with
> > >> JUL, SLF4J, custom loggers, or with the ol' System.out/err.
> > >>
> > >>
> > >>
> > >> My preferred compromise for this ticket was to keep Debug, making
> > >> System.out possible but optional, and mark the class internal only.
> > >> Now my preferred solution is to keep the Debug internal, but add a
> > >> logger to it. And then also add logging to replace where System.out is
> > >> used for the "verbose" mode.
> > >> ---
> > >>
> > >>
> > >>
> > >> Any thoughts? Objections? If none, I will try to work on this issue
> > >> next weekend, making the Debug class internal only, and replacing
> > >> System.out by a logging utility. After that, we should be good to
> > >> start preparing the vote for 1.0.
> > >>
> > >>
> > >>
> > >> * I know it's hard to get a consensus on having logging in Commons
> > >> components, as we have  normally low level libraries, where using
> > >> logging is not always practical.
> > >
> > > There are Log4j2 experts reading here.  It would be interesting
> > > to hear them about what is practical or not.  There are several
> > > aspects to "practical": simplicity, flexibility, compatibility,
> > > performance, ...
> > > How does Log4j2 fare in these areas?
> > > Is there a known (through experience) limit in where it should
> > > be used?
> > >
> > >> But I would now argue that Java own
> > >> ImageIO is low level. But ImageJ2, Processing, OpenJPEG, and Commons
> > >> Imaging are located at a higher level, some times even using it for
> > >> basic image handling/parsing/reading.
> > >
> > > As with many discussions on this list, conflicting arguments occur
> > > because people lack common (!) definitions.
> > > So one goes: "You cannot do <something> in a low-level component"
> > > but does not define "low-level"...
> > >
> > >> * Feel free to cast a counter-argument for it, but please think
> > >> whether you'd still be -0, +0 for this change. We have delayed 1.0 for
> > >> a while, so if you have a strong opinion on not adding a logger,
> > >> please provide an alternative for IMAGING-154.
> > >
> > >> Otherwise we may fail
> > >> to prepare a 1.0 release yet again, and then somebody else may have to
> > >> work on it in a few months/years...
> > >
> > > We are there because the project is too rigid about itself as
> > > a whole (cf. for example the [RNG] thread about BC).
> > > IMHO, it's not the always least common denominator that is the
> > > best decision...
> > > As you noticed, components most easily stall in their development
> > > for lack of proper review, or risk acceptance (i.e. assume that
> > > those who are closer to the code (at a given time) probably know
> > > best... :-/
> > >
> > > My opinion is that we can take the risk to introduce logging, and
> > > if people complain somehow, take it back later.
> > >
> > >> one possible compromise for this,
> > >> might be i) make Debug internal,
> > >
> > > +1
> > >
> > > [Hmm... Does "internal" mean that minor release can break BC
> > > on such a class?]
> > >
> > >> ii) remove all System.out calls,
> > >
> > > +1 or
> > > -1
> > >
> > > [Depends on what "low-level" means here. "stdout"/"stderr" is
> > > indeed used in low-level utilities but is the intent the same
> > > here?]
> > >
> > >> which means removing the verbose flags, the checks, and calls to
> > >> System.out in there.
> > >
> > > It would be a loss of potentially useful information (e.g. for
> > > debugging).
> > >
> > > Regards,
> > > Gilles
> > >
> > >>
> > >> Thanks!
> > >> Bruno
> > >>
> > >> ________________________________
> > >> From: Bruno P. Kinoshita <brunodepaulak@xxxxxxxxxxxx.INVALID>
> > >> To: Commons Developers List <dev@xxxxxxxxxxxxxxxxxx>
> > >> Sent: Tuesday, 6 February 2018 11:30 PM
> > >> Subject: Re: [imaging] IMAGING-154 remove Debug class
> > >>
> > >>
> > >>
> > >> Hi sebb,
> > >>
> > >>> Another aspect of debugging is ensuring that methods are small and
> > >>
> > >>> easily tested independently.
> > >>> However this is difficult to do, and care must be taken to ensure
> that
> > >>> the public API is not unnecessarily extended..
> > >>
> > >> A very good point.
> > >>
> > >> The parsers in commons-imaging expose some #dump... methods
> > >> (
> >
> https://github.com/apache/commons-imaging/blob/7e7f96857df999175bb614732e13272a82f7962a/src/main/java/org/apache/commons/imaging/ImageParser.java#L794
> > ).
> > >>
> > >> While I can see that parsers may need to dump the data they are
> > >> holding in some structured way for inspecting, reporting, serializing,
> > >> etc, it looks like some other classes were affected by it too. For
> > >> example...
> > >>
> > >>
> > >> A JPEG Segment has a #dump() method
> > >>
> > >>
> > >>
> >
> https://github.com/apache/commons-imaging/blob/7e7f96857df999175bb614732e13272a82f7962a/src/main/java/org/apache/commons/imaging/formats/jpeg/segments/Segment.java#L34
> > >>
> > >>
> > >> which gets defined in each subclass of Segment. It can be confusing
> > >> to have a method such as #dump() in a Segment, from the point of view
> > >> of someone writing a photo editor for example. The user could use that
> > >> to pass his/her own logger's PrintWriter, which would make removing or
> > >> changing logging in the future in commons-imaging.
> > >>
> > >>
> > >> If we keep the Debug class, and make it internal, there would still
> > >> be these methods to take care. And there are some methods where users
> > >> can provide a PrintWriter, while others instead use System.out
> > >> (e.g.
> >
> https://github.com/apache/commons-imaging/blob/7e7f96857df999175bb614732e13272a82f7962a/src/main/java/org/apache/commons/imaging/FormatCompliance.java#L70
> > ).
> > >>
> > >> Cheers
> > >> Bruno
> > >>
> > >> ________________________________
> > >> From: sebb <sebbaz@xxxxxxxxx>
> > >> To: Commons Developers List <dev@xxxxxxxxxxxxxxxxxx>; Bruno P.
> > >> Kinoshita <brunodepaulak@xxxxxxxxxxxx>
> > >> Sent: Tuesday, 6 February 2018 11:06 PM
> > >> Subject: Re: [imaging] IMAGING-154 remove Debug class
> > >>
> > >>
> > >>
> > >> On 6 February 2018 at 09:52, Bruno P. Kinoshita
> > >> <brunodepaulak@xxxxxxxxxxxx.invalid> wrote:
> > >>> Hi Jorg,
> > >>>
> > >>> I'd be fine with that solution too. I think this one would cause the
> > smaller change to the code as is.
> > >>>
> > >>> I believe my preference is still to remove the Debug class. But
> > between logging and making Debug internal only, I'd choose making it
> > internal.
> > >>
> > >> +1
> > >>
> > >> I think making it internal means it can still be dropped later.
> > >>
> > >>> Looking forward to hearing what others think about these options.
> > >>>
> > >>
> > >> Another aspect of debugging is ensuring that methods are small and
> > >> easily tested independently.
> > >> However this is difficult to do, and care must be taken to ensure that
> > >> the public API is not unnecessarily extended..
> > >>
> > >>> Thanks
> > >>> Bruno
> > >>>
> > >>>
> > >>> ________________________________
> > >>> From: Jörg Schaible <joerg.schaible@xxxxxxxxxxxxxxx>
> > >>> To: dev@xxxxxxxxxxxxxxxxxx
> > >>> Sent: Tuesday, 6 February 2018 9:24 PM
> > >>> Subject: Re: [imaging] IMAGING-154 remove Debug class
> > >>>
> > >>>
> > >>>
> > >>> Hi Bruno,
> > >>>
> > >>>
> > >>> if it might also be helpful to our users, why not keep and provide
> it.
> > As
> > >>>
> > >>> I understand it, the Debug class is a tool helping in development to
> > >>>
> > >>> analyze some behavior.
> > >>>
> > >>>
> > >>> Nothing stops us from declaring this class internal (we might even
> put
> > it
> > >>>
> > >>> into a package "internal" or "debug") that might be changed without
> > >>>
> > >>> further comment. Nobody may rely on it in production code, but during
> > >>>
> > >>> development it might be helpful. With such an approach we might not
> > have
> > >>>
> > >>> a need to find a better interface to provide this functionality.
> > >>>
> > >>>
> > >>> Just my 2¢,
> > >>>
> > >>> Jörg
> > >>>
> > >>>
> > >>>
> > >>> Am Mon, 05 Feb 2018 12:20:58 +0000 schrieb Bruno P. Kinoshita:
> > >>>
> > >>>
> > >>>> Hello,
> > >>>
> > >>>>
> > >>>
> > >>>> If memory serves me well, some time ago we had a discussion around
> > >>>
> > >>>> sanselan & commons-imaging 1.0. One of the issues with
> commons-imaging
> > >>>
> > >>>> 1.0 was the Debug class.
> > >>>
> > >>>>
> > >>>
> > >>>> https://issues.apache.org/jira/browse/IMAGING-154
> > >>>
> > >>>>
> > >>>
> > >>>> I finished the pull request, but Gilles raised an important point,
> > about
> > >>>
> > >>>> discussing other alternatives first.
> > >>>
> > >>>>
> > >>>
> > >>>> Initially I am against logging in low level libraries, especially
> > >>>
> > >>>> commons components. But some time ago I had to debug TIFF issues in
> > >>>
> > >>>> commons-imaging, and having the dump methods was a tremendous help.
> > >>>
> > >>>>
> > >>>
> > >>>>
> > >>>
> > >>>> The issue is that some imaging algorithms/processing have a lot of
> > >>>
> > >>>> variables that can be altered. And keeping an eye on all of them in
> > the
> > >>>
> > >>>> debugger can be quite hard - though not impossible.
> > >>>
> > >>>>
> > >>>
> > >>>> So all in all, now I am more confident to proceed without the Debug
> > >>>
> > >>>> class. But some users could have a hard time investigating possible
> > >>>
> > >>>> issues in the library without seeing what's going on within the
> > library.
> > >>>
> > >>>>
> > >>>
> > >>>> IMO, that could be solved with the logging/dump features... or
> > through a
> > >>>
> > >>>> better design, especially around exception handling/throwing. The
> > latter
> > >>>
> > >>>> is my preferred approach. Instead of logging, I prefer - whenever
> > >>>
> > >>>> possible - that low level libraries throw exceptions and let me
> handle
> > >>>
> > >>>> the logging.
> > >>>
> > >>>>
> > >>>
> > >>>>
> > >>>
> > >>>> So, any thoughts? :) I'm +1 to remove the Debug class, and +0 to a
> > >>>
> > >>>> logging added to commons-imaging.
> > >>>
> > >>>>
> > >>>
> > >>>> Bruno
> > >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@xxxxxxxxxxxxxxxxxx
> > > For additional commands, e-mail: dev-help@xxxxxxxxxxxxxxxxxx
> > >
> >
>


-- 
Matt Sicker <boards@xxxxxxxxx>