feat: file logging

[hnidoaht-101]

File logging

This PR is served for #39.

Implementation (Optional)

Following TODO placeholder left in the source code.

I have

• [x] run cargo fmt;
• [x] run cargo clippy;
• [x] run cargo testand all tests pass;
• [x] linked to the originating issue (if applicable).

[hnidoaht-101]

hi @MatteoNardi thank for your detailed reviews 💯 I'll update minor issues first. Based on your comment, I'd love to ask which possible path did you want to integrate with general pulsar logging? 😸 I should ask you guys before implementing this one 🥲

[hnidoaht-101]

Ah, nice catch!

Reusing pulsar.ini vs external log4rs config Reusing the pulsar.ini would give better consistency, pulsar CLI integration and eventual hot reloading. On the other hand, the external file gives greater flexibility and more features.

Actually I really wanted to reuse pulsar.ini for logging instead of external log4rs config. Err, you remind me about python logger ini file.... hmm, maybe we can use it with custom logger. Lemme think.

[MatteoNardi]

For now stick to to the minor issues and let's wait for Bandito's opinion before going forward.

I think we wanted to give tracing a try, the first solution which comes to mind would be replacing env_logger with tracing-subscriber and overriding it in the pulsar logger module by tracing-appender using with_default. But I don't want to waste your time on a solution we're not sure about, so let's wait some days.

[hnidoaht-101]

replacing env_logger with tracing-subscriber and overriding it in the pulsar logger module by tracing-appender using with_default.

@MatteoNardi tracing crate is very useful, i didn't recognize it has tracing-appender, I've read its code. Yah we may give it a try 😄 Lemme know once you guys made final decision for this feature, I'd love to rework it.

[banditopazzo]

Hi,

I think it's better to implement a simple handwritten file logger which doesn't use the log subsystem.

This module is called logger but in reality is the "threat event logger". it's goal is to log only threat events and not all the application logs.

Application and the event logging and separate things, we don't want to mix them.

We have planned to take a look at tracing for the application wide logging, if we decide to switch to tracing we will rethink a new solution.

[hnidoaht-101]

Hi,

I think it's better to implement a simple handwritten file logger which doesn't use the log subsystem.

This module is called logger but in reality is the "threat event logger". it's goal is to log only threat events and not all the application logs.

Application and the event logging and separate things, we don't want to mix them.

We have planned to take a look at tracing for the application wide logging, if we decide to switch to tracing we will rethink a new solution.

Got it. I will rework this PR & submit for review again.

[banditopazzo]

@hnidoaht-101 did you copy the code from https://github.com/polyverse/file-rotation/blob/main/src/asynchronous.rs ?

it's an unmaintained crate.

why did you change variables and function names?

I didn't have time to check in detail the code and in particular if the unsafe implementations of Send and Sync are legit, but I have doubts

at this state the code needs an accurate review, if you can't explain the code with better comments, a detailed review will be re scheduled for future

[thaodt]

@hnidoaht-101 did you copy the code from https://github.com/polyverse/file-rotation/blob/main/src/asynchronous.rs ?

it's an unmaintained crate.

why did you change variables and function names?

I didn't have time to check in detail the code and in particular if the unsafe implementations of Send and Sync are legit, but I have doubts

at this state the code needs an accurate review, if you can't explain the code with better comments, a detailed review will be re scheduled for future

Yeah sure, will add comment details for code. unsafe impl of Send and Sync are safe to use. Refs:

• https://doc.rust-lang.org/std/marker/trait.Send.html
• https://www.youtube.com/watch?v=yOezcP-XaIw

P/s: im in holidays, be back next week to update, thank for ur review.

[MatteoNardi]

unsafe impl of Send and Sync are safe to use.

[nitpiking mode on] If PendingRename or PendingCreate contained a !Send future, it would be a bug. Since you're provinding those callbacks and they're Send, it's not a problem (except that we're not letting the compiler make sure of that). A solution is to add the requirement for the dyn object to be Send:

pub enum RotationState {                                                      
    PendingRename(Pin<Box<dyn Future<Output = io::Result<()>> + Send>>),      
    PendingCreate(Pin<Box<dyn Future<Output = io::Result<fs::File>> + Send>>),
    PendingFlush,                                                             
    Done,                                                                     
}                                                                             

[thaodt]

unsafe impl of Send and Sync are safe to use.

[nitpiking mode on] If PendingRename or PendingCreate contained a !Send future, it would be a bug. Since you're provinding those callbacks and they're Send, it's not a problem (except that we're not letting the compiler make sure of that). A solution is to add the requirement for the dyn object to be Send:

pub enum RotationState {                                                      
    PendingRename(Pin<Box<dyn Future<Output = io::Result<()>> + Send>>),      
    PendingCreate(Pin<Box<dyn Future<Output = io::Result<fs::File>> + Send>>),
    PendingFlush,                                                             
    Done,                                                                     
}                                                                             

Neat! Thanks @MatteoNardi. 👍 Regarding to async file rotation, Im gonna rewrite a bit, its not copying and adjust it based on our needs. Currently its following rule rename file of logrotate in linux, for e.g. file.log, file.0.log, file.1.log, etc .... If we want to rotate by date time and rename by datetime, the code need to be handled more for this case

[hnidoaht-101]

As discussed with @banditopazzo, I'm going to close this PR first to rethink a simpler solution instead of using unmaintained version with my own update.