Skip to content

Introduce Logger and LogParams #7

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

Merged
merged 17 commits into from
Nov 10, 2024
Merged

Conversation

mxgrey
Copy link

@mxgrey mxgrey commented Nov 3, 2024

This PR introduces a Logger struct and makes some significant changes to the mechanics of the macros for reasons that I'll outline below.

The most obvious change is that LogConditions has been replaced with LogParams and the logging macros no longer have an overload that takes in a LogConditions argument. Instead the first argument of each macro is an expression that can be evaluated to something that implements the AsLogParams trait. All of the conditions that used to be expressed by LogConditions are now packed into LogParams.

As an example, in the previous code we had

log_info!(
    &node.name(), 
    LogConditions { 
        occurs: LoggingOccurrence::Always,
        publish_interval: Duration::from_millis(1000),
        log_if_true: count % 10 == 0,
    },
    "Manually constructed LogConditions",
);

but the new equivalent would be

log!(
    node
    .interval(Duration::from_millis(1000)
    .only_if(count % 10 == 0),
    "Manually constructed LogConditions",
);

More examples of the new syntax can be found in fn test_logging_macros inside logging.rs.

Another notable feature is the introduction of Logger which has two roles:

  • Pre-validate and pre-allocate logger names before a user tries to pass them into a logging macro
  • Allow the user to set the severity level of a logger (including the default logger)

Finally there was some significant restructuring of the log macro implementation, most of which was meant to avoid redundantly allocating CStrings on the heap every time the same log macro gets called. With the new implementation, each CString gets allocated exactly once, the first time it's needed. The only exception to this is the log message itself which may change each time the macro is called, so it's impractical to cache it.

One thing that could use improvement is we should add assertions to test_logging_macros to validate that the right output is being produced for every macro call. I'm deliberating whether to do this by capturing stdout/stderr or by subscribing to the rosout topics.

@geoff-imdex
Copy link
Owner

I couldn't resolve the conflicts in the files above without having it look like I did all the work! Not wanting to steal credit I've left this PR as-is. The conflict resolution was easy though - I created a local branch and want through the process.

Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Copy link
Author

mxgrey commented Nov 4, 2024

without having it look like I did all the work

I appreciate the consideration. I knew there would be merge conflicts because of the little bit of overlap between the PRs, and I planned on resolving that once the other PRs were merged. Thanks for doing the test run! I've resolve the merge conflicts now, and also fixed the formatting.

@mxgrey
Copy link
Author

mxgrey commented Nov 4, 2024

Sadly I just noticed that LazyLock, which I use extensively in this PR, was not stabilized until Rust 1.80, but upstream we'll need to target 1.75 at the latest in order to make it feasible for rclrs to be hosted on the build farm prior to Ubuntu 26.04 being released. I'll need to replace their use with OnceLock. I should have time to fix that tomorrow.

One other thing I'd like to do is use rcl_logging_configure_with_output_handler to redirect stdout and stderr in the logging tests to

  1. Suppress the misleading error messages that are being produced
  2. Add assertions to the tests that make sure the logging conditions are being handled correctly.

But I think we can leave this for a follow-up PR and move forward with merging this PR once the LazyLock issue is resolved.

@geoff-imdex
Copy link
Owner

Sadly I just noticed that LazyLock, which I use extensively in this PR, was not stabilized until Rust 1.80, but upstream we'll need to target 1.75 at the latest in order to make it feasible for rclrs to be hosted on the build farm prior to Ubuntu 26.04 being released. I'll need to replace their use with OnceLock. I should have time to fix that tomorrow.

I'd hit the same issue locally (we were using 1.79) - easy enough for us to upgrade our local rust, but good call on the upstream version (I'm afraid to say I'd not even looked into that).

But I think we can leave this for a follow-up PR and move forward with merging this PR once the LazyLock issue is resolved.

That's a nice idea and I agree that we should create a separate PR for that work - it would be nice to get the logging integrated if possible.

@mxgrey mxgrey mentioned this pull request Nov 5, 2024
@mxgrey
Copy link
Author

mxgrey commented Nov 5, 2024

I think this one is finally ready to merge with CI being green. After that #11 should be ready for review.

@geoff-imdex geoff-imdex merged commit 2bbf6b6 into geoff-imdex:main Nov 10, 2024
3 checks passed
@mxgrey mxgrey deleted the logging_params branch November 11, 2024 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants