-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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.
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
feat(test): improved op sanitizer errors + traces #13676
feat(test): improved op sanitizer errors + traces #13676
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! This should help out users a lot.
* To get high level metrics on async ops with no added performance cost, | ||
* use `Deno.core.metrics()`. | ||
*/ | ||
function enableOpCallTracing(): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just have a boolean value that can be assigned, rather than a function:
Deno.core.enableOpCallTracing = true;
This allows for future flexibility (if you want to turn it off) and less wrapper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The chosen approach is actually intentional: the "enabled" flag is a boolean (a primitive), so assigning it to Deno.core
assigns it by value, not by reference. To fix this, we'd need to change the check in opAsync
to be if (core.opCallTracingEnabled)
instead of if (opCallTracingEnabled)
. The former introduces an additional property lookup to all opAsync
calls which I don't really want as it is a super hot path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - seems like it will really help people understand their programs!
b515bcb
to
1cb2ec9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, great work
1ed2608
to
1cb2ec9
Compare
This commit improves the error messages for the `deno test` async op sanitizer. It does this in two ways: - it uses handwritten error messages for each op that could be leaking - it includes traces showing where each op was started This "async op tracing" functionality is a new feature in deno_core. It likely has a significant performance impact, which is why it is only enabled in tests.
1cb2ec9
to
06b9289
Compare
This commit improves the error messages for the
deno test
async opsanitizer. It does this in two ways:
This "async op tracing" functionality is a new feature in deno_core.
It likely has a significant performance impact, which is why it is only
enabled in tests.