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

RSDK-5986 - module logging propagation and resource-level loggers #312

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

stuqdog
Copy link
Member

@stuqdog stuqdog commented Oct 28, 2024

Before and after (server side):
Screenshot 2024-10-28 at 13 47 28
Screenshot 2024-10-28 at 13 47 54

Before and after (client side):
Screenshot 2024-10-28 at 13 48 16
Screenshot 2024-10-28 at 13 48 27

@stuqdog stuqdog requested a review from acmorrow November 1, 2024 14:40
@stuqdog
Copy link
Member Author

stuqdog commented Nov 1, 2024

@acmorrow fyi, this is still marked as draft because the abort trap issue isn't fully resolved, but I do think it's ready for you to look at whenever you're able. I've managed to significantly reduce (though not eliminate) the occurrence of abort traps, and isolate the failure points when we do get an abort trap. These places are highlighted in comments in the code. Let me know if you want to discuss or have any questions or thoughts!

Comment on lines +38 to +39
// CR erodkin: every now and again this call causes a failure. From adding log statements
// in rust-utils, I have confirmed that the failure happens within the rust code.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flagging this, for some reason free_string very occasionally fails in the rust call, specifically here. Because the rust code is so minimal I haven't tried much on that side beyond explicitly dropping the CString, which didn't seem to make a difference.

BOOST_LOG_TRIVIAL(error) << exc.what();
}
}
// CR erodkin: three things of note here:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flagging this as well, the majority of abort trap errors happen at the end of the ModuleService destructor, after all owned memory has seemingly been destroyed. It's entirely unclear to me what's going on here.

/// the third argument. Sets severity to info otherwise. Useful for module
/// implementations. See LogLevel documentation in the RDK for more information on
/// how to start modules with a "log-level" commandline argument.
void set_logger_severity_from_args(int argc, char** argv);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer necessary because we support per-logger severity levels. Additionally, it was causing us to fail to send debug logs to RDK.

@stuqdog stuqdog requested a review from lia-viam November 5, 2024 13:59
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.

1 participant