-
Notifications
You must be signed in to change notification settings - Fork 599
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Per-client logging #264
Comments
The problem is that each of the objects underneath the SSHClient has its own logger, so I don't think this will add much. You would normally do this using the MDC (message diagnostic context). So I don't think having a |
Well, the underlying implementation would be more complex, as I would want to set the Loggers for all the classes downstream from the SSHClient class on a per-client basis as well. Using MDC, I would have to inject context information into every sub-logger as well -- which is essentially the same problem, except much harder to use from logging frameworks like JUL. I have never found class-oriented logging to be particularly useful in the context of a connection-oriented system, considering that logging frameworks can generally tell you the class origin of each log message. If I can leave the default logging behavior as-is, would you be open to adding such a feature? |
Well actually I'm not in favour of this. Needing to propagate the same logger throughout the codebase feels like an absolute anti-pattern to me. Do you have an example of any framework/library which does this? The MDC is actually really the correct way of doing this. The MDC is shared in the thread as a ThreadLocal. So there is no need to propagate this to every Logger. From your own app you can just do:
Another option, and one I'm also not fond of, is introducing a wrapper class around the LoggerFactory, which is then used to lookup the loggers. But again this means that all loggers become instance variables, which is not something I'm fond of. |
I was a little confused because every logger in sshj is already an instance variable (only final). If you'd really wanted to enforce the current pattern they should be static final. I haven't studied the threading behavior of sshj. Are there not potentially multiple threads per SSH connection? Or are all the loggers assigned in the current thread in the SSHClient.connect method? Overriding the factory will not work, because org.slf4j.LoggerFactory is declared final. Our library is used for high-volume scanning of machines for vulnerabilities, and we make it possible to create separate log files for each target machine that's being scanned. The utility of this feature for debugging purposes should be obvious. The SSH client we choose years ago, vngx-jsch (a then-improved variant of JSch), had its own interface for logging. Over the course of the past several years, we changed it from using a single instance across the whole library, to an instance per session: Now we want to adopt a more widely-used SSH client, and obviously we want to avoid maintaining a separate fork. There is the option of no longer supporting merging the SSH client logs with the target logs, but ideally, we would continue supporting that existing feature. And somewhat problematically, the MDC data will not be available to log frameworks like JUL (which, being built into Java, is very widely-used), and the whole point of using SLF4J is to avoid forcing log framework requirements onto our users. I'm not quite sure how MDC would help me implement the ability to route log streams from each target to a different file. Off the top of my head, I think it would require me to implement my own loggers to process every message and forward them on depending on the current thread's context settings. But given how SLF4J works (with a single static log framework binding), I'd need to implement loggers for every binding that SLF4J supports. And that would be awful. |
Ok, I get your point. Hmm... I still think the
Then we can also inject the This is also more flexible than manually propagating the 'single' logger through the system, as you could also now on a 'getLogger' call tweak the logger name. Would that design work for you? |
I think that would be perfect! Do you want to implement it, or do you want me to implement it? |
Feel free to start a pr :) I'm currently working on another library (smbj), Op 19 aug. 2016 17:50 schreef "JovalCM" notifications@github.com:
|
Hi Jeroen, where do you envision this LoggerFactory implementation will be injected? Using the net.schmizz.sshj.Config class? Or, statically in some common class? |
It's Jeroen ;), And I would have it injected through the Config instance that is passed 2016-08-19 20:19 GMT+02:00 JovalCM notifications@github.com:
|
Sorry! I edited your name :) That's what I was thinking as well, but I wanted to make sure before marching down that path. |
merged #267, so this is done! |
Would you be amenable to my adding a setLog(org.slf4j.Logger) method to net.schmizz.sshj.SSHClient? (Meaning the log member variable would no longer be final).
When working with hundreds of connections in parallel, I think it's extremely useful to be able to separate out the log events for each one.
By default (i.e., if the setLog method is not called), I'd leave things as-is.
The text was updated successfully, but these errors were encountered: