Skip to content

Feat: Add "Plugin Crash" to messages logged when plugins crash within v8 to improve error observability #450

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Feat: Add "Plugin Crash" to messages logged when plugins crash within v8 to improve error observability #450

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6cbde81
Added 'Plugin Crash' to the plugin fail message for v8
rachgreen33 Aug 12, 2025
0a148d6
Add plugin crash message to higher level fail function in wasm_vm.h
rachgreen33 Aug 21, 2025
8bdf040
Moved the plugin crash prefix addition to wasm_vm.h, so that behavior…
rachgreen33 Sep 16, 2025
c04e0be
Merge branch 'main' into observability_plugin_crash
rachgreen33 Sep 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion include/proxy-wasm/wasm_vm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,11 @@ class WasmVm {

bool isFailed() { return failed_ != FailState::Ok; }
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));
Copy link
Contributor

@mpwarres mpwarres Oct 1, 2025

Choose a reason for hiding this comment

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

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.

} else {
integration()->error(message);
}
failed_ = fail_state;
for (auto &callback : fail_callbacks_) {
callback(fail_state);
Expand Down Expand Up @@ -345,6 +349,7 @@ class WasmVm {
std::vector<std::function<void(FailState)>> fail_callbacks_;

private:
static constexpr std::string_view PluginCrashPrefix = "Plugin crash: ";
bool restricted_callback_{false};
std::unordered_set<std::string> allowed_hostcalls_{};
};
Expand Down
Loading