Update Sentry usage in `@strapi/plugin-sentry` #17435

mydea · 2023-07-25T09:05:14Z

This updates the used Sentry version to the current latest (7.60.0), and also updates the middlewares based on https://docs.sentry.io/platforms/node/guides/koa/.

Update @sentry/node to ^7.60.0 - I think it makes sense to allow the caret version here, as otherwise you do not get bugfixes etc. If you prefer to hard-pin this to 7.60.0, let me know, then I can also do that.
Add new option tracesSampleRate which defaults to 0.1. Based on this we can also instrument performance monitoring in Sentry for the user.
Update middlewares based on current docs.

Fixes #17425

strapi-cla · 2023-07-25T09:05:18Z

All committers have signed the CLA.

lforst · 2023-07-25T11:19:11Z

packages/plugins/sentry/server/middlewares/sentry-performance.js

+  }
+
+  strapi.server.use(async (ctx, next) => {
+    return new Promise((resolve, reject) => {


Why are we instantiating a promise here?

good point, will just remove these wrapping promises!

lforst · 2023-07-25T11:20:02Z

packages/plugins/sentry/server/middlewares/sentry-performance.js

+        try {
+          await next();
+        } catch (err) {
+          reject(err);
+        }
+        resolve();


This is unnecessary if we just remove the surrounding promise.

good point, removed this (and will also update the docs accordingly 😅 )

lforst · 2023-07-25T11:20:35Z

packages/plugins/sentry/server/middlewares/sentry-performance.js

+          scope.addEventProcessor((event) =>
+            Sentry.addRequestDataToEvent(event, ctx.request, {
+              include: {
+                user: false,


Are there any other default values we should override for pii reasons?

The other things here here are ip which is off by default, request and transaction which should both be fine I think.

lforst · 2023-07-25T11:22:27Z

packages/generators/app/src/utils/usage.ts

          category: 'stderr',
          message: line,
-          level: Severity.Error,
+          level: Sentry.Severity.Error,


We prefer string literals here instead of the enum: https://github.com/getsentry/sentry-javascript/blob/df7bb254d3f7b1c456a5949efcc123606838aa77/MIGRATION.md#L424-L428

AbhiPrasad · 2023-07-25T13:00:46Z

packages/plugins/sentry/server/middlewares/sentry-performance.js

+    // connect to trace of upstream app
+    let traceparentData;
+    if (ctx.request.get('sentry-trace')) {
+      traceparentData = Sentry.extractTraceparentData(ctx.request.get('sentry-trace'));


we also want to extract baggage here, and also use distributed tracing features (don't gate by hasTracingEnabled). See express as an example:

https://github.com/getsentry/sentry-javascript/blob/53d32a3ac7d2238616ac6dc9c210e3d1d0584bbb/packages/node/src/handlers.ts#L56-L69

I updated this again, can you have another look?

packages/plugins/sentry/server/middlewares/sentry-performance.js

AbhiPrasad

LGTM!

remidej

Thanks for the PR!

A concern I have is that bumping the major version of Sentry means introducing a breaking change. It's only a small one and can be corrected though, see my comment there

And I think we tend to prefer fixed version dependencies, to avoid potential bugs in future releases or packages not respecting semver. The dependabot github app makes it easy to upgrade our dependencies, but with a small manual check for extra safety

mydea · 2023-09-13T09:34:44Z

Thanks for the PR!

A concern I have is that bumping the major version of Sentry means introducing a breaking change. It's only a small one and can be corrected though, see my comment there

And I think we tend to prefer fixed version dependencies, to avoid potential bugs in future releases or packages not respecting semver. The dependabot github app makes it easy to upgrade our dependencies, but with a small manual check for extra safety

OK, I pinned the deps to an exact version, and also added some code to migrate the old allow/deny URL options! 🚀

This updates the used Sentry version to the current latest (7.60.0), and also updates the middlewares based on https://docs.sentry.io/platforms/node/guides/koa/.

Adaphath · 2023-10-04T08:04:57Z

When can we expect to get the new Sentry SDK version released?

davidkassa · 2023-12-27T21:37:54Z

It appears that the changes requested are completed, but now we're up to 7.91.0 :)

davidkassa · 2023-12-30T18:34:49Z

This PR, as-is, doesn't work. I was receiving the following error until I changed the strapi.server.use methods to async with await next(). I haven't finished testing, but that solved the closed stream issue.

[2023-12-30 12:09:51.388] http: POST /graphql (12 ms) 404
This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). The promise rejected with the reason:
InternalServerError: stream is not readable
    at readStream (/Users/dave/Code/strapi/node_modules/raw-body/index.js:185:17)
    at executor (/Users/dave/Code/strapi/node_modules/raw-body/index.js:120:5)
    at new Promise (<anonymous>)
    at getRawBody (/Users/dave/Code/strapi/node_modules/raw-body/index.js:119:10)
    at module.exports [as json] (/Users/dave/Code/strapi/node_modules/koa-bodyparser/node_modules/co-body/lib/json.js:39:21)
    at parseBody (/Users/dave/Code/strapi/node_modules/koa-bodyparser/index.js:87:26)
    at bodyParser (/Users/dave/Code/strapi/node_modules/koa-bodyparser/index.js:67:25)
    at /Users/dave/Code/strapi/node_modules/apollo-server-koa/dist/ApolloServer.js:15:16
    at dispatch (/Users/dave/Code/strapi/node_modules/koa-compose/index.js:42:32)
    at /Users/dave/Code/strapi/node_modules/apollo-server-koa/dist/ApolloServer.js:18:16

lforst reviewed Jul 25, 2023

View reviewed changes

AbhiPrasad reviewed Jul 25, 2023

View reviewed changes

packages/plugins/sentry/server/middlewares/sentry-performance.js Show resolved Hide resolved

AbhiPrasad reviewed Jul 26, 2023

View reviewed changes

Marc-Roig requested a review from remidej August 7, 2023 09:57

Marc-Roig assigned remidej Aug 7, 2023

remidej requested changes Sep 13, 2023

View reviewed changes

mydea force-pushed the fn/sentry branch from 27e79e3 to ac1ead0 Compare September 13, 2023 09:34

mydea added 5 commits September 13, 2023 11:36

Update Sentry usage in @strapi/plugin-sentry

08352a3

This updates the used Sentry version to the current latest (7.60.0), and also updates the middlewares based on https://docs.sentry.io/platforms/node/guides/koa/.

PR feedback & format

1dfb848

Update performance middleware

3756806

handle changed config options

9244969

pin to 7.69.0

d2039ed

mydea force-pushed the fn/sentry branch from ac1ead0 to d2039ed Compare September 13, 2023 09:38

hanpaine added community source: plugin:sentry Source is plugin/sentry package pr: chore This PR contains chore tasks (cleanups, configs, tooling...) labels Apr 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Sentry usage in `@strapi/plugin-sentry` #17435

Update Sentry usage in `@strapi/plugin-sentry` #17435

mydea commented Jul 25, 2023

strapi-cla commented Jul 25, 2023 •

edited

lforst Jul 25, 2023

mydea Jul 25, 2023

lforst Jul 25, 2023

mydea Jul 25, 2023

lforst Jul 25, 2023 •

edited

mydea Jul 25, 2023

lforst Jul 25, 2023

AbhiPrasad Jul 25, 2023

mydea Jul 25, 2023

AbhiPrasad left a comment

remidej left a comment

mydea commented Sep 13, 2023

Adaphath commented Oct 4, 2023

davidkassa commented Dec 27, 2023

davidkassa commented Dec 30, 2023

Update Sentry usage in @strapi/plugin-sentry #17435

Are you sure you want to change the base?

Update Sentry usage in @strapi/plugin-sentry #17435

Conversation

mydea commented Jul 25, 2023

strapi-cla commented Jul 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lforst Jul 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbhiPrasad left a comment

Choose a reason for hiding this comment

remidej left a comment

Choose a reason for hiding this comment

mydea commented Sep 13, 2023

Adaphath commented Oct 4, 2023

davidkassa commented Dec 27, 2023

davidkassa commented Dec 30, 2023

Update Sentry usage in `@strapi/plugin-sentry` #17435

Update Sentry usage in `@strapi/plugin-sentry` #17435

strapi-cla commented Jul 25, 2023 •

edited

lforst Jul 25, 2023 •

edited