Skip to content

Log DLI exceptions#101

Open
figuernd wants to merge 1 commit intomainfrom
nf/log-dli-exceptions
Open

Log DLI exceptions#101
figuernd wants to merge 1 commit intomainfrom
nf/log-dli-exceptions

Conversation

@figuernd
Copy link
Copy Markdown
Contributor

@figuernd figuernd commented Dec 4, 2025

DL node has been silently failing for me, need to explicitly log the traceback to see simple failures modes; e.g. bad IP

@figuernd figuernd requested review from johnwaalsh and rgov December 4, 2025 00:26
@rgov
Copy link
Copy Markdown
Member

rgov commented Dec 4, 2025

Doesn't it just die with the backtrace already? There's nothing else to catch the exception so by default it goes to stderr.

@figuernd
Copy link
Copy Markdown
Contributor Author

figuernd commented Dec 4, 2025

I thought it should too :( I removed the try/catch and was seeing the stack trace in the stderr if I manually launched the node but it was not making it into the logs at all until I added this deliberate logging. Only other thing I know that might cause that would be if there was a separate worker thread spawning that threw the exception? Didn't think that was the case in urllib but I don't know.

@rgov
Copy link
Copy Markdown
Member

rgov commented Dec 4, 2025

If the exception were thrown on another thread, you wouldn't have been able to catch it here.

Seems like this is a shortcoming of our logging setup we should try to solve at a different layer so we catch any such instance. I wonder if you could sic a coding agent on making a reduced test case, i.e., a container with a similar logging config as ours, that runs a bare minimum ROS node that just throws an exception? Or makes a url lib request to a non-existent page?

@johnwaalsh
Copy link
Copy Markdown
Member

This PR looks good to me. But yeah I agree that it'd be better if these errors were caught at a different level, so users don't have to write this boilerplate for every node. I wonder if any other existing nodes could silently fail in a similar manner.

Copy link
Copy Markdown
Member

@rgov rgov left a comment

Choose a reason for hiding this comment

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

I think we should close this in favor of a solution that addresses missing logs across all ROS nodes.

@figuernd
Copy link
Copy Markdown
Contributor Author

Instead of closing this, can we merge it and create an issue to look for a more robust solution? This will bite again.

@rgov
Copy link
Copy Markdown
Member

rgov commented Dec 30, 2025

I don't yet understand how it works. Is it possible the improvement seen in testing can be attributed to something else?

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