-
Notifications
You must be signed in to change notification settings - Fork 9
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
Next.js integration #100
Comments
Hey again @tomaspinho! 👋
Indeed, and this is intentional as we need to group spans together via the route-matched path. The core concept of it is that you have a limited number of controller actions/routes/etc. If the path contains dynamic elements, this could create thousands of permutations of names, and that will break processing for that app, plus make AppSignal unusable. This is why we currently fallback to This will mean we'll need a fully-fledged integration for Next.js, so this can now become our tracking issue for that.
This is a bug in the span processor (not in this library), which we have a PR open for, so expect a fix imminently for this. |
Hi @xadamy ! :D
Yes, this definitely makes a ton of sense and I attempted partially doing that by using just the
Would it be possible to open source the C++ library that is loaded onto Node? Maybe the community could help ironing out bugs as well. |
This sounds great! Feel free to have a go at implementing this yourself if you have the time, we'd be happy to accept a PR for this. Otherwise, I can take a look sometime within the next couple of weeks.
Great question! In this case, the bug in question is in our processor code, which is server side, and will probably not be open-sourced. The extension is located here: https://github.com/appsignal/appsignal-nodejs/blob/master/packages/nodejs-ext/ext/appsignal_extension.cpp, however the actual agent code that this talks to is written in Rust, and although we plan you make this open source in the near future, we haven't got a timeline for this (yet). |
Yes, that was the codebase I meant, assumed it was written in C++ because of the interface code. |
While performing the daily checks some issues were found with this issue.
|
Was the package removed from NPM? can't install it
EDIT: nvm, noticed that the package hasn't been published yet. Working with a self-publish. |
@tomaspinho Apologies for the wait, the package is now published! I'd also like to point you in the direction of this, which we are releasing in an update soon. |
Cool stuff indeed! Today I found that the route matching only works for dynamic pages, I added some small changes to make it work for "static" pages too, will add it here shortly. EDIT: const span = appsignal.tracer().currentSpan();
const { pathname } = url.parse(req.url || '/', true);
if (span) {
const routes = app.router.dynamicRoutes || [];
let matched = routes.filter((el) => el.match(pathname))[0];
// Added by us to extend matches
// Match with pages with no dynamic routes
if (app.pagesManifest[pathname] !== undefined) matched = { page: pathname }; sorry for the typescript -> javascript conversion, we're not using typescript. |
I've been doing some work on getting this to integrate with Next.js.
We were using a custom server with Node's default HTTP server implementation
http
as described in https://nextjs.org/docs/advanced-features/custom-server . However, this wasn't correctly capturing the route paths, naming all spans asGET [unknown route]
. Moved to using a customexpress
server instead.Had to change the default express instrumentation a bit, so made our own middleware, like so:
Relevant changes are the insertion of the querystring arguments as part of the span and usage of
parse(req.originalUrl).pathname
instead ofreq.route.path
.req.route.path
is actually the path of the route that matches, not the path portion of the original URL. Because Next.js implements its own handler function, we must match on all*
routes and pass that on to the default handler function. See https://github.com/zeit/next.js/blob/canary/examples/custom-server-express/server.js for an example.Right now, we have spans in AppSignal with request duration, however can't find the span metadata anywhere. Also, the span itself has no drilldown available, listing a single
unknown
event.The text was updated successfully, but these errors were encountered: