Feat: Add "Plugin Crash" to messages logged when plugins crash within v8 to improve error observability #450
Conversation
Signed-off-by: Rachel Green <rachgreen@google.com>
Signed-off-by: Rachel Green <rachgreen@google.com>
… is shared across all runtimes. Signed-off-by: Rachel Green <rachgreen@google.com>
rachgreen33
requested review from
PiotrSikora,
leonm1,
martijneken and
mpwarres
as code owners
src/v8/v8.cc
Outdated
| std::string V8::getFailMessage(std::string_view function_name, wasm::own<wasm::Trap> trap) { | ||
| auto message = "Function: " + std::string(function_name) + " failed: "; | ||
| std::string V8::getPluginFailMessage(std::string_view function_name, wasm::own<wasm::Trap> trap) { | ||
| auto message = "Plugin Crash: Function: " + std::string(function_name) + " failed: "; |
There was a problem hiding this comment.
bikeshedding, how about: "Plugin failure in function " + std::string(function_name) + ": "
| void fail(FailState fail_state, std::string_view message) { | ||
| integration()->error(message); | ||
| if (fail_state == FailState::RuntimeError) { | ||
| integration()->error(std::string(PluginCrashPrefix) + std::string(message)); |
There was a problem hiding this comment.
Looking at this again, it feels slightly off to me to (1) be singling out FailState::RuntimeError for a particular prefix, and (2) be making this message customization choice on behalf of all users of proxy-wasm-cpp-host, as opposed to letting integrations decide how/whether they want to tweak the error message.
I'd propose instead doing one of the following:
A. Adding the FailState value as an argument to WasmVmIntegration::error(), and leave it up to the WasmVmIntegration implementation how it wants to customize error messages for RuntimeError (or other FailState types). In order to avoid having this be a breaking change for downstream consumers of proxy-wasm-cpp-host (e.g. Envoy, Google Service Extensions, Apache, etc.), it could be added like this:
// Integrator specific WasmVm operations.
struct WasmVmIntegration {
// ...
virtual void error(std::string_view message) = 0;
virtual void error(FailState fail_state, std::string_view message) { error(message); }
};
B. Leaving it up to the WasmVmIntegration::error() implementation to check the WasmVm::failed_ FailState value if it wants to adjust the message based on FailState. In that case, no change would be needed in OSS proxy-wasm-cpp-host, but our own WasmVmIntegration subclass implementation of error() would need to (1) be passed a raw (i.e. non-owning) pointer to the WasmVm in its constructor, and (2) use that pointer to access WasmVm::failed_ when deciding what message string to log.
Either one SGTM, though (A) seems simpler to me.
2 participants
Add a "Plugin crash: " prefix to logs indicating plugin runtime errors. Without this change, plugin crashes are not clear to the customer in the logs. For example: For v8, see stack traces in Cloud Logging that just say "function: failed" (see logs example)