osdir.com


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

Re: [imaging] IMAGING-154 remove Debug class


If you want to avoid a dependency I would not create a custom logging abstraction but just use JUL. Most logging libraries have JUL adapters so clients can do the bridging on their side. 

(Shameless plug) Every java main() method deserves http://picocli.info

> On Aug 13, 2018, at 0:17, Matt Sicker <boards@xxxxxxxxx> wrote:
> 
> 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>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@xxxxxxxxxxxxxxxxxx
For additional commands, e-mail: dev-help@xxxxxxxxxxxxxxxxxx