osdir.com


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

Re: [imaging] IMAGING-154 remove Debug class


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
> >
>