Skip to content
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

deps(sentry): upgrade Sentry to version 8 #17505

Closed
wants to merge 2 commits into from
Closed

Conversation

chenba
Copy link
Contributor

@chenba chenba commented Aug 29, 2024

Because:

  • Sentry 8 is the latest version

This commit:

  • deletes some integration code since Sentry does it automatically
  • updates code to use the Sentry 8 API
  • updates a few proxyquire calls because they were cause module not
    found errors in CI

@chenba chenba force-pushed the fxa-10192-sentry-upgrade branch 2 times, most recently from df55980 to 73793ec Compare September 6, 2024 19:24
@@ -23,7 +24,8 @@ const CODE_WITHOUT_KEYS = 'f0f0f0';
const mockDb = { touchSessionToken: sinon.stub() };
const mockStatsD = { increment: sinon.stub() };
const mockGlean = { oauth: { tokenCreated: sinon.stub() } };
const tokenRoutes = proxyquire('../../../lib/routes/oauth/token', {
const tokenRoutePath = path.join(__dirname, '../../../lib/routes/oauth/token');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this causing the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the fix.

@chenba chenba force-pushed the fxa-10192-sentry-upgrade branch 4 times, most recently from d58c600 to 6c030f1 Compare September 16, 2024 13:30
@chenba chenba changed the title WIP Sentry v8 deps(sentry): upgrade Sentry to version 8 Sep 16, 2024
@chenba
Copy link
Contributor Author

chenba commented Sep 16, 2024

I'm putting this up for an initial review. But at the time of writing (mid-Sept), we've exceed the tracing quota on all projects(?) but the content server, making verification difficult.

@chenba chenba marked this pull request as ready for review September 16, 2024 13:42
@chenba chenba requested a review from a team as a code owner September 16, 2024 13:42
@@ -120,6 +115,7 @@ async function configureSentry(server, config, processName = 'key_server') {
method(request, h) {
request.sentryScope = new Sentry.Scope();

/**
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commented out the code block below and didn't notice any difference. But it's entirely possible I was not looking in the right place. Maybe @dschom can let me know what I should look for. 🙇

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. That would indicate to me that the auto instrumentation is now capturing spans. I wonder if we even need this server extension at all anymore?

Sentry.browserTracingIntegration({
enableInp: true,
}),
],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most integrations are now automatic.

@dschom dschom force-pushed the fxa-10192-sentry-upgrade branch 14 times, most recently from a681f2c to d5f5fd9 Compare September 20, 2024 21:49
@chenba chenba marked this pull request as draft September 25, 2024 15:17
@chenba
Copy link
Contributor Author

chenba commented Sep 25, 2024

Setting the PR back to draft for now as we've noticed some performance issues with Sentry 8.x.

Because:
 - Sentry 8 is the latest version

This commit:
 - deletes some integration code since Sentry does it automatically
 - updates code to use the Sentry 8 API
 - updates a few proxyquire calls because they were cause module not
   found errors in CI
@dschom dschom marked this pull request as ready for review September 25, 2024 16:12
package.json Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chenba @dschom just wanted to mention the resolutions of @sentry/types. Seems like it's fine to remove that now.

Becuase:
- After upgrading sentry we were seeing test timeouts
- There were some kinda weird patterns in place for these tests anyways
- Test server wasn't getting cleaned up
- Test server start up is taking longer (presumably because of sentry upgrade?)

This Commit:
- Moves before an after blocks next to each other so setup and tear down are easy to follow
- Makes sure test server start / stop is done in before each blocks
- Increase test timeouts to 60s when test server is used
- Switches to async/await for before and after blocks, which feels less error prone
@chenba
Copy link
Contributor Author

chenba commented Sep 25, 2024

This was broken into two PRs #17697 and #17698. They've been merged so this can be closed without merging now.

@chenba chenba closed this Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants