Next.js integration

[tomaspinho]

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 as GET [unknown route]. Moved to using a custom express server instead.

Had to change the default express instrumentation a bit, so made our own middleware, like so:

const { parse } = require('url');

function expressMiddleware(appsignal) {
  return function (req, res, next) {
    const tracer = appsignal.tracer();
    const rootSpan = tracer.currentSpan();

    if (!rootSpan) {
      return next();
    }

    if (req.params.password) {
      rootSpan.setSampleData('params', {
        ...req.params,
        password: '[FILTERED]',
      });
    } else {
      rootSpan.setSampleData('params', req.params);
    }

    return tracer.withSpan(rootSpan, (span) => {
      const originalEnd = res.end;

      tracer.wrapEmitter(req);
      tracer.wrapEmitter(res);

      res.end = function (...args) {
        res.end = originalEnd;

        const { method = 'GET' } = req;

        // if there is no error passed to `next()`, the span name will
        // be updated to match the current path
        span.setName(`${method} ${parse(req.originalUrl).pathname}`);

        span.set('method', method).set('status_code', res.statusCode);
        span.set('qs', req.query);

        return res.end.apply(this, args);
      };

      return next();
    });
  };
}

function expressErrorHandler(appsignal) {
  return function (err, req, res, next) {
    const span = appsignal.tracer().currentSpan();

    if (!span) {
      return next();
    }

    // if there's no `status` property, forward the error
    // we also ignore client errors here
    if (err && (!err.status || (err.status && err.status >= 500))) {
      span.addError(err);
    }

    return next(err);
  };
}

module.exports = {
  expressMiddleware,
  expressErrorHandler,
};

Relevant changes are the insertion of the querystring arguments as part of the span and usage of parse(req.originalUrl).pathname instead of req.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.

[adamyeats]

Hey again @tomaspinho! 👋

req.route.path is actually the path of the route that matches, not the path portion of the original URL.

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 [unknown route] if we do not match to a route in Express' route handler.

This will mean we'll need a fully-fledged integration for Next.js, so this can now become our tracking issue for that.

Also, the span itself has no drilldown available, listing a single unknown event.

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.

[tomaspinho]

Hi @xadamy ! :D

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 [unknown route] if we do not match to a route in Express' route handler.

Yes, this definitely makes a ton of sense and I attempted partially doing that by using just the pathname part of the URL - which definitely is not enough. Next.js probably needs a page-aware sample namer. I'll try helping out with this once I have some free time.

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.

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.

[adamyeats]

Next.js probably needs a page-aware sample namer. I'll try helping out with this once I have some free time.

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.

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.

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).

[tomaspinho]

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.

[backlog-helper]

While performing the daily checks some issues were found with this issue.

• This issue has not had any activity in 14 days. Please provide a status update if it is still relevant. Closed it if it is no longer relevant. Or move it to another column if it's blocked on requires another look at it. - (More info)

---

New issue guide | Backlog management | Rules | Feedback

[tomaspinho]

Was the package removed from NPM? can't install it

npm install --save @appsignal/nextjs
npm ERR! code E404
npm ERR! 404 Not Found - GET https://registry.npmjs.org/@appsignal%2fnextjs - Not found
npm ERR! 404 
npm ERR! 404  '@appsignal/nextjs@latest' is not in the npm registry.
npm ERR! 404 You should bug the author to publish it (or use the name yourself!)
npm ERR! 404 
npm ERR! 404 Note that you can also install from a
npm ERR! 404 tarball, folder, http url, or git url.

EDIT: nvm, noticed that the package hasn't been published yet. Working with a self-publish.

[adamyeats]

@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.

[tomaspinho]

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.