remove slf4j dependency? about tls-channel HOT 12 CLOSED

with another thought on the subject, i recommend removing logging completely, from the library all together.

from tls-channel.

i think we will just use shading instead.

from tls-channel.

it seems shading does not work well enough for slf4j providers even though the api itself is shadeable, therefore reopening this and PRs.

from tls-channel.

Hi Mikko,

Thanks for using the library and bringing this topic. This is something that I never thought about, and I am a bit split, to be honest.

On one hand, logging was useful in debugging the library itself, and I think there is value in those statements, even if usually deactivated. Additionally, there is an optional (but in the same Jar) functionality in tlschannel.async that is higher level (i.e., creates threads) and probably needs logging.

Moving to java.util.logging seems to just move the problem: what if someone is creating an appender for that framework?

On the other hard, yes, this is a low-level library and should be usable anywhere.

In any case, I am curious about how the problem materializes. You write that "this causes a circular problem because slf4j provider is not yet available when tls-channel tries to use it". Which is the actual problem? Of course, I am assuming that logging is disabled for tlschannel (that would be clearly some sort of infinite loop).

from tls-channel.

Hi Mariano,

Thank you for the library, it is really convenient drop-in solution for our use case.

We share the same view on the usefulness of logging for debugging, and that's why we have actually created logback, log4j, java.util.logging (jul) and log4j2 appenders for the reliable event logging protocol (relp).

Definitely the higher level api should have logging available. I share the same view and on the pr #140 where I propose removing logging complete I did not bear to remove the useful printout on https://github.com/marianobarrios/tls-channel/pull/140/files#diff-3430cf034d48177fff61969d06b34add371460990a74dac971db6041233fb82fR503 and converted it to System.err.println, which itself is neither a good solution but just a workaround. However I felt that it is more appropriate to indicate a bad usage and by fixing usage the println would not trigger.

We have not yet tried out what happens with java.util.logging handler (https://github.com/teragrep/jla_04) (appenders are called handlers in that context) with the said solution, however it is more likely due to the nature of java.util.logging that we could instruct the users of the library not to configure the handler for our libraries, jla_04, rlp_01 and tls-channel as it would even be obvious for them that using the handler for the handler itself is not really something that can work. This leaves however still the possibility that one may configure everything with the same handler and configure default logging level which would emit logging events from the said libraries and the problem rises again.

At least SLF4J configured with Logback causes following to appear when configured with SLF4J in the appender:

SLF4J: A number (1060) of logging calls during the initialization phase have been intercepted and are
SLF4J: now being replayed. These are subject to the filtering rules of the underlying logging system.
SLF4J: See also http://www.slf4j.org/codes.html#replay

As seen from the link since 1.7.15 SLF4J has tried to address the case for appending before logging is set up. I think they were more concerned about other cases than actual SLF4J implementations using the SLF4J itself.

We offer drop-in solution by just having the appender jar and it's dependencies in the classpath having a requirement for a version of SLF4J is hindering.

Another and really important topic for us is the reliability of the logging. As seen from the SLF4J printout the calls will be delayed and from the alternative https://www.slf4j.org/codes.html#substituteLogger the logging system is not transactional while being initialized. While reliable event logging protocol by the definition is lossless having this kind of printout appear just because that the implementation itself caused will cause users to suspect the system itself. Of course the application code using the SLF4J is capable of causing the same printout but then the cause is real and not by the logging implementation itself.

Thank you for reaching us out!

With kind regards,
Mikko

from tls-channel.

Hi Mikko,

Thanks for your message and sorry for the delay. After quite some thinking, I want to report this to the SLF4J/Logback project. It just seems hard to believe that in so many years of very widespread use, they did not hit this problem.

from tls-channel.

Hi Mikko,

After some tests and consideration, I convinced myself that SLF4J is just problematic for logging from appenders (for the reasons you stated). However, I still think logging is important so, based on your branch, I created this, which also migrates the tests to Java Logging. What do you think? your comments are welcome.

Best regards.

from tls-channel.

Hi Mariano,

There are still some FINER level logging calls present in the branch. While making it I thought to preserve the trace/debug difference however the readme now states that they are only FINEST.

https://github.com/marianobarrios/tls-channel/pull/150/files#diff-c33ab674c05c1d03502d1ab4cc621bb3bc149e30b96cfae35f078cfd4ca33469R637
https://github.com/marianobarrios/tls-channel/pull/150/files#diff-cc209db5f8ac176acd485862acdfababb382229cb0beeee1fbc2f38b890a9fa0R88-R91

I think they could be changed to FINEST to match the proposed behaviour.

Now that I think this after some time off from the keyboard, would it be possible to further enhance the logging so that it can be disabled all together for the library? One possibility is to use

                logger.setFilter(new Filter() {
                    @Override
                    public boolean isLoggable(LogRecord logRecord) {
                        return false;
                    }
                });

Where a user could just ship the Filter to be used for all classes. This way the library could take advantage of different logging levels and user would have the possibility still to shutdown the logging or configure the filter to their liking.

Java provides https://docs.oracle.com/javase/8/docs/api/java/util/logging/LogManager.html which can configure the Filters, however for appender usage it's bit problematic because one only supply either, configuration class or configuration properties and I would like to just override the tls-channel's configration by using a configuration class but this then restricts usage of the properties which are quite handy for the end user. I can think of an alternative such as accessing all the tls-channels loggers by configuring them from our appender code but that too is a bit of a dirty workaround.

Logger.getLogger(TlsChannelImpl.class.getName()).setFilter(...);
Logger.getLogger(xxx.class.getName()).setFilter(...);
Logger.getLogger(yyy.class.getName()).setFilter(...);
Logger.getLogger(zzz.class.getName()).setFilter(...);

Please share your thoughts about it.

Kind regards,
Mikko

from tls-channel.

Thanks for the review. Let's go step by step: I updated the branch and fixed the logging level.

from tls-channel.

About Java Logging: I will be honest, I tried to use it for tests, and could not make it work the way I wanted. As you can see in the tests, it's just sent to SLF4J, and everything controlled in that framework. I would like to make logging configuration out-of-scope if possible... That said, is what you said something that cannot be done by the use of the library, in case they want to use Java Logging?

from tls-channel.

Please allow me to explain it bit further:

Java Util Logging provides means to configure the framework with two methods via the LogManager class. The LogManager class loads either user supplied configurator class that sets the framework's configuration or if that is not present it will use logging.properties which can be user supplied as well. Not both, meaning we as a Java Util Logging Handler (appender) provider can not override things programmatically and leave rest for the properties configuration.

This is acceptable in most circumstances and will allow the separation-of-concern approach regarding the configuration. However for a Java Util Logging Handler (appender) use case it might make sense to provide a method to disable the emission of log events completely by installing a Filter (or boolean that toggles logging off all together). If the library would have such capability, it would allow the user to configure the Handler to be used even with FINEST for all logging without impacting the Handler's ability to forward the data.

With current changes I think we will be able to cover all the cases except the user story of "allow configuring all logging to FINEST level" which is bit of a corner-case most likely, but as our users have large and varying code bases we are really keen on the details how everything behaves even in the most exciting configurations.

from tls-channel.

Thanks Mikko. I see. But my question would be: does something prevent your own apender from doing this (knowing that the loggers all start with "tslchannel")?

from tls-channel.