-
-
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
Sentry Express + Serverless losing transaction #13871
Comments
Hey @bvanderdrift thanks for writing in! First off: Are you transpiling your application to CommonJS ( I'll try to reproduce this in the meantime |
To confirm because I'm not sure if I understood the problem correctly:
|
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 |
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:
|
Sorry for confusion
Let me get back to you on the transpiling in a minute |
This is the transpiled code, I'm actually not sure whether this is cjs or esm. Guess CJS since I see "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 |
Didn't spot any differences (except for the mentioned ones by you). Curious to hear if that did succeed. |
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 |
I'm by the way completely new to |
Right, sorry missing some context there. This is the plugins we use in 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 |
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 |
Thanks @Lms24 really appreciate the fast response 🙏 |
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 This is happening because serverless-offline creates a server around the lambda handler that catches the We can somewhat work around this by calling 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. |
Offline WorkaroundOk turns out, there's a workaround for local development with sls offline: Start the offline sls server with Let's see how all of this behaves in deployed environments |
Deployed stateIt seems like in deployed state, the http instrumentation also doesn't properly kick in, meaning we don't get 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:
This won't be perfect though. In my testing, the transaction names have been a bit off but it's probably better than nothing. |
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. |
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! |
@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 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. |
@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 |
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
Steps to Reproduce
Run the above file as a single file using
serverless
andserverless-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 usingnode index.js
, which successfully logs the error, so it must be specific to theserverless-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
Actual Result
Also missing headers + query params
The text was updated successfully, but these errors were encountered: