File logging #504
File logging #504
Conversation
omsupply-bot
bot
added
back-end
mark-prins
requested review from
roxy-dao,
clemens-msupply and
andreievg
and removed request for
roxy-dao
| @@ -39,6 +40,7 @@ export const Login = () => { | |||
|
|
|||
| useEffect(() => { | |||
| setPageTitle(`${t('app.login')} | ${t('app')} `); | |||
| LocalStorage.removeItem('/auth/error'); | |||
There was a problem hiding this comment.
offroad: I had the server error set, and couldn't shake it! With this - reset the auth error when you load the login page
server/server/src/main.rs
Outdated
|
|
||
| let (off_switch, off_switch_receiver) = oneshot::channel(); | ||
| let result = start_server(settings, off_switch_receiver).await; | ||
| // off_switch is not needed but we need to keep it alive to prevent it from firing | ||
| let _ = off_switch; | ||
| result | ||
| } | ||
|
|
||
| fn logging_init(settings: &LoggingSettings) { |
There was a problem hiding this comment.
should probably be initialise_logging but I liked thinking of this in a cockney accent.. "it's loggin, innit??"
Could also move this out to a separate file, but thought that it was ok to keep in main 🤷
Bundle size differenceComparing this PR to
|
There was a problem hiding this comment.
Looks good thanks.
Is fast_log popular enough? As an alternative, there is also file_rotate
https://docs.rs/file-rotate/latest/file_rotate/index.html
but we would need to implement our own Logger to use it.
The Android build is broken right now. There is another comment, I wait for you decision on it and can provide an Android fix.
server/configuration/example.yaml
Outdated
| @@ -26,3 +26,13 @@ | |||
| # username: "postgres" | |||
| # password: "password" | |||
| # database_name: "omsupply-database" | |||
| # logging: | |||
| ## one of: Console | File | |||
| # destination: Console | |||
There was a problem hiding this comment.
destination sounds like "file destination" maybe call it logging "mode"?
There was a problem hiding this comment.
thanks - will update
server/service/src/settings.rs
Outdated
| @@ -7,6 +8,7 @@ pub struct Settings { | |||
| pub server: ServerSettings, | |||
| pub database: DatabaseSettings, | |||
| pub sync: Option<SyncSettings>, | |||
| pub logging: LoggingSettings, | |||
There was a problem hiding this comment.
Can it be optional Option? i.e. if not specified use the defaults? (don't break existing setups)
There was a problem hiding this comment.
yeah, I wondered about that. can do - I was thinking that it was nice to have it specified in the config, so that the current logging options were plain to see.
Possibly not - it's not hugely used. there are a lot of versions though, which is another indicator.
That's where I started 😆 I like the file handling in it - I was trying to use the env_logger though, and the bug in that threw me. Like you say, we'd have to do more of the implementation ourselves and I thought that there should be a logging crate available already 🤷 |
|
thanks for the review @clemens-msupply! |
| #[derive(serde::Deserialize, Clone)] | ||
| pub struct LoggingSettings { | ||
| /// Console (default) | File | ||
| pub mode: LogMode, |
There was a problem hiding this comment.
Do we need a Console + File mode ?
There was a problem hiding this comment.
I did wonder about that! will have a quick try and see if I can get that working..
Closes #344
Logging configurable with the yaml config files - and logging to files with file rollover based on file size.
I've replace
env_loggerbecause there is an issue with thePipetarget - which fails silently ( this is last year, but i still have the problem in the latest release )I was trying to use the custom formatter in env_logger to write using
file-rotatewhich gives better rotation options.. but was worried about performance and the clunky way I had to implement.So have moved to the async
fast_loginstead. It doesn't rotate exactly as I'd like - but is good enough I think.Some notes are in the inline comments - but basically.. a temp file is logged to, and this is split when a size limit is reached, with a max number of files being specified in config. These temp files are created on start, so the history in these is lost when the server starts.
The main log will retain some history - as this is appended to when writing. This also has a max size set to prevent huge logfiles, and will keep a second file too when the size limit is hit. Couldn't see how to create multiple of these extra files though. Ideally I'd like the rotation to be limited to a specified number of files and these to be retained on start.. but what this PR does is good enough for now I think. ( actually, I've retested and there is a temp file retained now?? 🤷 they are temps, so I'd expect them to be removed ) The documentation isn't great for this crate!