Add Additional Listener(s) for Logging

[mdemille]

Per a Slack conversation with @sasbury , I'm creating this issue.

In summary, while troubleshooting some issues in our projects, we've run in to some problems where we've found ourselves largely blind as to what exactly is going on under the covers because of the lack of debug and/or trace logs. We were going to add SLF4J logging, but found that it was removed here: #60

In short, per @sasbury 's suggestion, we want to add 1 or more additional listeners so that we can at least tie in to all of the log points that were removed in the aforementioned PR. Beyond what was removed there, we're not sure off-hand of any information we need.

Full text of the Slack conversation is included below

Mike DeMille:
Hello nats team.

We’re a java shop and use the nats streaming java client. We started using nats a few months ago, and really like it, but we’ve noticed that there seems to be some blind spots in the client - in particular surrounding logging and metrics. So we put in a ticket for us to contribute back some logging and metrics to the java client, but we found a PR/commit from 3 years ago where the type of logging we were planning on putting in was actually ripped out. https://github.com/nats-io/stan.java/pull/60

Would anyone know why that was done? Or if you guys would be open to putting it back in (whether we do it or you do it is fine)?

Stephen Asbury  2 hours ago
in general we (and in particular I for Java) have tried not to have extensive logging/monitoring in the client itself because it places a lot of subjective choices in the library

Stephen Asbury  2 hours ago
even the choice of library would be a thing, i suspect

Stephen Asbury  2 hours ago
that said, I have done some logging in the core client during the connect phase based on an option, to help with debugging
:+1:
1


Stephen Asbury  2 hours ago
can you give a few examples what you are thinking?

Mike DeMille  2 hours ago
@Jon Pendlebury or @Noah could probably speak to that better than I could, but I think basically reverting that commit seemed like a good place to start.
Only visible to you

Slackbot  2 hours ago
Ok! I've sent @Noah a link to your message.

Stephen Asbury  2 hours ago
reverting would add a dependency which may not match everyones logging library choice/version - we have tried very hard to reduce dependencies across all client libraries, and especially Java

Stephen Asbury  2 hours ago
some of these logging calls are inside catch blocks - perhaps we need more use of the listeners that exist (or a new one)
:+1:
1


Mike DeMille  2 hours ago
The dependency point makes some sense, I suppose, though it seems like if you used an industry-standard library, the majority of people wouldn’t have a problem.
But regardless of that, it would be nice to have some solution to get those logs rather than System.out or System.err . Right now we just feel like we’re completely blind
:+1:
1


Mike DeMille  2 hours ago
I don’t really care how you do it per-se, I just don’t want to be blind :slightly_smiling_face:

Jon Pendlebury  2 hours ago
Originally it had SLF4J and that is just a logging facade that can be used with some different frameworks.

Jon Pendlebury  2 hours ago
But yeah, basically it would be nice to not have it be such a blackbox.
:+1:
1


Mike DeMille  1 hour ago
So what are you thinkin’, @sasbury? We’d love to help contribute if we can so that we don’t have to maintain a fork forever… But some degree of increased visibility would be nice, if possible.

Stephen Asbury  38 minutes ago
i guess i would say that my understanding for all of the NATS clients is that they are essentially black boxes, modulo callbacks/listeners. So if there is a good way to give you the information you need via the ConnectionListener or ErrorListener that is the best choice. Stuffing logging into the library is a very opinionated set of choices that I don’t think belong in the library. Which I guess is my way of saying - i dont think we should add logging - but i do want to help.

Mike DeMille  11 minutes ago
So, I’m trying to figure out what exactly we want, but it’s a little hard to say exactly.
For this particular issue I don’t think we really need the logs anymore. It seems to be something else, but it was a huge pain to track that down because from our side it seemed to be working as expected, we couldn’t say that for nats because we had no idea what it was doing. We just knew we weren’t getting any exceptions. And that the connection was still open. We thought it might be waiting for an ack, or some timeout, or maybe a blocking queue, etc, but we had no idea…
So we’re looking for something that would help with visibility in to that

Stephen Asbury  8 minutes ago
+1 i definitely see the need to open the box more, i am just pushing back against logging within the library - as opposed to say a listener all over the place - because it adds a dependency
:+1:
1


Stephen Asbury  8 minutes ago
i know that i have used the debug/verbose mode on the servers a lot, but in production that isn’t always an option and you need the client to spit out what you need
:+1:
1


Rick Hightower  7 minutes ago
Good points. Sorry I am late to the discussion. Nice to eMeet you Stephen. Nice to meet you Mike. I contributed to the Nats Java client recently and I understand @Mike DeMille point of view. It is good to hear @sasbury take on logging too. I wondered many of the same things that Mike has but have not met Stephen yet. I like the idea of a callback, and I think we can add some more logging and hints to what is happening to make debugging a bit easier.

Stephen Asbury  6 minutes ago
i am thinking that adding a “listener” that can be null or can be implemented however you want is safer for the library - we have some of that now, but perhaps not enough
:+1:
1


Mike DeMille  5 minutes ago
Yeah I get it, Stephen. I’m being vague because I don’t know what solution to suggest to you.
That’s probably where I’d land, too, is that maybe there’s just not enough. After talking with @Jon Pendlebury for a while, we’re still unsure what exactly we’d need, but it seems like the existing listeners aren’t enough

Stephen Asbury  4 minutes ago
Could you make a github issue along the lines of - make sure we have a listener that provides callbacks all the places https://github.com/nats-io/stan.java/pull/60 removed them - and include any concerns you have off the top of your head (edited) 

Stephen Asbury  3 minutes ago
that will get us kicked off i think

Mike DeMille  3 minutes ago
:thumbsup: can do