inngest / inngest-js

The developer platform for easily building reliable workflows with zero infrastructure for TypeScript & JavaScript
https://www.inngest.com/
GNU General Public License v3.0
414 stars 41 forks source link

Sentry middleware example improvements #471

Closed jpwilliams closed 2 months ago

jpwilliams commented 8 months ago

Summary

The Sentry middleware example on the docs has a few issues:

A helpful user has spent time on this and recommended we use scopes, like so:

    return {
      onFunctionRun({ ctx, fn }) {
        return Sentry.withScope((scope) => {
          // Add specific context for the given function run
          scope.setTags({
            "inngest.client.id": client.id,
            "inngest.function.id": fn.id(client.id),
            "inngest.function.name": fn.name,
            "inngest.event": ctx.event.name,
            "inngest.run.id": ctx.runId,
          });

          // Start a transaction for this run
          const transaction = Sentry.startTransaction({
.....

This, along with then changing some code to alter transactions and their child spans (which use APIs which are also now deprecated), we should be able to smarten up the example.

Ideally, this results in an @inngest/middleware-sentry package. It's a very popular middleware.

Tasks

fnimick commented 8 months ago

re: "and initializing it again can cause issues" - in particular if you initialize it using a framework-specific sentry package, then initialize here using @sentry/node, it breaks the path transformation used for sourcemaps.

Related: https://github.com/getsentry/sentry-javascript/issues/10089

fnimick commented 8 months ago

Also, my Sentry scope example doesn't actually work, because the scope is popped off the Sentry scope stack as soon as the function returns - it's not actually present at the time that e.g. the returned inner function transformOutput is called.

Ideally the Sentry scope needs to wrap the actual call made at the Inngest library level from end to end, and then individual callbacks from the middleware can use the Sentry import and have the scope automatically apply.

I have a middleware solution using pushScope and popScope, but ideally this would be a native wrapper in the inngest client library using withScope rather than those deprecated functions.

export const sentryMiddleware = new InngestMiddleware({
  name: "Sentry Middleware",
  init({ client }) {
    return {
      onFunctionRun({ ctx, fn }) {
        const scope = Sentry.getCurrentHub().pushScope();
        scope.setTags({
          "inngest.client.id": client.id,
          "inngest.function.id": fn.id(client.id),
          "inngest.function.name": fn.name,
          "inngest.event": ctx.event.name,
          "inngest.run.id": ctx.runId,
        });

        // Start a transaction for this run
        const transaction = Sentry.startTransaction({
          name: "Inngest Function Run",
          op: "run",
          data: ctx.event,
        });

        let memoSpan: Sentry.Span;
        let execSpan: Sentry.Span;

        return {
          beforeMemoization() {
            // Track different spans for memoization and execution
            memoSpan = transaction.startChild({ op: "memoization" });
          },
          afterMemoization() {
            memoSpan.finish();
          },
          beforeExecution() {
            execSpan = transaction.startChild({ op: "execution" });
          },
          afterExecution() {
            execSpan.finish();
          },
          transformOutput({ result, step }) {
            // Capture step output and log errors
            if (result.error) {
              Sentry.withScope((nestedScope) => {
                if (step) {
                  nestedScope.setTags({
                    "inngest.step.name": step.displayName,
                    "inngest.step.op": step.op,
                  });
                }
                Sentry.captureException(result.error, nestedScope);
              });
            }
          },
          async beforeResponse() {
            // Finish the transaction and flush data to Sentry before the
            // request closes
            transaction.finish();
            Sentry.getCurrentHub().popScope();
            await Sentry.flush();
          },
        };
      },
    };
  },
});
fnimick commented 8 months ago

(for anyone who used the above, my transformOutput hook was only capturing errors that occurred in steps, and not those that occurred at the top level of the function. this is now fixed.)

aon commented 7 months ago

Hey everyone. I made some improvements to log more data and remove the deprecatedstartTransaction. Although we still cannot avoid using the pushScope This is the middleware I'm currently using:

import * as Sentry from "@sentry/nextjs"
import { type Span } from "@sentry/nextjs/types/server"
import { type Primitive, type SpanAttributes } from "@sentry/types"
import { type EventPayload, InngestMiddleware } from "inngest"

export const sentryMiddleware = new InngestMiddleware({
  name: "Sentry Middleware",
  init({ client }) {
    return {
      onFunctionRun({ ctx, fn }) {
        const scope = Sentry.getCurrentHub().pushScope()
        const event = parseInngestEventIntoAttributes(ctx.event)

        scope.setTags({
          "inngest.client.id": client.id,
          "inngest.function.id": fn.id(client.id),
          "inngest.function.name": fn.name,
          "inngest.run.id": ctx.runId,
          ...flattenEventIntoPrimitive(event, "inngest.event"),
        })

        // Start a span for this function run
        return Sentry.startSpan(
          {
            name: "Inngest Function Run",
            op: "run",
          },
          () => {
            let memoSpan: Span | undefined
            let execSpan: Span | undefined

            return {
              beforeMemoization() {
                memoSpan = Sentry.startInactiveSpan({
                  name: "Inngest Memoization",
                  op: "memoization",
                })
              },
              afterMemoization() {
                memoSpan?.end()
              },
              beforeExecution() {
                execSpan = Sentry.startInactiveSpan({
                  name: "Inngest Execution",
                  op: "execution",
                })
              },
              afterExecution() {
                execSpan?.end()
              },
              transformOutput({ result, step }) {
                if (Boolean(result.error)) {
                  Sentry.withScope((nestedScope) => {
                    if (step) {
                      nestedScope.setTags({
                        "inngest.step.name": step.name,
                        "inngest.step.op": step.op,
                      })
                    }
                    Sentry.captureException(result.error)
                  })
                }
              },
              async beforeResponse() {
                Sentry.getCurrentHub().popScope()
                await Sentry.flush()
              },
            }
          },
        )
      },
    }
  },
})

function parseInngestEventIntoAttributes(event: EventPayload) {
  const stringifyWithoutFail = (data: unknown) => {
    try {
      return JSON.stringify(data)
    } catch {
      return "Failed to stringify"
    }
  }

  const attributes = {
    name: event.name,
    data: stringifyWithoutFail(event.data),
    ts: event.ts,
    user: stringifyWithoutFail(event.user),
    v: event.v,
  } satisfies SpanAttributes

  return attributes
}

function flattenEventIntoPrimitive(
  event: ReturnType<typeof parseInngestEventIntoAttributes>,
  prefix: string,
) {
  const result: Record<string, Primitive> = {}

  for (const [key, value] of Object.entries(event)) {
    result[`${prefix}.${key}`] = value
  }

  return result
}
mmachatschek commented 4 months ago

@jpwilliams can you verify the above example by @aon and update the middleware example in the docs? sentry-js v8 has been released two weeks ago and it would be nice to have an official example on the website

brijmcq commented 4 months ago

@aon what version of sentry are you using? I tried your code and getting this error and warning:

aon commented 4 months ago

Hey, I haven't touched much the code since I posted it here but it's still running in a production app. This is the version I'm using:

"@sentry/nextjs": "^7.103.0"

Both getCurrentHub and pushScope are deprecated, but it was the only solution at the time to implement the middleware. Let me know if it works!

jpwilliams commented 4 months ago

Hi folks!

Appreciate the conversation here. It's not a great experience for anyone having to scrape the solution from our docs and, as we've seen, it inevitably goes out of date.

I've added a PR for a new @inngest/middleware-sentry package over at #598. It's on npm but I'd love to get some comments here and/or on that PR to make sure it works for all of you before we ship it proper.

Installation and usage from that PR looks like:

npm install @inngest/middleware-sentry@pr-598
import * as Sentry from "@sentry/node";
import { Inngest } from "inngest";
import { sentryMiddleware } from "@inngest/middleware-sentry";

// Initialize Sentry as usual wherever is appropriate
Sentry.init(...);

const inngest = new Inngest({
  id: "my-app",
  middleware: [sentryMiddleware()],
});

Right now this is v8+ due to the usage of the new APIs.

Options are also detailed in that PR. @aon's parseInngestEventIntoAttributes() functions are interesting, too, and may be reasonable to put in at least behind an option.

Please let me know if this works and we'll get that package shipped. 🙂 Leaving this issue open for discussion and feedback until that's done.

brijmcq commented 3 months ago

@jpwilliams works on my end :) Thank you for responding really quick!

jpwilliams commented 2 months ago

As there has been no further feedback since the release (#638) of @inngest/middleware-sentry (#598), I'm closing this.

Please open another issue referencing that new package for any further requests!