Skip to content

Include Rust's native stack trace in CometNativeException #104

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

Closed
advancedxy opened this issue Feb 24, 2024 · 7 comments · Fixed by #170
Closed

Include Rust's native stack trace in CometNativeException #104

advancedxy opened this issue Feb 24, 2024 · 7 comments · Fixed by #170
Labels
enhancement New feature or request

Comments

@advancedxy
Copy link
Contributor

advancedxy commented Feb 24, 2024

What is the problem the feature request solves?

Currently, the backtrace is only added when rust panics.

However, when the exception is thrown back to JVM side. The error msg sometimes might be not that helpful. It would be ideal to get the backtrace for the rust error as well.

Describe the potential solution

No response

Additional context

No response

@advancedxy advancedxy added the enhancement New feature or request label Feb 24, 2024
@viirya
Copy link
Member

viirya commented Feb 24, 2024

I remember we intentionally trim stack trace to reduce binary size.

@advancedxy
Copy link
Contributor Author

Seems reasonable to trim stack trace in production.

Maybe we can enable stack trace in the debug build?

@viirya
Copy link
Member

viirya commented Feb 25, 2024

If possible, then yes, I think it is good idea to add stack trace for debugging.

I remember we have discussed before and tried it but seems not working as the config cannot be changed between debug and release build.

Maybe I misremembered it. 🤔

@sunchao
Copy link
Member

sunchao commented Feb 25, 2024

I think we looked into this before. The problem is a large part of the errors are originated from Arrow or DF. @comphead has worked on this in DF, see apache/datafusion#7360. We could probably apply the same changes to Comet.

I remember we intentionally trim stack trace to reduce binary size.

Yes we did that for release profile, see https://github.com/apache/arrow-datafusion-comet/blob/main/core/Cargo.toml#L96. For debug profile, the backtrace should still contain full information including the file names and line numbers.

@advancedxy
Copy link
Contributor Author

@comphead has worked on this in DF, see apache/datafusion#7360. We could probably apply the same changes to Comet.

Sounds great.

@comphead
Copy link
Contributor

I'll check how it works with the Comet but for DF the quick doc to enable the backtrace is https://arrow.apache.org/datafusion/user-guide/example-usage.html#enable-backtraces

Basically if you set the backtrace it should show the backtrace for most cases, the coverage on DF still getting improved.
The backtrace will show rootcause within DF, however if the error happens on arrow-rs side, this is not covered on their side.

In this case we can only go to arrow-rs entrypoint and then debug manually

@sunchao sunchao changed the title Include rust's native stack trace in CometNativeException Include Rust's native stack trace in CometNativeException Feb 29, 2024
@snmvaughan
Copy link
Contributor

There were examples where the inclusion of the panic backtrace with the Exception was very helpful. I can understand limiting the length/size of the resulting stacktrace, but there is a lot of utility in including at least some of the information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants