Skip to content
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

decoder logging: add parameter to indicate defmt awareness #724

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

spookyvision
Copy link
Contributor

@spookyvision spookyvision commented Jan 17, 2023

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.

@Urhengulas Urhengulas added type: enhancement Enhancement or feature request breaking change fix / feature / improvement involves a breaking change and needs to wait until next minor version labels Jan 23, 2023
@Urhengulas
Copy link
Member

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
Copy link
Member

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.

@spookyvision
Copy link
Contributor Author

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

@Urhengulas
Copy link
Member

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
Copy link
Member

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

@spookyvision
Copy link
Contributor Author

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
Copy link
Contributor Author

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?

@Urhengulas
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

Urhengulas commented Jun 23, 2023

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(),
);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change fix / feature / improvement involves a breaking change and needs to wait until next minor version type: enhancement Enhancement or feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants