esbuild.serverless.js seems misleading/wrong
#157
Comments
|
@oliversalzburg The .node file is not there yet because we still need to release a new version that enable the changes. I'll look into validity of the esbuild config file |
|
Oh, that explains it then :D Should I try the alpha instead? Or did I miss anything else about how to make the 0.3.0 work with bundling? |
|
@oliversalzburg I am targeting to release 1.0.0 today by eod so I would say best to wait :) |
|
Just FYI, I still wanted to prepare myself by trying the beta.7, but I couldn't get esbuild to copy the Maybe you already have this fixed in the final 1.0.0, but I wanted to bring it up just in case. |
|
@oliversalzburg mind sharing the config? Are you testing on alpha.7? I just release a 1.0.0-beta.1 which we started using internally at Sentry before I release 1.0.0 |
|
This is the entire build script we're using to bundle our lambdas: const argv = require("minimist")(process.argv.slice(2));
const { sentryEsbuildPlugin } = require("@sentry/esbuild-plugin");
const path = require("node:path");
const fs = require("node:fs");
if (!argv.in || !argv.out) {
console.error(
"Usage: node build.cjs --in packages/fn-customer-create/source/main.mts --out packages/fn-customer-create/output/bundle.cjs"
);
process.exit(1);
}
const entryPoint = argv.in ?? "source/main.mts";
const outfile = argv.out ?? "output/bundle.cjs";
const outdir = path.dirname(outfile);
require("esbuild")
.build({
bundle: true,
entryPoints: [entryPoint],
external: [
// These are available through a Lambda layer.
"@aws-lambda-powertools/commons",
"@aws-lambda-powertools/logger",
"@aws-lambda-powertools/tracer",
"@aws-lambda-powertools/metrics",
// Shipped in Lambda environment.
"@aws-sdk/client-dynamodb",
"@aws-sdk/client-s3",
"@aws-sdk/client-sso",
"@aws-sdk/client-sts",
// Breaks bundling when using @middy/secrets-manager.
"@aws-sdk/client-secrets-manager",
// this breaks bundling but is not needed by auth0 - see https://auth0.github.io/node-auth0/
"superagent-proxy",
],
format: "cjs",
loader: {
".node": "copy",
},
logLevel: "debug",
metafile: true,
minify: Boolean(argv.minify),
outdir,
platform: "node",
plugins: process.env.SENTRY_AUTH_TOKEN
? [
sentryEsbuildPlugin({
project: "dcc-backend",
org: "dyn-media-gmbh",
authToken: process.env.SENTRY_AUTH_TOKEN,
release: process.env.SENTRY_VERSION,
telemetry: false,
}),
]
: [],
sourcemap: true,
target: "node18",
})
.then(result => {
fs.writeFileSync(path.resolve(outdir, "meta.json"), JSON.stringify(result.metafile));
})
.catch(() => process.exit(1));Given that there are explicit |
|
@oliversalzburg I just tested beta-1 with esbuild ^0.17.18 with the following config and I can see all the binary files moved. Do you mind 
Would you mind testing with the same config as I am using to see if it works for you? If it doesn't, then I would proceed by enabling the other options to see if any of them break the binary copy procedure |
|
I have esbuild 0.17.19 over here. It's still not behaving as it should with your config. I'll try to find out what's special on my side :( |
|
It seems like this is related to the format of the files that are being bundled. We have When I create a const ProfilingPlugin = require("@sentry/profiling-node");
new ProfilingPlugin();then the To double-check, I created import { ProfilingIntegration } from "@sentry/profiling-node";
new ProfilingIntegration();As expected, this will then not copy the Can't say that I understand this though :( |
|
That is unfortunate and I do not quite understand the reasoning behind that. I think the best next step would be to mark @sentry/profiling-node as an external package and opening an issue on esbuild repository regarding the copying of binaries. |
|
I actually initially started with the I was wondering if I could just introduce the binaries into the deployment by using the Lambda layer, but I haven't looked into this option yet. |
|
Yeah, @oliversalzburg if you mark the package as external, then you need to also ensure it's installed - I think someone else had this problem here and they managed to make it work. It's a shame .mts does not end up copying files which would make this work out the box. An alternative that maybe is not the most pretty, but will work is to not mark the package as external and copy the binaries yourself. Something like |
JonasBa
added
the
💾 💾 installation 💾 💾
|
After the most recent feedback, I think I will just have to find a way to copy the binaries manually. Very interesting insights though. I just really wanted to avoid the manual copy, because it feels so fragile. Maybe paths change in the future, or the location of packages changes in the monorepo. I'd just feel better if it was fully automated through bundling :D |
|
Interestingly, when I install the const pluginNodeProfiling = {
name: "sentry-profiling-node",
setup(build) {
build.onEnd(result => {
const moduleEntrypoint = require.resolve("@sentry/profiling-node");
const moduleBasePath = path.dirname(moduleEntrypoint);
for (const entry of fs.readdirSync(moduleBasePath)) {
if (!entry.endsWith(".node")) {
continue;
}
const fileSource = path.resolve(moduleBasePath, entry);
const fileOutput = path.resolve(outdir, entry);
fs.copyFileSync(fileSource, fileOutput);
}
});
},
}; |
|
After deploying this, I see new problems. My deployed code will fail on the I saw your latest comment on the esbuild thread. If you're going to make any adjustments to the plugin, I'd be happy to give those a test-drive :) |
|
Yes @adesso-os, I made some changes and will release beta.2 soon that hopefully fixes this |
|
@oliversalzburg @adesso-os mind testing 1.0.0-beta.2 as it should now be using the correct version of require? |
|
That seems to be working beautifully :) esbuild also now specifically complains if I don't specify a loader for the |
|
Perfect, sorry for the back and forth, hope you enjoy using profiling! |
|
@adesso-os worth noting that I will release a first major version in the next day or so. Feel free to upgrade to that |
|
I feel like something might still be off, but I will create a new ticket for it when I'm back to work. I'm now seeing new runtime errors (
|
|
@oliversalzburg I think there is a chance that other sentry modules are not fully esm compatible. Can you post a stack trace so I can see what is happening here? |
|
The stack trace is: I looked at that, and the line in question seems to be an import in In the bundle of of the profiling-node plugin, there are these suspicious imports: Not sure why it would first wrap the CJS of @sentry/utils and @sentry/node, and then try to convert that back to ESM, when those modules already provide ESM exports. I also looked at the Hope you have more luck. |
|
@oliversalzburg, mind trying to upgrade to 1.0.2 and letting me know if it works? There is a chance that our dependency here is not esm compatible... |
|
With 1.0.2 I'm back to missing the The tests pass now. So that error is gone, but I'm surely back to |
|
Ok, will revert that change then, ESM with native modules is just a mine field of edge cases and it seems we wont be able to make all of them work. In this particular case, it seems that .node files just cannot be loaded via imports in esm in sveltekit. I will revert that change |
|
Reverted this in 1.0.3 |
|
@oliversalzburg just a thought, but what happens if you import @sentry/profiling-node from @sentry/profiling-node/lib/index.js? |
|
@JonasBa Basically only results in other build errors: |
|
So I'm looking at the As |
|
I also tried going back to CJS bundle, just to see if that works, but that also leads to new errors: When I'm building a CJS bundle, I don't understand why it will contain parts of the plugin code that uses Either way, I'll probably have to give up on Sentry profiling now, as this was supposed to be a quick task and I'm way over budget. Thanks for the efforts. |
|
@oliversalzburg thanks for the contributions though. I'm considering just removing esm support from the bundle as the pain does not seem to be worth it in this case and there just seem to be edge case after edge case of issues. If you would still like to use sentry profiling, I would go with marking @sentry/profiling-node as an external library and installing it separately, that should work without any issues |
|
I will likely come back to this at a later time, but the way our Lambda bundling/deployment works right now, setting things as external is not a full solution for us. In general, we don't necessarily want to use ESM for the bundle, but the CJS approach also didn't work for us initially. We'll definitely come back to this, I just can't invest any more right now. Your efforts are appreciated nonetheless :) |
|
Just FYI, with the latest release, I was also able to make profiling work on our project. I noticed something interesting though. I build all our lambdas in a single process. I just provide all the entrypoints and let esbuild write the output directory structure. In that scenario, the I resolved that with the plugin below, which also copies the file to the output root, but rewrites the import to be // We want the profiling binary to reside at the root of each lambda folder.
// We ensure manually that the right plugin is bundled together with each lambda.
// This plugin ensures that the import paths in the build output refer to the
// correct locations.
const sentryProfilingImportPathPlugin = {
name: "SentryProfilingImportPath",
setup(build) {
build.onResolve({ filter: /sentry_cpu_profiler.+\.node$/ }, async resolveArgs => {
fs.mkdirSync(pathOutput, {
recursive: true,
});
fs.copyFileSync(
path.resolve(resolveArgs.resolveDir, resolveArgs.path),
path.resolve(pathOutput, resolveArgs.path)
);
return { path: resolveArgs.path, external: true };
});
},
};And then copying to all output directories at the end of the build: // Try finding the right Sentry profiling plugin for our arch.
const profilingPlugins = fs
.readdirSync(pathOutput)
.filter(file => file.match(/sentry_cpu_profiler/));
const profilingPlugin = profilingPlugins.filter(file => file.match(new RegExp(arch)))[0];
if (!profilingPlugin) {
console.warn(
`Cannot find matching Sentry profiling plugin for '${arch}'! Sentry profiling will not work in this build!`
);
} else {
// Copy the plugin to all output directories.
console.info(
`Found '${profilingPlugins.length}' Sentry profiling plugin candidates in build output. Desired architecture is '${arch}'.`
);
console.info(
` + Choosing: '${profilingPlugin}' and associating with '${lambdaEntrypoints.length}' lambda outputs...`
);
for (const lambdaMain of lambdaEntrypoints) {
fs.copyFileSync(
path.resolve(pathOutput, profilingPlugin),
path.resolve(path.dirname(lambdaMain.path), profilingPlugin)
);
}
}Maybe this helps someone else :) |
Is there an existing issue for this?
SDK Version
0.3.0
What environment is your node script running in?
AWS Lambda
Are you using an alpine based docker image?
Does the failure occur on a remote environment only?
Are you using alpine based node docker image. If so, what version are you using? e.g node:16-alpine3.12
No response
If you are using alpine, have you tried using latest version of alpine or (currently alpine3.16 or alpine3.17)?
Have you tried using the latest minor version of node (currently 16.20 or 18.15)?
If you run npm install --verbose --foreground-scripts @sentry/profiling-node in an empty directory, what is the output?
install.log
Crash report
No response
Actual Issue Report
The example in the
READMEsuggests to useoutfile: "./dist",in the esbuild configuration, which is an invalid setting foroutfile.When using the correct
outdirinstead, there are still no.nodefiles being copied at all, resulting in MODULE_NOT_FOUND errors down the line.The text was updated successfully, but these errors were encountered: