defmt icon indicating copy to clipboard operation
defmt copied to clipboard

decoder logging: add parameter to indicate defmt awareness

Open spookyvision opened this issue 3 years ago • 10 comments

sometimes it's preferable to use an existing logger instead of a bespoke defmt-aware one. This PR makes it possible to preserve log levels in this case. It breaks a public API so requires a version bump.

spookyvision avatar Jan 17 '23 02:01 spookyvision

Thank you for the PR. It does look good to me, but it is unclear when we want the next breaking change of defmt-decoder.

Urhengulas avatar Jan 23 '23 15:01 Urhengulas

I am just wondering if it wouldn't be easier to just always set the log-level if it is Some? defmt doesn't need that but as far as I am aware it doesn't hurt either and then we wouldn't need to handle the two cases differently.

Urhengulas avatar Mar 29 '23 19:03 Urhengulas

that could shave off a few lines - in that case I'd probably remove the level from the target payload.

spookyvision avatar Mar 30 '23 13:03 spookyvision

in that case I'd probably remove the level from the target payload.

I am unsure if this is possible, since we use Option<Level> in the payload and None means println. But maybe it is, didn't try it out.

Urhengulas avatar Jun 13 '23 10:06 Urhengulas

If we don't change the public api, then we could also merge this sooner, since it wouldn't be a breaking change anymore.

Urhengulas avatar Jun 13 '23 10:06 Urhengulas

I am unsure if this is possible, since we use Option<Level> in the payload and None means println. But maybe it is, didn't try it out.

ok, probably not a good idea then. I didn't try it out either though 🙃

If we don't change the public api, then we could also merge this sooner, since it wouldn't be a breaking change anymore.

hm. any idea how to achieve that?

spookyvision avatar Jun 19 '23 13:06 spookyvision

I guess I could introduce a new function for defmt-aware logging instead of adding a parameter to the existing one. New functions aren't actually breaking, even if they change an API surface, no?

spookyvision avatar Jun 19 '23 13:06 spookyvision

I guess I could introduce a new function for defmt-aware logging instead of adding a parameter to the existing one. New functions aren't actually breaking, even if they change an API surface, no?

I am unsure. If someone did use defmt::* they would get an error if they happen to use the same function name.

But if we just always pass the log level I think we don't need to make any change to the API

Urhengulas avatar Jun 19 '23 13:06 Urhengulas

@spookyvision There is another breaking release for defmt-decoder coming up (#762). I think we can just release your PR and that one together as soon as they are both done.

Urhengulas avatar Jun 23 '23 08:06 Urhengulas

I think following code would allow to use both a defmt-aware logger and a normal one, without the need to add a new argument.

let mut builder = Record::builder();

// set Level, useful for non-defmt loggers
// defmt-aware loggers should use the level from the Payload
if let Some(level) = level {
    builder.level(level);
}

let target = format!(
    "{}{}",
    DEFMT_TARGET_MARKER,
    serde_json::to_value(Payload { timestamp, level }).unwrap()
);

log::logger().log(
    &builder
        .args(format_args!("{}", frame.display_message()))
        .target(&target)
        .module_path(module_path)
        .file(file)
        .line(line)
        .build(),
);

Urhengulas avatar Jun 23 '23 08:06 Urhengulas