Notes on `slog` usage.

[dpc]

Hi. I'm the author of slog. I sometimes check rev-dependencies on slog and I've noticed that reql is using it.

First of all, thanks for using slog - I've put quite a bit of effort into making it, and I'm happy to see it being used. I hope it will work well for you. Second - I know reql is 0.0.5 and some stuff I might point out you might find obvious, and just made it as shortcuts in reql. Even so - maybe what I wrote will be useful for other people when eg. google will point them here. Third - feel free to ignore anything that you disagree with, I'm just sharing my opinion on how I envisioned things to work, and doing it your own way is fine with me.

Logger is Sync so it does not require wrapping in RwLock. The idea is that Loggers should be worry-free and when creating them you should only be concerned about the "context of logging", and not about your program threading-structure and so on.

Generally libraries should almost never create their own Drains. It's app to application using libraries to setup logging. Only the application can know what are the logging requirements: where, how and what to log. Usually libraries should accept Logger from whatever is using them, potentially falling back to something. Here's the reference example: https://github.com/slog-rs/slog/blob/master/crates/example-lib/lib.rs . In case of reql, I guess corresponding struct to MyLib would be Client. Note that this requires storing Logger in Client and potentially other structs. This is beneficial, as such Loggers can carry the additional data. In reql case, I guess the Client's logger should carry the user, host, db records with respective values. So eg. when someone is using multiple connections, it's clear which message comes from which Client.

It's not slog related but while looking at reql the:

pub const r: command::Client = command::Client;

thing is very non-idiomatic. reql is it's own crate, with it's own namespace. Having a const object, introduces a singleton, which forces locking and other problems. If anything there should be either global static function returning a connection builder, or Client should have a static method, returning a builder, and the user of reql would be responsible for locking etc.

extern create reql;

fn main() {
  let logger = /* ... */
  let reql_db = reql::Client::new(logger).set_db("x").build().expect("couldn't connect to db");
}

I'm not sure if logging errors in error! is a good idea. I mean - it's the data that is being returned to the user of the library and most of the time user is going to log an error themselves. It only makes sense for the library to log things that the user of it is not able to access directly, to allow transparency into what is happening internally.

[rushmorem]

Thank you for taking some time off your busy schedule to review my code. I'm sorry for taking so long to get back to you. I thought I would have replied by now but I have been awfully busy. Since I don't want to rush my response to this and I would like to take this opportunity to seek further guidance from you, I will give you a shout once I'm ready. I really appreciate your feedback. I have already addressed some of your points. Thank you, once again.

[dpc]

No worries. We all do our projects when we have free time. I understand, and thanks for the head ups.

[rushmorem]

I think I have now addressed all your points so I am closing this now. However, I would really appreciate it if you can review the code again. I know you are very busy so don't feel bad at all if you can't. I will totally understand that. You have helped me a lot as it is already. I have responded to your original comments below but just to explain why I did it the way I did it at the time. The new code is almost a full re-write.

I'm the author of slog.

I already knew who you were by that time so you didn't really need to introduce yourself :wink: I'm a huge fan of your work.

I hope it (slog) will work well for you.

I think it's working very well for me so far :smile: I love it! Right now I'm working towards releasing v0.0.6 within the next 24 hours. I would have loved to use slog v2 but unfortunately I couldn't get slog-term (v2 alpha) to work with it. I need slog-term for the examples.

Logger is Sync so it does not require wrapping in RwLock.

Yes I noticed that. The reason I put it in a RwLock at that time was so I could mutate it at runtime. The idea was to listen for certain signals on a running server (in production) and change the configuration of the logger accordingly.

Generally libraries should almost never create their own Drains.

For v0.0.5 and below logging was just for me to help with debugging as I developed the library (instead of using print statements). I don't think I even exposed it publicly at that time.

... thing is very non-idiomatic. reql is it's own crate, with it's own namespace.

This wasn't about namespacing. I know the docs said "The top-level ReQL namespace" but that was just because I was trying to be consistent with the documentation of the official drivers. The client object for ReQL is called r by convention and for convenience it comes defined by all the official drivers including the Java driver which, like Rust, already has namespaces baked in. I was trying to do the same thing for convenience and consistency with the official drivers.

I'm not sure if logging errors in error! is a good idea. I mean - it's the data that is being returned to the user of the library and most of the time user is going to log an error themselves. It only makes sense for the library to log things that the user of it is not able to access directly, to allow transparency into what is happening internally.

I'm glad you actually noticed this because I may have valuable feedback for you here. It would be awesome if you could incorporate this feedback into slog somehow, if at all possible. The thing is, as an amateur Sysadmin I'm often overwhelmed by too much logging information. I find it counter-productive. Instead of helping me debug the problem at hand I find all the other logs distracting. The idea with that error macro was so that I could log only when an error occurred and also print only information that's helpful in debugging that particular error.

I hope you will find my feedback somehow useful. Once again, thank you very much for taking your time to review my work. I really appreciate that. Thank you for the helpful pointers and thank you for slog, it's amazing :+1:

[dpc]

(v2 alpha) is not the right version. It was yanked. I think the latest one is 2.0.0-4.0 . Sorry for the confusion.

[dpc]

Oh wow, a lot have changed. There has been a lot of effort put into this crate. This was one of the first crates using slog so it was important for me to be able to show it as an example, which I'm going to do eagerly now. :) Great job.

[rushmorem]

Thank you. I'm glad you like the new changes.