Log when actor deserialization fails#881
Conversation
| auto jsException = tryCatch.Exception(); | ||
| if (!tryCatch.HasTerminated()) { | ||
| KJ_LOG(ERROR, "actor storage deserialization failed", errorMsg, jsException, | ||
| key, buf.size(), buf.slice(0, std::min(static_cast<size_t>(3), buf.size()))); |
There was a problem hiding this comment.
Please leave out at least buf if not also key! This could leak personally identifiable information or secrets to logs.
There was a problem hiding this comment.
Hmm, what about the FAIL_ASSERTs below? I directly copied one of them so should that change too?
There was a problem hiding this comment.
Yeah, if you would. As long as we have identifying DO info from our log context, we can check the storage backend later.
There was a problem hiding this comment.
You absolutely sure? This comment indicates there was some thought put into this:
// If we do hit a deserialization error, we log information that will be helpful in understanding
// the problem but that won't leak too much about the customer's data. We include the key (to help
// find the data in the database if it hasn't been deleted), the length of the value, and the
// first three bytes of the value (which is just the v8-internal version header and the tag that
// indicates the type of the value, but not its contents).
We were failing to log this because we'd throw an exception before the log line.
65281cf to
3a77155
Compare
| JSG_REQUIRE(!tryCatch.HasTerminated(), | ||
| Error, "isolate terminated while deserializing value from Durable Object storage; " | ||
| "contact us if you're wondering why you're seeing this"); | ||
| auto jsException = tryCatch.Exception(); |
There was a problem hiding this comment.
suggestion: While you're here, mind scrubbing mentions of "actor storage" or "durable object storage"? We're going to want to use this function outside of the storage api soon. 😄
There was a problem hiding this comment.
There are situations where tryCatch.HasCaught() is true but tryCatch.Exception() returns an empty v8::Local. In such cases, we want to treat it the same as tryCatch.CanContinue() == false
| auto jsException = tryCatch.Exception(); | ||
| if (!tryCatch.HasTerminated()) { | ||
| KJ_LOG(ERROR, "actor storage deserialization failed", errorMsg, jsException, | ||
| key, buf.size(), buf.slice(0, std::min(static_cast<size_t>(3), buf.size()))); |
There was a problem hiding this comment.
Yeah, if you would. As long as we have identifying DO info from our log context, we can check the storage backend later.
| key, buf.size(), buf.slice(0, std::min(static_cast<size_t>(3), buf.size()))); | ||
| JSG_FAIL_REQUIRE(Error, | ||
| "isolate terminated while deserializing value from Durable Object storage; " | ||
| "contact us if you're wondering why you're seeing this"); |
There was a problem hiding this comment.
"Failed to deserialize value into isolate", we were hoping to make this message less misleading!
We were failing to log this because we'd throw an exception before the log line.