-
Notifications
You must be signed in to change notification settings - Fork 14
refactor: refactor to spdlog-rs
instead of tracing
#538
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
base: master
Are you sure you want to change the base?
Conversation
Initially it looks good, but what I'm wondering is whether we should just replace the |
Yeah we should replace tracing with spdlog, current codes is just convenient for debugging. I'll delete the tracing codes after spdlog parts are good to go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a brief look and most of it seems to be fine. I'll probably merge SpriteOvO/spdlog-rs#93 in the next couple days. In the meantime, you can try removing the use of tracing from oma entirely.
src/main.rs
Outdated
let rotating_sink = if let Ok(log_file) = &log_file { | ||
Some( | ||
AsyncPoolSink::builder() | ||
.sink(Arc::new( | ||
RotatingFileSink::builder() | ||
.base_path(&log_file) | ||
.formatter(formatter.clone()) | ||
.rotation_policy(RotationPolicy::Hourly) | ||
.build() | ||
.unwrap(), | ||
)) | ||
.overflow_policy(spdlog::sink::OverflowPolicy::DropIncoming) | ||
.build() | ||
.unwrap(), | ||
) | ||
} else { | ||
None | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RotatingFileSink
should not rotate hourly as oma did not do so. Could try RotationPolicy::FileSize(u64::MAX)
and set max_files(config.save_log_count())
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, file size is set to 10MB.
src/main.rs
Outdated
.join("oma") | ||
}; | ||
|
||
let log_file = create_log_file(&log_dir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating variants for filenames has been done internally by spdlog-rs, so oma no longer needs to create names manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
FYI, SpriteOvO/spdlog-rs#93 is merged. |
I'll work on this PR tomorrow. |
|
3ef6995
to
95052c9
Compare
Refactor
tracing
tospdlog-rs
for logging, which is behind thespdlog-rs
feature flag, andtracing
codes should be removed before this PR gets merged.The original
tracing
logger is behind thetracing
feature flag now.Haven't written comments yet...
And blocked by SpriteOvO/spdlog-rs#93 .
This closes #486 .
cc @SpriteOvO