Skip to content
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

Sentry Express + Serverless losing transaction #13871

Closed
3 tasks done
bvanderdrift opened this issue Oct 4, 2024 · 19 comments
Closed
3 tasks done

Sentry Express + Serverless losing transaction #13871

bvanderdrift opened this issue Oct 4, 2024 · 19 comments
Assignees
Labels
Package: node Issues related to the Sentry Node SDK

Comments

@bvanderdrift
Copy link

bvanderdrift commented Oct 4, 2024

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/node

SDK Version

8.33.1

Framework Version

express 4.18.2, serverless-http 3.1.1

Link to Sentry event

https://amplify-30.sentry.io/issues/5951306644/?environment=development&project=4506665822584832&query=is%3Aunresolved%20issue.priority%3A%5Bhigh%2C%20medium%5D&referrer=issue-stream&statsPeriod=1h&stream_index=0

Reproduction Example/SDK Setup

import {init, setupExpressErrorHandler} from '@sentry/node';
import {nodeProfilingIntegration} from '@sentry/profiling-node';

init({
  dsn: process.env.SENTRY_DSN,
  environment: 'development',

  integrations: [nodeProfilingIntegration()],

  tracesSampleRate: 1.0,
  profilesSampleRate: 1.0,
});

import express from 'express';
import serverless from 'serverless-http';

const app = express();

app.get('/debug-sentry/:test', (req, res) => {
  throw new Error('My first Sentry error!');
});

setupExpressErrorHandler(app);

export const handler = serverless(app);

Steps to Reproduce

Run the above file as a single file using serverless and serverless-offline plugin.

Then run curl http://localhost:3000/debug-sentry/hello

I've removed the serverless part and just started it as a raw express server using node index.js, which successfully logs the error, so it must be specific to the serverless-http wrapper.

I've also tried using @sentry/aws-serverless instead of @sentry/node which sadly logs nothing, presumably since express already handles the error.

Sentry 7 is logging errors correctly with the serverless + express combo.

Expected Result

Issue with URL, Headers & Query params

Image

Actual Result

Also missing headers + query params

Image

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Oct 4, 2024
@github-actions github-actions bot added the Package: node Issues related to the Sentry Node SDK label Oct 4, 2024
@Lms24
Copy link
Member

Lms24 commented Oct 7, 2024

Hey @bvanderdrift thanks for writing in! First off: Are you transpiling your application to CommonJS (require syntax) or does it stay ESM (import syntax) as you showed in your example? Please note that with SDK v8, you there are specific instructions to ESM where only importing on top if the file doesn't work anymore: https://docs.sentry.io/platforms/javascript/guides/node/install/

I'll try to reproduce this in the meantime

@getsantry getsantry bot removed the status in GitHub Issues with 👀 3 Oct 7, 2024
@Lms24 Lms24 self-assigned this Oct 7, 2024
@Lms24
Copy link
Member

Lms24 commented Oct 7, 2024

To confirm because I'm not sure if I understood the problem correctly:

  • Using v8 of the SDK with exactly the file you provided (i.e. with serverless): No errors are caught and the fields from the screenshot are missing
  • Using v7 of the SDK with exactly the file you provided (i.e. with serverless): Errors are caught and all fields are present?

@Lms24
Copy link
Member

Lms24 commented Oct 7, 2024

Using v8 of the SDK with exactly the file you provided (i.e. with serverless): No errors are caught and the fields from the screenshot are missing

I cannot reproduce this fully. In my reproduction example, the error successfully sent to Sentry. However, I can reproduce that information about the request (url, status) as well as the transaction field are indeed missing. I'll dig a bit deeper to see what's going on here.

@Lms24
Copy link
Member

Lms24 commented Oct 7, 2024

Here's the link to my reproduction repo: https://github.com/Lms24/gh-sentry-javascript-13871-node-express-serverless-missing-data

Please take a look if there are any obvious differences!

I did make some adjustments from your initial example:

  • Assuming you run your function in ESM (import syntax), I added an instrumentation.mjs file as explained in our docs and started it by adding NODE_OPTIONS: --import ./instrumentation.mjs to the serverless environment variables
  • Removed profinling-node for now to reduce problem space

@bvanderdrift
Copy link
Author

Sorry for confusion

  • On v8: I do get the errors, just the transaction data is missing
  • On v7: I do get the error, including transaction data.

Let me get back to you on the transpiling in a minute

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Oct 7, 2024
@bvanderdrift
Copy link
Author

bvanderdrift commented Oct 7, 2024

This is the transpiled code, I'm actually not sure whether this is cjs or esm. Guess CJS since I see require.

"use strict";
var __importDefault = (this && this.__importDefault) || function (mod) {
    return (mod && mod.__esModule) ? mod : { "default": mod };
};
Object.defineProperty(exports, "__esModule", { value: true });
exports.handler = void 0;
var node_1 = require("@sentry/node");
var profiling_node_1 = require("@sentry/profiling-node");
(0, node_1.init)({
    dsn: process.env.SENTRY_DSN,
    environment: 'development',
    integrations: [(0, profiling_node_1.nodeProfilingIntegration)()],
    tracesSampleRate: 1.0,
    profilesSampleRate: 1.0,
});
var express_1 = __importDefault(require("express"));
var serverless_http_1 = __importDefault(require("serverless-http"));
var app = (0, express_1.default)();
app.get('/debug-sentry/:test', function (req, res) {
    throw new Error('My first Sentry error!');
});
(0, node_1.setupExpressErrorHandler)(app);
exports.handler = (0, serverless_http_1.default)(app);
//# sourceMappingURL=index.js.map

@bvanderdrift
Copy link
Author

Here's the link to my reproduction repo: https://github.com/Lms24/gh-sentry-javascript-13871-node-express-serverless-missing-data

Please take a look if there are any obvious differences!

Didn't spot any differences (except for the mentioned ones by you). Curious to hear if that did succeed.

@Lms24
Copy link
Member

Lms24 commented Oct 7, 2024

Ok so you're running in CJS. I'm currently in ESM mode in my repro app. Will try to change that and report back

@Lms24
Copy link
Member

Lms24 commented Oct 7, 2024

I'm by the way completely new to serverless. Never used it before. How did you get to the transpiled output? I'm currently only running serverless offline and I don't see any build output.

@bvanderdrift
Copy link
Author

bvanderdrift commented Oct 7, 2024

Right, sorry missing some context there. This is the plugins we use in serverless.yml; I think the transpiling is combo of typescript + jetpack.

plugins:
  - serverless-plugin-typescript
  - serverless-offline-sqs # Don't think this is related, but including nonetheless
  - serverless-offline
  - serverless-dotenv-plugin
  - ./scripts/sentry-plugin.js
  - serverless-jetpack

Here is the custom sentry plugin, because I'm sure you're curious.

const {execSync} = require('child_process');

/**
 * Since sls doesn't have seperate CLI commands for
 * - package/build
 * - package/zip
 *
 * We need to create a plugin that will run before the package/zip command but after the package/build command
 * in order to inject & upload sentry sourcemaps
 */

class MyPlugin {
  constructor() {
    this.hooks = {
      'before:package:createDeploymentArtifacts': () => {
        console.log('Starting Sentry injecting & deploying');
        execSync('npm run sentry:sourcemaps', {
          stdio: 'inherit',
        });
        console.log('Sentry injecting & deploying done');
      },
    };
  }
}

module.exports = MyPlugin;

And then in turn sentry:sourcemaps: sentry-cli sourcemaps inject --org amplify-30 --project backend ./.build/src && sentry-cli sourcemaps upload --org amplify-30 --project backend ./.build/src

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Oct 7, 2024
@Lms24
Copy link
Member

Lms24 commented Oct 7, 2024

Thanks for the information. I can reproduce the same issue with my more minimal example now running in CJS (as well as previously in ESM). So far from what I tested, it seems like there likely is a bug in OpenTelemetry's Express and Http instrumentations (which we use in v8 of our Node SDK) when using serverless(-http). The strange thing is that it seems like the incoming requests are not triggering any kind of otel instrumentation. It should create spans, a trace, extract the request data and transaction name (which you reported missing) but it's doing none of that. I'll try to investigate why

@bvanderdrift
Copy link
Author

Thanks @Lms24 really appreciate the fast response 🙏

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Oct 7, 2024
@Lms24
Copy link
Member

Lms24 commented Oct 7, 2024

So I think there's a fundamental problem with how the serverless framework adds code around the lambda handler. What I'm not sure yet is how local behaviour (via serverless-offline) differs from behaviour of a deployed lambda function.

The root problem: Our httpIntegration, i.e. OpenTelemetry's HttpInstrumentation is not triggered when running the lambda handler via serverless-offline.

This is happening because serverless-offline creates a server around the lambda handler that catches the curl requests to the lambda functions. Then, it internally fires up a runner in a Node worker thread that executes the lambda handler. This means that the http instrumentation doesn't actually trigger because there's no real Http request at the time of invoking the handler.

We can somewhat work around this by calling NODE_OPTIONS="-r ./instrumentation.js" sls offline which fires up the Sentry SDK around the sls-offline server. This will cause the sls-offline server to create and send an http.server root span (and transaction name) for the original incoming request. However, because the runner starts the handler in a new worker thread, the async context is lost and the error thrown in the handler isn't tied to the http.server trace.

So TLDR: At least in sls-offline: We currently cannot automatically instrument incoming http requests like in a conventional express application because sls-offline builds a server around the server 😬

Now if this is only broken locally, I think it's fine. The real question is how does all of this behave when you deploy the function? Have you tested this by any chance?

I'll try to get this working on my end as well. I also might have somewhat of a workaround for getting better local data but not sure yet if it's applicable and how it interops with deployed lambdas. Please keep in mind, I have no deep understanding of Serverless, so take what I say here with a grain of salt.

@Lms24
Copy link
Member

Lms24 commented Oct 7, 2024

Offline Workaround

Ok turns out, there's a workaround for local development with sls offline: Start the offline sls server with sls offline --useInProcess. This will cause the server to run the handler in-process and not in a worker thread. For me this fixed the missing transaction name.

Let's see how all of this behaves in deployed environments

@Lms24
Copy link
Member

Lms24 commented Oct 7, 2024

Deployed state

It seems like in deployed state, the http instrumentation also doesn't properly kick in, meaning we don't get http.server or request spans out of the box. Neither do we get proper scope isolation per request nor the transaction name out of the box. You can work around some of this by manually adding our AWS Lambda handler from @sentry/aws-serverless:

const Sentry = require("@sentry/aws-serverless");

Sentry.init(...);

const express = require("express");
const serverless = require("serverless-http");

const app = express();

app.get("/debug-sentry/:test", (req, res) => {
    throw new Error("My first Sentry error!");
});

Sentry.setupExpressErrorHandler(app);

module.exports.handler = Sentry.wrapHandler(serverless(app));

This should give you:

  • a transaction name
  • a function.aws-lambda span
  • error catching
  • ensure that we flush out events before the lambda function terminates.

This won't be perfect though. In my testing, the transaction names have been a bit off but it's probably better than nothing.
Technically, our @sentry/aws-lambda SDK also exports OTel's AWS Lambda instrumentation. I can see from the logs that it gets loaded but it doesn't wrap anything. This is something we should figure out long-term but for now, I hope the workaround is good enough.

@bvanderdrift
Copy link
Author

Thanks @Lms24. Server around a server, sounds like a headache 😅. Really appreciate the extensive write-up. Indeed we hadn't deployed this, but I see you managed to get a deployment done yourself.

Since v7 is stable for us we decided internally to stick to v7 now. If we ever do decide to upgrade I will report back what we find.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Oct 7, 2024
@Lms24
Copy link
Member

Lms24 commented Oct 7, 2024

Considering that this worked better in v7 I think that's a fine decision. Still thanks for letting us know, I learned a lot about sls today 😅

I'll close this for now as I don't see us doing much to improve this at the moment. Please feel free to reach out if you have additional problems. thanks!

@mridang
Copy link

mridang commented Oct 24, 2024

@bvanderdrift I ran into the same issue. I was using a NestJS app with the Express transport and the serverless-express adapter. https://github.com/CodeGenieApp/serverless-express It is rather similar to @Lms24 usage of serverless-http.

I think you're right on the money that the http instrumentation doesn't kick in because both serverless-http and serverless-express don't create HTTP requests into Express but simply use the express.handle method.

The usage of Express, Nest and the serverless-express does seem a little clunky but I don't see any other way to run Nest/Express on Lambda without the wrapper. I'll try to use the workaround you suggested.

@relm923
Copy link

relm923 commented Nov 8, 2024

@bvanderdrift we settled on a similar approach to what you have above. Unfortunately this appears to drop all the inbound HTTP/request integrations. We lose the ability to see the inbound request in the Sentry dashboard and the ability to filter client on the request itself.

Any suggestions for how to get AWS & inbound request information on v8? This worked out of the box with v7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: node Issues related to the Sentry Node SDK
Projects
Archived in project
Development

No branches or pull requests

4 participants