add `TimePadding` with `Right` and `AddZeros` #134

cscherrNT · 2023-08-02T08:24:06Z

This is a proposed fix for #133.

I added a TimePadding Enum. in write_time, the formatted time stamp is generated with the time module like before, then I do some formatting.

TimePadding::Right just applys an offset to the timestamp, like other paddings.
TimePadding::AddZeros adds zeros to the Timestamp, accodring to it's length. (only with rfc3339 formatting)

Usage

Build a Config like this:

                let conf = conf_builder
                    .set_location_level(elem)
                    .set_target_level(elem)
                    .set_max_level(elem)
                    .set_time_level(elem)
                    .set_time_format_rfc3339()
                    .set_time_padding(config::TimePadding::AddZeros)
                    .build();

Example output:

2023-08-02T08:08:45.217511363Z [INFO] simplelog::tests: [src/lib.rs:238] Test Information
2023-08-02T08:08:45.217519856Z [INFO] simplelog::tests: [src/lib.rs:238] Test Information
2023-08-02T08:08:45.217590429Z [INFO] simplelog::tests: [src/lib.rs:238] Test Information
2023-08-02T08:08:45.217599035Z [INFO] simplelog::tests: [src/lib.rs:238] Test Information
2023-08-02T08:08:45.217624283Z [INFO] simplelog::tests: [src/lib.rs:238] Test Information
2023-08-02T08:08:45.217628916Z [INFO] simplelog::tests: [src/lib.rs:238] Test Information
2023-08-02T08:08:45.217652289Z [INFO] simplelog::tests: [src/lib.rs:238] Test Information
2023-08-02T08:08:45.217656130Z [INFO] simplelog::tests: [src/lib.rs:238] Test Information
2023-08-02T08:08:45.217680170Z [INFO] simplelog::tests: [src/lib.rs:238] Test Information
2023-08-02T08:08:45.217684238Z [INFO] simplelog::tests: [src/lib.rs:238] Test Information
2023-08-02T08:08:45.217714112Z [INFO] simplelog::tests: [src/lib.rs:238] Test Information
2023-08-02T08:08:45.217717736Z [INFO] simplelog::tests: [src/lib.rs:238] Test Information
2023-08-02T08:08:45.217741200Z [INFO] simplelog::tests: [src/lib.rs:238] Test Information
2023-08-02T08:08:45.217782376Z [INFO] simplelog::tests: [src/lib.rs:238] Test Information
2023-08-02T08:08:45.217815202Z [INFO] simplelog::tests: [src/lib.rs:238] Test Information
2023-08-02T08:08:45.217819815Z [INFO] simplelog::tests: [src/lib.rs:238] Test Information
2023-08-02T08:08:45.217886072Z [INFO] simplelog::tests: [src/lib.rs:238] Test Information
2023-08-02T08:08:45.217891502Z [INFO] simplelog::tests: [src/lib.rs:238] Test Information

Note: This is my first Pull Request, I hope I didn't miss anything important.

only do timepadding for rfc3339 fix a doc comment

cscherrNT · 2023-08-02T08:25:29Z

src/loggers/logging.rs

+                //
+                // the rfc3339 timestamp is 30 characters long, if it would not end with a 0 in the
+                // subseconds part. To fix this inconsistency, we add zeros before the Z.
+                while formatted.len() < 30 {


Maybe make the 30 into a constant?

Drakulix

This looks like a really solid start.

A couple of smaller remarks and also please add your changes in the Unreleased section of the CHANGELOG.md. Otherwise very good job!

Drakulix · 2023-08-06T18:54:46Z

src/config.rs

@@ -345,6 +363,7 @@ impl Default for Config {
            module: LevelFilter::Off,
            time_format: TimeFormat::Custom(format_description!("[hour]:[minute]:[second]")),
            time_offset: UtcOffset::UTC,
+            time_padding: TimePadding::AddZeros,


I think this modifies the current default (of no padding), which I would want to release as a minor version. Can we maybe consider this to be Off by default?

Leaving the padding as Off is certainly possible, but I would argue that setting a padding by default would improve usability a little.

I agree, but then we will need a new major version.

Drakulix · 2023-08-06T18:57:27Z

src/loggers/logging.rs

-        Err(err) => panic!("Invalid time format: {}", err),
-        _ => {}
+    let mut formatted: String =
+        formatted.unwrap_or_else(|err| panic!("Invalid time format: {}", err));


I think this drops the special casing for Format::StdIo, you should be able to use assign using the match here instead.

Suggested change

formatted.unwrap_or_else(|err| panic!("Invalid time format: {}", err));

match formatted {

Ok(res) => res,

Err(Format::StdIo(err)) => return Err(err),

Err(err) => panic!("Invalid time format: {}", err),

};

Drakulix · 2023-08-06T18:59:40Z

src/loggers/logging.rs

+                // subseconds part. To fix this inconsistency, we add zeros before the Z.
+                while formatted.len() < 30 {
+                    formatted.insert(formatted.len() - 1, '0')
+                }


I feel like this is not very performant and there should be a way to not add the zeros one by one.

Drakulix · 2023-08-06T18:59:45Z

src/loggers/logging.rs

+                //
+                // the rfc3339 timestamp is 30 characters long, if it would not end with a 0 in the
+                // subseconds part. To fix this inconsistency, we add zeros before the Z.
+                while formatted.len() < 30 {


add TimePadding right and AddZeros

3bcf427

only do timepadding for rfc3339 fix a doc comment

cscherrNT mentioned this pull request Aug 2, 2023

logs with rfc3339 time have no consistent length #133

Open

cscherrNT commented Aug 2, 2023

View reviewed changes

Drakulix reviewed Aug 6, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `TimePadding` with `Right` and `AddZeros` #134

add `TimePadding` with `Right` and `AddZeros` #134

cscherrNT commented Aug 2, 2023

cscherrNT Aug 2, 2023

Drakulix Aug 6, 2023

Drakulix left a comment

Drakulix Aug 6, 2023

PlexSheep Aug 6, 2023

Drakulix Aug 6, 2023

Drakulix Aug 6, 2023

Drakulix Aug 6, 2023

Drakulix Aug 6, 2023

-        formatted.unwrap_or_else(|err| panic!("Invalid time format: {}", err));
+        match formatted {
+            Ok(res) => res,
+            Err(Format::StdIo(err)) => return Err(err),
+            Err(err) => panic!("Invalid time format: {}", err),
+        };

add TimePadding with Right and AddZeros #134

Are you sure you want to change the base?

add TimePadding with Right and AddZeros #134

Conversation

cscherrNT commented Aug 2, 2023

Usage

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Drakulix 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

add `TimePadding` with `Right` and `AddZeros` #134

add `TimePadding` with `Right` and `AddZeros` #134