immobiliare / fastify-sentry Goto Github PK

Sentry is using some new api internally in its integrations, let's update ours to reflect that.

(node:15939) [FSTDEP012] FastifyDeprecation: Request#context property access is deprecated. Please use "Request#routeConfig" or "Request#routeSchema" instead for accessing Route settings. The "Request#context" will be removed in fastify@5.

probably from line in utils.js:88

can I send PR?

We should change the default setting of Sentry integration to include the uncaughtException handler.

Use tap as a test runner.

I am not clear whether fastify-sentry is needed if I am already using @sentry/opentelemetry, and my goal is to just capture performance traces. As far as I understand, @opentelemetry/auto-instrumentations-node already handles instrumentation for us.

It looks like some types are not resolved on the fastify.Sentry instance as they would when using the Sentry instance.

Hello guys, just wanted to let you know about an issue with typescript types for this module, when using moduleResolution: Node16 it causes some errors when trying to import the default exported fastify plugin callback, you can check this out here https://arethetypeswrong.github.io/?p=%40immobiliarelabs%2Ffastify-sentry%406.0.10

Go into Workflow jobs and remove ignore pattern to activate it

Looking at how Sentry is integrated in Express (https://github.com/getsentry/sentry-javascript/blob/master/packages/node/src/handlers.ts) I think we can pick up a thing or two and apply them here, for example:

• use a function (shouldHandlerError in the Express integration) to customize the event filtering instead of our array of statusCodes
• improve how we enrich the event data

Implementing these changes would also resolve issues #5 and #3

My app uses JWT to authenticate users, which only contain a user ID, and not a user name.

The current requirement for getting extractUserData to work (based on the types at least) is to always provide a username though:

Is there a way to make them all optional? Not all my requests have an authenticated user (some are public routes) so being able to return 'nothing' would also come in quite handy.

Just an idea I had as I'm working through adding this to a project.

I am using a fastify server inside of AWS Lambda and thus already have a Sentry instance initialized. It would be nice to have an option to simply skip the Sentry initalization step to not confuse the library.

Currently all Sentry deps of fastify-sentry have strict versions:

 "dependencies": {
    "@sentry/node": "7.59.2",
    "@sentry/tracing": "7.59.2",
    "@sentry/utils": "7.59.2",
  },

It means that if I need to install @sentry/node to my project separatly to use some functions from it, I have to sync versions. If I use versions with caret (^7.60.0), yarn installs all sentry deps in 2 places:

• project/node_modules — my project
• project/node_modules/@sentry/node/node_modules/ — that's a problem

That causes type errors:

If we look to yarn.lock file, we could find that all Sentry version have duplicates with no caret:

So, my suggestion is to prefix all versions with caret:

 "dependencies": {
    "@sentry/node": "^7.59.2",
    "@sentry/tracing": "^7.59.2",
    "@sentry/utils": "^7.59.2",
  },

Specifying strict resolution in package.json is a workaround

Issue

Hello. We started using your package to integrate Sentry into our Fastify applications. It worked really great until we integrated Sentry into another application which calls the first one.

I found that this is related to baggage, hence why the issue only appears when Sentry is enabled in the caller app.
I then looked at Sentry's SDK code and found that the parseBaggageString() function has been renamed to parseBaggageHeader() in version 7.3.0.

Versions

• node: 16.18.1
• fastify: 4.6.0
• @immobiliarelabs/fastify-sentry: 5.0.0
• @sentry/*: 7.18.0

We're doing some hacky stuff to make Fastify work inside the main electron process. Somehow its hitting this line every time inside the electron process.

When the Fastify server runs directly with node, it works as normal.

I'm thinking this is because this project uses direct imports of @sentry/node and in our case we might need to use @sentry/electron. Is there a way to change it or could you guys give us some help?

  fastify.register(fastifySentry, {
    shouldHandleError(error, request, reply) {
      if (error.name == 'NotFoundError') { 
        return false
      }
      return true
    }
)

I want to filter these logs in the console as well:

The error handler of the plugin uses a property on the error object to decide if it should redact the message. I think it's nice to check for the same property and add support for the plugin errors:

https://github.com/fastify/fastify-sensible/blob/48ef8a00ef7b9396a538711b58e7d09b410e1025/index.js#L47

Just wanted to pop in and say thank you for making and maintaining this library :)

I see a bunch of recent commits to update deps which is quite exciting, particularly domain changes relating to Sentry 7.48!

Amazing job to everyone involved!

Is there a reason why Sentry versions are now pinned instead of using a ^ range? This seems to have changed in v5.0.1, just curious if that's actually needed.

The documentation cites fastify as a parameter but it has been removed.

hey, i see that i need to import sentry transactions to be imported first then profiling import. how can i add profiling using the library?

I didn't want to use Sentry hooks to skip the event reporting for some status codes. I think we should benchmark the difference and consider using the default Sentry hooks if the drop isn't big. I think it's better to have a single point to handle the skipping, it's less confusing.

Test also the initialization options. Now we are just registering the plugin with the default options.

Attaach a hook to the onTimeout hook of Fastify.

getsentry/sentry-javascript#6066

Pinning ti 7.16 temporarily to avoid breaking people npm install

The log at https://github.com/immobiliare/fastify-sentry/blob/next/index.js#L70 implies that Sentry configuration should be skipped when DSN is not provided. But it is clearly initialized a few lines above.

My Context

I get DSN from an environment variable, which is available only in prod. During local development the DSN is empty. Because fastify-sentry still initializes the Sentry, when an exception occurs, a long Sentry Logger [error]: SentryError: SDK not enabled, will not capture event. error is logged to the console, which is unwanted for me as it clutters the console.

I am aware I could wrap the plugin registration into if (env !== 'development'), but then the fastify.Sentry is undefined, causing calls like fastify.Sentry.addBreadcrumb to fail and the TS does not warn about it, because the plugin provides the following type:

declare module 'fastify' {
    interface FastifyInstance {
        Sentry: typeof Sentry;
    }
}

I see two solutions:

1. Overwrite the 'fastify' module type with

declare module 'fastify' {
    interface FastifyInstance {
        Sentry?: typeof Sentry;
    }
}

and then write fastify.Sentry?.addBreadcrumb every time (with ?).

2. Do not call Sentry.init() in the plugin unless DSN is provided.

I am not aware on how to do the first without changing the code of this plugin, otherwise I would have done it.
The second solution also seems reasonable to me and I think it is a bug that it is initialized even though the log says it won't be. Therefore I am opening this issue.

I couldn't make autoSessionTracking work, opening this to remember to add support for it.

I have a route with a Knex instance throwing SQL errors. I use an error handler in my app.ts file to catch these and set a generic error message.

fastify.setErrorHandler((error, request, reply) => {

    if ((process.env.NODE_ENV || 'development') === 'production') {
      reply.status(500).send({ message: "Internal Server Error"});`

However, these thrown errors are coming back with a 2xx status code and a response body
{"error":500,"message":"Internal Server Error"}

fastify.withTypeProvider<ZodTypeProvider>().route({
    method: "DELETE",
    url: "/ingredients/:id",
    onRequest: [fastify.authenticate, fastify.isAdmin],
    schema: {
      params: z.object({
        id: z.string()
      }),
    },
    handler: async (request, reply) => {
      const ingredientId = Number(request.params.id);
      await fastify.knex('ingredient')
        .where('id', ingredientId)
        .del();
  
      reply.status(204).send();
    },
  });

This is what my route handler looks like.

And this is my Sentry plugin init

import fp from 'fastify-plugin';

export default fp(async (fastify, opts) => {
  if (process.env.NODE_ENV === 'production') {
    fastify.register(require('fastify-sentry'), {
      dsn: process.env.SENTRY_DSN,
      environment: process.env.NODE_ENV,
      release: '1.0.0',
    });
  }
});

We are linting only during local development.It might be useful to do that in a CI job too.

This library has been great, but it's currently making it difficult to upgrade our Sentry library versions.

It seems like the problem has to do with a change to the definition of Sentry's NodeOptions type.

Here's the typescript error output:

src/server.ts:41:3 - error TS2769: No overload matches this call.
  Overload 1 of 3, '(plugin: FastifyPluginCallback<FastifySentryOptions, RawServerDefault, FastifyTypeProviderDefault>, opts?: FastifyRegisterOptions<...> | undefined): FastifyInstance<...> & PromiseLike<...>', gave the following error.
    Argument of type 'NodeOptions' is not assignable to parameter of type 'FastifyRegisterOptions<FastifySentryOptions> | undefined'.
      Type 'NodeOptions' is not assignable to type 'RegisterOptions & FastifySentryOptions'.
        Type 'NodeOptions' is not assignable to type 'FastifySentryOptions'.
          Types of property 'defaultIntegrations' are incompatible.
            Type 'false | import("/Users/james/code/arcol/node_modules/@sentry/types/types/integration").Integration[] | undefined' is not assignable to type 'false | import("/Users/james/code/arcol/node_modules/@immobiliarelabs/fastify-sentry/node_modules/@sentry/types/types/integration").Integration[] | undefined'.
              Type 'Integration[]' is not assignable to type 'false | Integration[] | undefined'.
                Type 'import("/Users/james/code/arcol/node_modules/@sentry/types/types/integration").Integration[]' is not assignable to type 'import("/Users/james/code/arcol/node_modules/@immobiliarelabs/fastify-sentry/node_modules/@sentry/types/types/integration").Integration[]'.
                  Type 'import("/Users/james/code/arcol/node_modules/@sentry/types/types/integration").Integration' is not assignable to type 'import("/Users/james/code/arcol/node_modules/@immobiliarelabs/fastify-sentry/node_modules/@sentry/types/types/integration").Integration'.
                    Types of property 'setupOnce' are incompatible.
                      Type '(addGlobalEventProcessor: (callback: import("/Users/james/code/arcol/node_modules/@sentry/types/types/eventprocessor").EventProcessor) => void, getCurrentHub: () => import("/Users/james/code/arcol/node_modules/@sentry/types/types/hub").Hub) => void' is not assignable to type '(addGlobalEventProcessor: (callback: import("/Users/james/code/arcol/node_modules/@immobiliarelabs/fastify-sentry/node_modules/@sentry/types/types/eventprocessor").EventProcessor) => void, getCurrentHub: () => import("/Users/james/code/arcol/node_modules/@immobiliarelabs/fastify-sentry/node_modules/@sentry/types/typ...'.
                        Types of parameters 'addGlobalEventProcessor' and 'addGlobalEventProcessor' are incompatible.
                          Types of parameters 'callback' and 'callback' are incompatible.
                            Type 'import("/Users/james/code/arcol/node_modules/@sentry/types/types/eventprocessor").EventProcessor' is not assignable to type 'import("/Users/james/code/arcol/node_modules/@immobiliarelabs/fastify-sentry/node_modules/@sentry/types/types/eventprocessor").EventProcessor'.
                              Types of parameters 'event' and 'event' are incompatible.
                                Type 'import("/Users/james/code/arcol/node_modules/@immobiliarelabs/fastify-sentry/node_modules/@sentry/types/types/event").Event' is not assignable to type 'import("/Users/james/code/arcol/node_modules/@sentry/types/types/event").Event'.
                                  Types of property 'spans' are incompatible.
                                    Type 'import("/Users/james/code/arcol/node_modules/@immobiliarelabs/fastify-sentry/node_modules/@sentry/types/types/span").Span[] | undefined' is not assignable to type 'import("/Users/james/code/arcol/node_modules/@sentry/types/types/span").Span[] | undefined'.
                                      Type 'import("/Users/james/code/arcol/node_modules/@immobiliarelabs/fastify-sentry/node_modules/@sentry/types/types/span").Span[]' is not assignable to type 'import("/Users/james/code/arcol/node_modules/@sentry/types/types/span").Span[]'.
                                        Type 'Span' is missing the following properties from type 'Span': attributes, spanContext, end, setAttribute, and 3 more.
  Overload 2 of 3, '(plugin: FastifyPluginAsync<FastifySentryOptions, RawServerDefault, FastifyTypeProviderDefault>, opts?: FastifyRegisterOptions<...> | undefined): FastifyInstance<...> & PromiseLike<...>', gave the following error.
    Argument of type 'FastifySentryPlugin' is not assignable to parameter of type 'FastifyPluginAsync<FastifySentryOptions, RawServerDefault, FastifyTypeProviderDefault>'.
  Overload 3 of 3, '(plugin: FastifyPluginCallback<FastifySentryOptions, RawServerDefault, FastifyTypeProviderDefault> | FastifyPluginAsync<...> | Promise<...> | Promise<...>, opts?: FastifyRegisterOptions<...> | undefined): FastifyInstance<...> & PromiseLike<...>', gave the following error.
    Argument of type 'NodeOptions' is not assignable to parameter of type 'FastifyRegisterOptions<FastifySentryOptions> | undefined'.

41   server.register(FastifySentry, sentryOptions);
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I haven't bisected @sentry/node to figure out when exactly the breaking change was introduced, but it appears to have happened sometime between v7.75 and v7.105, the current version as of this issue's creation.

Let me know if I can provide more information that would be helpful here.

This filed has been deprecated, we should use setContext instead of setExtra.

I think it would be nice to expose an option to pass a custom function that can be used to enrich the event with custom data.

Hi there, thanks for building this useful integration!

I'm using Fastify with TRPC, which means my routes all match to a single request handler in fastify under something like /trpc/:rpcCall. To break these routes out by their TRPC function call, I added a getTransactionName:

        getTransactionName: (request: FastifyRequest) => {
            if (request.routerPath === '/trpc/:path') {
                return (request.params as any)['path'];
            }

            return request.routerPath;
        },

This works well, but because the source is type URL, Sentry treats these routes as a high cardinality value and groups them in this <<unparameterized>> block

Happy to submit a fix, but wanted to get your thoughts on a few approaches:

1. Switch the current source value from url to route (here)
It seems to me like fastify's routerPath value should be treated as a route source in sentry and not a url source, since it returns urls like /users/:id and not /users/1. Relevant docs

2. Switch the current value of url to custom if the user provides a getTransactionName function
(self explanatory)
3. Allow users to provide a custom getTransactionSource
Since the docs discourage users from doing this and say source should only be specified by integrations, I'd probably suggest one of the former two.

Hi, thank you for your work, I love your integration.

I used this plugin in my app that is bundled into single file during build, but i stumbled on an obstacle.
During runtime, there si require call for package.json via relative path, which cannot be bundled at current state.

Is there a possibility to change this behavior to support code bundling?

I've tested one sollution. Changing

const PACKAGE_NAME = require(path.resolve(__dirname, 'package.json')).name;

to

const PACKAGE_NAME = require('./package.json').name;

would help :-)

Thanks!

Avoid having a big array with all the status code and see if we can do some filtering based on strings, eg: 5xx

We can re-use the current scope of the request using:

Sentry.getCurrentHub().getScope()

instead of creating a new one with .withScope()

When a DSN is not passed, the plugin logs a message which implies that the init function of the Sentry SDK will not be called and that's not true.

Ref #93 .

Hi guys, is there any plan to update deps to support fastify v4 any time soon?
Would be great to test it.
Have a great day!

Hey, I'm not sure what would be the best approach for this but I don't think the current status quo is correct either.

Let's say there's a route

  fastify.get('/someroute', {
    schema: {
      querystring: {
        type: 'object',
        properties: {
          dob: { type: 'string', format: 'date' },
        },
        required: ['dob']
      }
    }
  }, (req, res) => {
    return res.send('foo')
  })

Now, if with fastify-sentry I try to call this route with an invalid date of birth, I'm getting {"statusCode":500,"error":"Internal Server Error","message":"Something went wrong"}. Without fastify-sentry I get the more sensible {"statusCode":400,"error":"Bad Request","message":"querystring/dob must match format \"date\""}

IMO these format errors should be fully excluded from fastify-sentry and returned to the client as is.

Seems like one approach (described here https://www.fastify.io/docs/latest/Reference/Validation-and-Serialization/#validation-messages-with-other-validation-libraries) would be to check if validation property exists on the error, and in those cases let it pass through

This looks like a great wrapper for Sentry + Fastify. I'd love to use it in my project.

I don't know if I'm missing an important step in the instructions or something, but I cannot get this to work at all:

import fastifySentry from '@immobiliarelabs/fastify-sentry'
await fastify.register(fastifySentry, {
  dsn: process.env.SENTRY_DSN,
  debug: true,
  environment: process.env.SERVER_ENV,
  tracesSampleRate: 1.0,
  release: '1.0.0',
})
fastify.route({
  method: 'GET',
  url: '/test',
  handler: async (request, reply) => {
    throw new Error('test1234')
  },
})

I have the following issues:

1. The error is never captured in my Sentry UI. The call to captureException in base.js is firing, but nothing is showing up in Sentry.
2. Calling fastify.Sentry.captureMessage() doesn't work either
3. Enabling tracing with tracesSampleRate: 1.0 does not work because if (hasTracingEnabled()) { in request.js evaluates to false.

I've ensured my SENTRY_DSN is correct, as it works when I use the Sentry Node SDK, and double checked your examples and I don't think I'm missing anything.

Any ideas would be appreciated!

In its latest versions Sentry has added tracing performance capabilities, it would be nice to be able to support that, using an option to enable/disable the functionality or maybe using a separate plugin 🤔