*: Use slog #3010
*: Use slog #3010
Conversation
|
Cool. What is the benchmark? |
src/config.rs
Outdated
| @@ -696,15 +696,15 @@ impl Default for MetricConfig { | |||
| } | |||
|
|
|||
| #[derive(Serialize, Deserialize)] | |||
| #[serde(remote = "LogLevelFilter")] | |||
| #[serde(remote = "Level")] | |||
| #[serde(rename_all = "kebab-case")] | |||
| pub enum LogLevel { | |||
There was a problem hiding this comment.
This is a breaking change.
slog does not have the concept of an Off setting, and changes the name of Warn, while introducing a Critical level.
There was a problem hiding this comment.
Off is barely used, I think it's OK to just removing it. But Warn is commonly used, better make it compatible.
There was a problem hiding this comment.
can we do a conversion internally? some users now use warn in production.
|
Using |
Hoverbear
force-pushed
the
slog
branch
4 times, most recently
from
b863213 to
8648e36
Compare
|
Sample of file based logging: I'm not a huge fan of the empty brackets (which is where the currently unused structured data goes) so I'm currently investigating the best way to work around this. I think we might be able to use a decorator like we do in the terminal. |
|
I was able to use the |
|
PTAL @BusyJay @overvenus |
src/bin/setup.rs
Outdated
| @@ -11,6 +11,9 @@ | |||
| // See the License for the specific language governing permissions and | |||
| // limitations under the License. | |||
|
|
|||
| use slog::{self, Drain}; | |||
| use slog_async; | |||
| use slog_term; | |||
src/config.rs
Outdated
| @@ -695,16 +697,41 @@ impl Default for MetricConfig { | |||
| } | |||
| } | |||
|
|
|||
| // This type exists purely for interacting with Serde, and using `slog::Level` should | |||
| // be preferred. | |||
| #[derive(Serialize, Deserialize)] | |||
There was a problem hiding this comment.
Looks like not necessary for Deserialize anymore?
There was a problem hiding this comment.
I tried to remove it and got an error:
Compiling tikv v2.1.0-alpha.1 (file:///home/hoverbear/git/tikv)
error[E0308]: match arms have incompatible types
--> src/config.rs:762:28
|
762 | #[derive(Clone, Serialize, Deserialize, PartialEq, Debug)]
| ^^^^^^^^^^^
| |
| expected enum `slog::Level`, found enum `config::Level`
| match arm with an incompatible type
|
= note: expected type `slog::Level`
found type `config::Level`
= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
error[E0308]: match arms have incompatible types
--> src/config.rs:762:28
|
762 | #[derive(Clone, Serialize, Deserialize, PartialEq, Debug)]
| ^^^^^^^^^^^
| |
| expected enum `slog::Level`, found enum `config::Level`
| match arm with an incompatible type
|
= note: expected type `slog::Level`
found type `config::Level`
= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
error: aborting due to 2 previous errors
For more information about this error, try `rustc --explain E0308`.
error: Could not compile `tikv`.
To learn more, run the command again with --verbose.
There was a problem hiding this comment.
Have you tried to set config.log-level to warn and get it parsed successfully? I'm afraid something has gone wrong.
Also seems the enum can be removed completely now that you custom the deserialization. You can also check out util::config::compression_type_level_serde to see how does it work.
src/config.rs
Outdated
| #[serde(rename_all = "kebab-case")] | ||
| pub enum LogLevel { | ||
| pub enum Level { |
There was a problem hiding this comment.
Why not use FilterLevel? What's the difference?
There was a problem hiding this comment.
Are you referring to slog::LevelFilter (which is a Drain wrapper) or are you suggesting to rename this to LevelFilter?
There was a problem hiding this comment.
What I mean is slog::FilterLevel.
There was a problem hiding this comment.
It's a Drain which is a target for logging, it is produced when you call Drain::filter_level.
I'm not sure I understand what you are suggesting I do. 😦
There was a problem hiding this comment.
Drain::filter_level accepts a Level which we call here.
I'm not entirely clear what the difference is, but here is their definition and it looks like Level is preferred.
There was a problem hiding this comment.
seem that we need to define a new Drain or pass a filter like https://docs.rs/slog/2.2.3/slog/trait.Drain.html#method.filter if we want to use FilterLevel directly.
Btw, the FilterLevel also uses warning, we still need to do the warn -> warning conversation for the compatibility. If there is no extra benefit and we don't care off, I prefer Level here.
Hoverbear
force-pushed
the
slog
branch
2 times, most recently
from
563475d to
6c281d2
Compare
| "debug" => Some(Level::Debug), | ||
| "trace" => Some(Level::Trace), | ||
| "info" => Some(Level::Info), | ||
| _ => None, |
There was a problem hiding this comment.
em, I prefer using info instead of None here. Log Level is not a critical configuration.
There was a problem hiding this comment.
Returning a None here allows the caller to determine if an invalid value was provided, and decide how to act. If we replace this with Level::Info we are forcing a default. Level::Info is should still be the default after this PR.
|
Rest LGTM PTAL @BusyJay |
|
While working on #3035 I may have found a bug with log rotation, please DNM while I investigate. |
|
Disregard, this is related to #3035 . |
* Removes `off` as a log level option on the command line. * Adds `critical` and `warning` log levels. * Lowercases log level serialization. * Flush before rollover.
|
so @Hoverbear can we merge this now? |
|
@siddontang Yes, no bugs which I know of currently in this PR. |
|
LGTM |
|
/run-all-tests |
After speaking with @siddontang we think this is a better option than #2997.
Motivation
We noted with some users that logging was taking a considerable amount of time during system operation. This is due to the fact that the
logcrate operates syncronously. We noted when exploring #2997 that moving to asyncronous logging was measurably faster.During our discussion it was decided that
slogwas a good alternative that offered built in asyncronous logging capabilities. It offers other benefits as well, such as structured logging and fancy terminal output.Implementation
This PR moves us to using
slogvia theslog_stdlogcrate which essentially adapts thelogbased macros to useslog. In the future we can place work into migrating to theslogbased logging macros which offer the option for structure data.With this PR it was possible to completely remove the
StdErrLoggerand use the providedslog_termterminal logger. TheRotatingFileLoggerwas altered so it simply implementsWriteand can be called from theslogdecorators. Additionally the distinction between the Logger and the LoggerCore was removed, as well as the mutex which was involved.A future PR will implement file rotation by size.
Benchmarks
On master:
With this PR:
Samples
Sample of terminal output:
Sample of a log file: