Chore/enable rust backtrace in tests

[casimiro]

No description provided.

[coveralls]

Pull Request Test Coverage Report for Build 5834586402

• 6 of 6 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.007%) to 91.021%

---

Totals
Change from base Build 5828356494:	0.007%
Covered Lines:	7542
Relevant Lines:	8286

---

💛 - Coveralls

[thibaultcha]

I don't really understand the purpose of this. We have the backtraces directive to enable detailed backtraces, we shouldn't directly play with environment variables as each runtimes (and underlying SDK whether Rust or Go or other) has different ways of enabling detailed backtraces. If a test is not giving a detailed backtrace and needs individual investigation, it takes very little effort to isolate it and add whatever is necessary to extract more details, whether it be a backtrace or something else.

[thibaultcha]

Besides, adding backtraces on by default to all tests would also defeat the purpose, since it currently allows us to test both backtraces [on|off] throughout the whole test suite (off by default and on in backtraces_directive.t), which has been by design.

The majority of our test suite should (and currently does) run with the default configuration; so unless we force RUST_BACKTRACE by default in Nginx itself (which again, is only something to be done with some runtimes), then our test suite should also avoid running with extra configuration settings.

[thibaultcha]

If anything, I believe it would make more sense to add it directly within the module itself rather than the test suite, to easily get better feedback in case of a crash in proxy-wasm-rust-sdk - the runtimes - thenselves. We could add it unconditionally the same way we set the wasmtime env variable for backtraces.

[thibaultcha]

Oh and it also struck me that some of our runtimes are implemented in Rust, and those are probably the ones you are trying to cover here... 🤦🏼‍♂️

What do you think about:

1. Leaving stub as-is (granted it doesn't cause CI failures with longer backtraces names etc...)
2. Unconditionally add RUST_BACKTRACE through setenv in ngx_wrt_wasmtime.c and ngx_wrt_wasmer.c
3. Removing WASMTIME_BACKTRACE_DETAILS from TestWasm.pm

[casimiro]

Thank you for your careful review @thibaultcha!

To be fair, what motivated these commits was having a convenient way to get Rust backtraces to the logs when investigating an erratic behaviour involving Wasmtime or Wasmer.

As you said, though, it can be easily achieved by adding backtraces on; to the test case under investigation.

I agree with the 3 suggestions, but maybe we only set RUST_BACKTRACE using setenv if backtraces on; is present?
I think it's safe to assume that when users set backtraces on; they expect to have any backtraces available.

That way we can have RUST_BACKTRACE set only when Wasmtime or Wasmer is used, following our current approach to enable WASMTIME_BACKTRACE_DETAILS here.

[thibaultcha]

but maybe we only set RUST_BACKTRACE using setenv if backtraces on; is present?

I was initially going for an unconditional setting when using a Rust-written runtime... But it does seem like RUST_BACKTRACE has some unexpected overhead (e.g. rust-lang/rust-playground#132)... So yes, maybe when backtraces on; might be safer at first.

[thibaultcha]

With all that, it could indeed be interesting to add a --- backtraces block to TestWasm.pm as you were doing in the previous PR! As a way to easily add backtraces on; to any test case without having to add a whole --- main_config block.