Add exception stack traces to trace/tail events. #1910
Conversation
kentonv
force-pushed
the
kenton/trace-stacks
branch
from
01764a0
to
a015f0c
Compare
| void addExceptionDetail(Lock& js, kj::Exception& exception, v8::Local<v8::Value> handle) { | ||
| js.tryCatch([&]() { | ||
| Serializer ser(js); | ||
| ser.write(js, JsValue(handle)); |
There was a problem hiding this comment.
Should we be doing anything to bound the amount of data added to an exception?
Edit: Oh, I guess the existing trace size bound would help limit negative effects, and I suppose V8 might have some internal bound on stack trace generation.
| "Should always be possible to unwrap error interface from an object."); | ||
| Worker::Api::ErrorInterface error; | ||
|
|
||
| if (exception.isObject()) { |
There was a problem hiding this comment.
I guess the isObject() check is needed here because the deserialized result isn't necessarily an Object? (But would typically be an Object?)
There was a problem hiding this comment.
Annoyingly, JavaScript permits you to throw any value, not just objects. I think there was a bug here that caused us to fail to log non-object exceptions, but it wasn't noticed before because our exception tunneling would turn non-objects into objects. (Exception tunneling today always produces Error values even if the original wasn't.)
There was a problem hiding this comment.
Ah, OK... that would explain why the current test cases for throw "string" were working without the Object check.
kentonv
force-pushed
the
kenton/trace-stacks
branch
from
a015f0c
to
e57f5bb
Compare
| // want to log the exception even if the isolate has been terminated. | ||
| } | ||
|
|
||
| KJ_IF_SOME(d, deserialized) { |
There was a problem hiding this comment.
Looks like exceptionToJsValue() does some additional property population, filtering, and logging of internal tunneled exceptions that would be skipped when deserialization is successful. I guess the assumption here is that if a valid serialized value is present, it didn't come from an internal tunneled exception? (Or that the other populated properties are not currently consumed by logUncaughtException()?)
There was a problem hiding this comment.
Yeah, logUncaughtException() doesn't inspect any of those extra properties today. Only type, message, and now stack.
A new property, `stack`, contains the stack trace. To support this in the case of async exceptions (which have already been converted to KJ), exception tunneling now adds a serialized copy of the V8 error to the exception itself, as a "detail". For now, this is _only_ used by `logUncaughtException()`. In the future, though, when the exception propagates back into JavaScript, we could de-tunnel it by deserializing this detail, rather than based on the description string. However, there are several complexities that need to be solved first, so I have not tried to do that in this PR. Depends on: capnproto/capnproto#1978
kentonv
force-pushed
the
kenton/trace-stacks
branch
from
e57f5bb
to
aaf2a04
Compare
| KJ_IF_SOME(s, error.stack) { | ||
| kj::StringPtr slice = s; | ||
|
|
||
| // Normally `error.stack` repeats the error type and message first. We don't want send two |
There was a problem hiding this comment.
@kentonv — see internal thread on this, I think by stripping out here to be efficient in what gets sent across, we ended up making it such that the stack property in the Tail Worker ends up not including full stack trace?
There was a problem hiding this comment.
I don't know which internal thread you mean, and I don't see how the code here could possibly be stripping any part of the actual stack trace.
A new property,
stack, contains the stack trace.To support this in the case of async exceptions (which have already been converted to KJ), exception tunneling now adds a serialized copy of the V8 error to the exception itself, as a "detail". For now, this is only used by
logUncaughtException(). In the future, though, when the exception propagates back into JavaScript, we could de-tunnel it by deserializing this detail, rather than based on the description string. However, there are several complexities that need to be solved first, so I have not tried to do that in this PR.Depends on: capnproto/capnproto#1978