-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Introduce Async Hooks into the SDK #7691
Comments
Did a little load testing with our changes with artillery (good tech). This was with production express, that had been warmed up, single route: const express = require("express");
const app = express();
const Sentry = require("@sentry/node");
Sentry.init({
dsn: "https://5382661235ae4a2da41a88dd2a4cc141@o19635.ingest.sentry.io/5338414",
tracesSampleRate: 1.0,
integrations: [
// enable HTTP calls tracing
new Sentry.Integrations.Http({ tracing: true }),
// enable Express.js middleware tracing
new Sentry.Integrations.Express({
// to trace all requests to the default router
app,
// alternatively, you can specify the routes you want to trace:
// router: someRouter,
}),
],
});
// RequestHandler creates a separate execution context using domains, so that every
// transaction/span/breadcrumb is attached to its own Hub instance
app.use(Sentry.Handlers.requestHandler());
// TracingHandler creates a trace for every incoming request
app.use(Sentry.Handlers.tracingHandler());
app.get("/hello/:name", function (req, res) {
res.send("hello " + req.params.name);
});
app.use(Sentry.Handlers.errorHandler());
app.listen(3000);
console.log("Listening on port 3000"); Domains: 2500 req/s
Async Local Storage: 3300 req/s
This represents a 30% improvement in throughput 🚀 first peak is domains - second is async local storage |
Thanks for doing the load testing Abhi! Out of interest, what throughput do you get if you don't set a strategy (ie. it just does the default which calls the callback directly)? |
No strategy enabled is 3600 rps
Around a 8% increase when using ALS, and 60% increase in response time when using domain in the p99 case (oooof) |
Considering we have just the docs left - going to close this issue. Will go back and close the related issues once the release has happened! |
I'm working an update from 6.16 to latest for our app and just want to say I appreciate you guys shipping this. Domains were mentioned in the comments of our code and I was a bit shocked and ended up learning a ton about Sentry today. Also thank you for having this code opensource, very helpful and looks awesome! |
ref: #3660
Let's work toward adding async hooks into the SDK. What we probably want to use is
AsyncLocalStorage
, which was made stable inv16.4.0
, but added inv13.10.0, v12.17.0
Important to note that for Node 8, 10, 12 we should still use domains, and then for Node 14 and above we can use
AsyncLocalStorage
.domain
implementation ofAsyncContextStrategy
#7767AsyncContextStrategy
#7779AsyncLocalStorage
implementation ofAsyncContextStrategy
#7800AsyncContextStrategy
for Node.js version #7804Requirements:
Additional Details:
We can get this done with the following steps.
getCurrentHub
to reference some global strategy for grabbing the current hub. When you initialize the SDK in Node 8-12, it uses a domain strategy. For Node 14+ it'll use async local storage strategy.runWithHub
method that creates a new domain/asynclocalstorage/whatever for that code. Then we can go in and replace domain usage withrunWithHub
instead.The global strategy can just just be registered onto the global object. This means there can only be one global strategy at a time.
We have to add this concept of a strategy to
@sentry/core
, so it can also be used by non-node SDKs (think vercel edge, cloudflare workers etc.).The text was updated successfully, but these errors were encountered: