Added logging submodule containing a `SentryLogger` struct #18

davechallis · 2018-03-16T15:58:53Z

** Summary **

Adds a SentryLogger struct that implements the log::Log trait.

This allows for logging to Sentry using the macros provided by the log
crate, e.g. debug!("foo"), error!("Thing failed: {}", thing) etc.

I'm working separately on a simple logger which combines multiple loggers, and am currently using it to enable logging to both Sentry (using this module) and to stdout/stderr (using env_logger) using the same log macros.

Test Plan

I've added a file to examples/log-macro-demo.rs which shows usage, and can be used for testing.

…ements the `log::Log` trait. This allows for logging to Sentry using the macros provided by the `log` crate, e.g. `debug!("foo")`, `error!("Thing failed: {}", thing)` etc.

Mythra

I very much like the idea of this, but some very small comments.

Thanks for the PR!

Mythra · 2018-03-16T16:04:10Z

examples/log-macro-demo.rs

+use std::env;
+
+fn main() {
+  let dsn = "https://XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX:[email protected]/XX";


Can we change this to getting the DSN from the environment similar to other examples? This allows some testing I do easier (as I can validate directly in sentry).

Sounds good, I must have missed that example, will take a look shortly.

Mythra · 2018-03-16T16:05:33Z

src/logging.rs

+
+use log::{self, Log, Record, Level, Metadata, SetLoggerError};
+
+use super::Sentry;


I'm not as big of a fan of super for something that's in the same directory. I know this is a nitpick but would you mind changing this to: use Sentry;?

No problem, will update this.

Mythra · 2018-03-16T16:09:28Z

src/logging.rs

+  /// * `sentry` - Sentry client used to deliver log messages.
+  /// * `logger_name` - String used as logger name in messages.
+  /// * `level` - Minimum level to log messages to Sentry at.
+  pub fn new(sentry: Sentry, logger_name: &str, level: Level) -> Self {


I know this adds more work for the example, but I'm more of the idea we should take logger_name as a String here (instead of as a str).

The reason for this is because we're hiding a clone behind them, and it always clones with this implementation. Even if I already have a String. Say for example I have:

let a_str: String = get_some_cool_string_from_somewhere!(); // I now have to "clone" a_str in order to create a logger even though I already have an instance of "String". SentryLogger::new(sentry, &a_str, level);

Thus taking a String here (and not using to_owned()) allows us to not "hide" anything away. It's very explicit we need ownership, and we won't clone or anything under the hood.

Good point, will also fix that here.

Mythra · 2018-03-16T16:10:35Z

src/logging.rs

+        Level::Warn => self.sentry.warning(&self.logger_name, &format!("{}", record.args()), None, None),
+        Level::Info => self.sentry.info(&self.logger_name, &format!("{}", record.args()), None, None),
+        Level::Debug => self.sentry.debug(&self.logger_name, &format!("{}", record.args()), None, None),
+        _ => (), // client doesn't support logging at Trace level


Should we maybe send this as a debug to sentry, and just document that if you send us a "TRACE" message we'll "downgrade" it to debug?

Yup, that's probably more useful than silently discard.

davechallis · 2018-03-16T16:22:05Z

One other thing I haven't figured out yet is multithreaded usage - it would be nice for users to be able to e.g. initialise the logger once, then just have debug! etc. work across threads (as they do for some of the basic loggers, e.g. env_logger), but not sure this is possible yet...

Mythra · 2018-03-16T16:24:56Z

The underlying Sentry object is thread safe. I see no reason that this shouldn't be.

Maybe the answer is to add:

unsafe impl Sync for SentryLogger {}
unsafe impl Send for SentryLogger {}

And see what complains?

davechallis · 2018-03-16T16:38:59Z

Will experiment with doing that for the SentryLogger and see what works. Will take a look at some of the other log implementations and see how they handle things too.

Details at: Mythra#18

davechallis · 2018-03-16T17:22:18Z

Did some more experiments around multithreaded used of the new logger, seemed to work without any problems on the client side, which is good.

I did notice that Sentry combined them as a single event on their side of things, though I'm not sure how that works on the server-side.

E.g. I had some code which looked like:

let t1 = thread::spawn(|| { warn!("a"); });
let t2 = thread::spawn(|| { warn!("b"); });
let t3 = thread::spawn(|| { warn!("c"); });
let t4 = thread::spawn(|| { warn!("d"); });
let _ = t1.join();
let _ = t2.join();
let _ = t3.join();
let _ = t4.join();

this then showed up in Sentry as:

So looks hopeful so far :)

Mythra · 2018-03-16T17:26:50Z

If you're using the same sentry instance that doesn't particularly surprise me. Perhaps there's some way to tag thread ids in sentry logging? I'll be honest I'm not too sure how sentry logging works (we only use error reporting for it at work).

Mythra · 2018-03-16T19:25:39Z

Looks like the circleci error is caused by us using an older rust image that is no longer maintained:

jimmycuadra/rust:latest

Specifically.

If you change the line in circle.yml to match:

- image: rust:latest

The build should pass! I'd also like to see your example added to the circle.yml + appveyor.yml so we can test the example running.

davechallis · 2018-03-19T09:30:03Z

Excellent idea, have done that now, will see how it goes.

Added logging submodule containing a SentryLogger struct which impl…

01a218b

…ements the `log::Log` trait. This allows for logging to Sentry using the macros provided by the `log` crate, e.g. `debug!("foo")`, `error!("Thing failed: {}", thing)` etc.

Mythra reviewed Mar 16, 2018

View reviewed changes

Fixes/improvements to previous pull request.

8eefef8

Details at: Mythra#18

Updated Rust image and added new example to appveyor/circle YAML.

035a76c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added logging submodule containing a `SentryLogger` struct #18

Added logging submodule containing a `SentryLogger` struct #18

davechallis commented Mar 16, 2018

Mythra left a comment

Mythra Mar 16, 2018

davechallis Mar 16, 2018

Mythra Mar 16, 2018

davechallis Mar 16, 2018

Mythra Mar 16, 2018

davechallis Mar 16, 2018

Mythra Mar 16, 2018

davechallis Mar 16, 2018

davechallis commented Mar 16, 2018

Mythra commented Mar 16, 2018

davechallis commented Mar 16, 2018

davechallis commented Mar 16, 2018

Mythra commented Mar 16, 2018

Mythra commented Mar 16, 2018 •

edited

Loading

davechallis commented Mar 19, 2018


		use log::{self, Log, Record, Level, Metadata, SetLoggerError};

		use super::Sentry;

Added logging submodule containing a SentryLogger struct #18

Are you sure you want to change the base?

Added logging submodule containing a SentryLogger struct #18

Conversation

davechallis commented Mar 16, 2018

Mythra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davechallis commented Mar 16, 2018

Mythra commented Mar 16, 2018

davechallis commented Mar 16, 2018

davechallis commented Mar 16, 2018

Mythra commented Mar 16, 2018

Mythra commented Mar 16, 2018 • edited Loading

davechallis commented Mar 19, 2018

Added logging submodule containing a `SentryLogger` struct #18

Added logging submodule containing a `SentryLogger` struct #18

Mythra commented Mar 16, 2018 •

edited

Loading