-
-
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
Fastify is not instrumented in combination with node profiling and tracesSampleRate #13662
Comments
Hi @Xhale1, thanks for filing this. I can confirm this is indeed not working, thank you for the reproduction. We're going to look into this. One thing to note: The reason you're not seeing the issue when So definitely some issue with using cc @JonasBa |
As a workaround, you can leave out the integration inside import * as Sentry from "@sentry/node";
import { nodeProfilingIntegration } from "@sentry/profiling-node";
import Fastify from "fastify";
const fastify = Fastify();
Sentry.setupFastifyErrorHandler(fastify);
Sentry.addIntegration(nodeProfilingIntegration()) |
@andreiborza @Xhale1 are you seeing any profiling SDK logs if you set debug: true to the SDK init? |
@JonasBa Sentry Logger [log]: Initializing Sentry: process: 65271, thread: main.
Sentry Logger [log]: Integration installed: InboundFilters
Sentry Logger [log]: Integration installed: FunctionToString
Sentry Logger [log]: Integration installed: LinkedErrors
Sentry Logger [log]: Integration installed: RequestData
Sentry Logger [log]: Integration installed: Console
Sentry Logger [log]: Integration installed: Http
Sentry Logger [log]: Integration installed: NodeFetch
Sentry Logger [log]: Integration installed: OnUncaughtException
Sentry Logger [log]: Integration installed: OnUnhandledRejection
Sentry Logger [log]: Integration installed: ContextLines
Sentry Logger [log]: Integration installed: LocalVariablesAsync
Sentry Logger [log]: Integration installed: Context
Sentry Logger [log]: Integration installed: Modules
Sentry Logger [log]: Integration installed: Express
Sentry Logger [log]: Integration installed: Fastify
Sentry Logger [log]: Integration installed: Graphql
Sentry Logger [log]: Integration installed: Mongo
Sentry Logger [log]: Integration installed: Mongoose
Sentry Logger [log]: Integration installed: Mysql
Sentry Logger [log]: Integration installed: Mysql2
Sentry Logger [log]: Integration installed: Redis
Sentry Logger [log]: Integration installed: Postgres
Sentry Logger [log]: Integration installed: Nest
Sentry Logger [log]: Integration installed: Hapi
Sentry Logger [log]: Integration installed: Koa
Sentry Logger [log]: Integration installed: Connect
Sentry Logger [log]: Integration installed: GenericPool
Sentry Logger [log]: Integration installed: Kafka
Sentry Logger [log]: [Profiling] Profiling integration setup.
Sentry Logger [log]: [Profiling] Continuous profiler mode enabled.
Sentry Logger [log]: Integration installed: ProfilingIntegration
Sentry Logger [log]: Running in CommonJS mode.
Sentry Logger [debug]: @opentelemetry/api: Registered a global for diag v1.9.0.
Sentry Logger [debug]: @opentelemetry/api: Registered a global for trace v1.9.0.
Sentry Logger [debug]: @opentelemetry/api: Registered a global for context v1.9.0.
Sentry Logger [debug]: @opentelemetry/api: Registered a global for propagation v1.9.0.
Sentry Logger [debug]: @opentelemetry/instrumentation-http Applying instrumentation patch for nodejs core module on require hook { module: 'http' }
Sentry Logger [debug]: @opentelemetry/instrumentation-http Applying instrumentation patch for nodejs core module on require hook { module: 'https' }
[Sentry] fastify is not instrumented. This is likely because you required/imported fastify before calling `Sentry.init()`.
Server listening at http://0.0.0.0:3000 Without node profiling (reports as instrumented as far as I'm aware Sentry Logger [log]: Initializing Sentry: process: 65327, thread: main.
Sentry Logger [log]: Integration installed: InboundFilters
Sentry Logger [log]: Integration installed: FunctionToString
Sentry Logger [log]: Integration installed: LinkedErrors
Sentry Logger [log]: Integration installed: RequestData
Sentry Logger [log]: Integration installed: Console
Sentry Logger [log]: Integration installed: Http
Sentry Logger [log]: Integration installed: NodeFetch
Sentry Logger [log]: Integration installed: OnUncaughtException
Sentry Logger [log]: Integration installed: OnUnhandledRejection
Sentry Logger [log]: Integration installed: ContextLines
Sentry Logger [log]: Integration installed: LocalVariablesAsync
Sentry Logger [log]: Integration installed: Context
Sentry Logger [log]: Integration installed: Express
Sentry Logger [log]: Integration installed: Fastify
Sentry Logger [log]: Integration installed: Graphql
Sentry Logger [log]: Integration installed: Mongo
Sentry Logger [log]: Integration installed: Mongoose
Sentry Logger [log]: Integration installed: Mysql
Sentry Logger [log]: Integration installed: Mysql2
Sentry Logger [log]: Integration installed: Redis
Sentry Logger [log]: Integration installed: Postgres
Sentry Logger [log]: Integration installed: Nest
Sentry Logger [log]: Integration installed: Hapi
Sentry Logger [log]: Integration installed: Koa
Sentry Logger [log]: Integration installed: Connect
Sentry Logger [log]: Integration installed: GenericPool
Sentry Logger [log]: Integration installed: Kafka
Sentry Logger [log]: Running in ESM mode.
Sentry Logger [debug]: @opentelemetry/api: Registered a global for diag v1.9.0.
Sentry Logger [debug]: @opentelemetry/api: Registered a global for trace v1.9.0.
Sentry Logger [debug]: @opentelemetry/api: Registered a global for context v1.9.0.
Sentry Logger [debug]: @opentelemetry/api: Registered a global for propagation v1.9.0.
Sentry Logger [debug]: @opentelemetry/instrumentation-http Applying instrumentation patch for nodejs core module on require hook { module: 'http' }
Sentry Logger [debug]: @opentelemetry/instrumentation-http Applying instrumentation patch for nodejs core module on require hook { module: 'https' }
Sentry Logger [debug]: @opentelemetry/instrumentation-fastify Applying instrumentation patch for module on require hook {
module: 'fastify',
version: '4.28.1',
baseDir: '/Users/[...]/Code/sentry-openai/node_modules/.pnpm/fastify@4.28.1/node_modules/fastify'
}
Sentry Logger [debug]: @opentelemetry/instrumentation-fastify Patching fastify preHandler function
Sentry Logger [debug]: @opentelemetry/instrumentation-fastify Patching fastify server.addHook function
Sentry Logger [debug]: @opentelemetry/instrumentation-fastify Patching fastify route.handler function
Sentry Logger [debug]: @opentelemetry/instrumentation-fastify Patching fastify route.handler function
Server listening at http://0.0.0.0:3000 It seems that node profiling might force the sdk to run in commonjs mode instead of esm? |
@Xhale1 are you setting profilesSampleRate as well, or is that not defined? I can see the SDK enables continuous profiling mode, which should only be enabled if profilesSampleRate and profilesSampler are not set |
I was not, but I tried adding Also, I edited my previous reply with additional logs for with-error and without-error |
Let me look at the repo. If you setProfilesSampleRate, then the continuous profiling log should not appear, if it does, then that is an SDK bug. I'm going to investigate |
Ok, so if I set profilesSampleRate to > 0 and curl the endpoint (curl localhost:3000), I can see that the profiler is started and stopped.
Server logs
Did you try sending a request to the server to see if the profiler is started and stopped? What is happening here in my setup is that the profile is being filtered out by the client (null return log) and not sent to the server, as we require a minimum of 2 samples at ~100hz sampling rate (aka the handler should take 20ms+) |
I have not tried to verify if profiling or tracing are working as expected or not Are you saying that the |
Hm, I don't think it is properly instrumented if you don't see |
@Xhale1 That seems to be the case yes. I dont know exactly what the check relies on, maybe its, because there are no routes defined in the file? |
I should clarify, I have not confirmed myself whether the warning message is accurate or not. @JonasBa I experienced this warning message in our internal codebase at trainwell which has a number of routes defined so I don't think that's the case. I did not define routes in my repo to keep things as minimal as possible. As far as I'm aware my repo follows the Sentry docs for getting started with Fastify closely with the biggest difference being the use of |
Hey everyone, I've made a change to scope require creation to the binding import here so that we dont end up tripping cjs detection. |
Thank you! That's a welcomed fix, should hopefully clear up some confusion.
Truthfully we're not moving forward with sentry node profiling due to a lack of confidence in it. Seeing warnings being reported after setting up a minimal repo that (I think?) follows the getting started documentation makes me worried about how thoroughly it's been tested. To be clear, I appreciate yours and your team's dedication, effort, and time towards sentry v8 and helping to triage issues in the open! |
Sounds good, I appreciate the honesty :) Unfortunately, I simply just wasn't aware of the CJS check in our node package (it makes no difference for profiling), we only test that the package runs fine in both environments. |
Do you happen to know if the improved CJS check logic will fix the My apologies if that was already addressed, it's totally possible I missed it. |
@Xhale1 Yes, it will. Right now what is happening is that your code still runs in ESM, however since profiling creates a require function and stores it under global scope, that check ends up returning true and generating a false positive message. I tested on a local fastify server, and even though Sentry was saying my code is not instrumented, after I fired a few requests, I could see the data was coming into Sentry, confirming that it was a false positive |
This issue has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you remove the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
What is the verdict on this? Is it fixed on some version and up? Is there a workaround? Do we just ignore the warning? |
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.30.0
Framework Version
nodejs 20.17.0
Link to Sentry event
No response
Reproduction Example/SDK Setup
I made a repo here to demo the issue: https://github.com/Xhale1/sentry-repro
Steps to Reproduce
integrations: [nodeProfilingIntegration()]
andtracesSampleRate: 1
are set. Removing one or the other seems to make the error go away.Expected Result
Fastify is instrumented properly.
Actual Result
Sentry prints the following to the console:
The text was updated successfully, but these errors were encountered: