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

Configure Sentry Replay #1232

Merged
merged 19 commits into from
Oct 24, 2022
Merged

Configure Sentry Replay #1232

merged 19 commits into from
Oct 24, 2022

Conversation

kachkaev
Copy link
Contributor

@kachkaev kachkaev commented Oct 20, 2022

🌟 What is the purpose of this PR?

This PR updates Sentry and introduces @sentry/replay. The setup has been generally aligned with blockprotocol/blockprotocol.

🔗 Related links

🚫 Blocked by

🔍 What does this change?

next.config.js contains several statements like process.env.NEXT_PUBLIC_X = process.env.X. This simplifies infrastructure setup as it makes it easier to use env variables outside Next.js. The PR follows this principle for SENTRY_REPLAYS_SAMPLING_RATE. NEXT_PUBLIC_SENTRY_DSN has been renamed to SENTRY_DSN too and new variables are created on Vercel accordingly.

📜 Does this require a change to the docs?

No

⚠️ Known issues

🐾 Next steps

Check

🛡 What tests cover this?

  • Manual

❓ How to test this?

  • Open latest Vercel deployment
  • Make sure it does not crash in any new ways

@github-actions github-actions bot added area/apps > hash* Affects HASH (a `hash-*` app) area/deps Relates to third-party dependencies (area) frontend labels Oct 20, 2022
@@ -49,6 +53,10 @@ module.exports = withSentryConfig(
eslint: { ignoreDuringBuilds: true },
typescript: { ignoreBuildErrors: true },

sentry: {
autoInstrumentServerFunctions: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -34,6 +34,10 @@ process.env.NEXT_PUBLIC_BLOCK_BASED_ENTITY_EDITOR =
// This allows the frontend to generate the graph type IDs in the browser
process.env.NEXT_PUBLIC_FRONTEND_DOMAIN = process.env.FRONTEND_DOMAIN;

process.env.NEXT_PUBLIC_SENTRY_DSN = process.env.SENTRY_DSN;
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 had to bump Next.js from 12.2 to 12.3 for this to work.

Before the bump, NEXT_PUBLIC_SENTRY_DSN was equal to "undefined" instead of undefined (likely because of a Next.js bug).

This broke Playwright: https://github.com/hashintel/hash/actions/runs/3292255307/jobs/5427399664#step:8:20

Comment on lines 22 to 23
replaysSamplingRate > 0 &&
replaysSamplingRate <= 1 &&
Copy link

Choose a reason for hiding this comment

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

It's nice to have guards on the bounds, and makes things self-documenting too.

But in practice with this integration you can pass a value that is higher or lower than 0.0 or 1.0. So it's possible to get away with as little as this if you wanted:

  environment: VERCEL_ENV,
  integrations:
    replaysSamplingRate
      ? [new Replay(...)]
      : [],
  tracesSampleRate: 1.0,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, addressed in aba0eb9 🙂

Comment on lines 12 to 15
const replaysSamplingRate = Number.parseInt(
SENTRY_REPLAYS_SAMPLING_RATE ?? "0",
10,
);
Copy link

Choose a reason for hiding this comment

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

You might want to use Number.parseFloat here instead of Number.parseInt.

The allowed range is 0.0 <= replaysSamplingRate <= 1.0. Parsing to int means you'll endup with either 0 or 1, no values in between.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Many thanks for catching this typo @ryan953! I replaced parseInt with parseFloat 👍

@lgtm-com
Copy link

lgtm-com bot commented Oct 21, 2022

This pull request introduces 1 alert when merging ff54033 into c317591 - view on LGTM.com

new alerts:

  • 1 for Useless conditional

@lgtm-com
Copy link

lgtm-com bot commented Oct 21, 2022

This pull request introduces 1 alert when merging 6b3c136 into 9852e3f - view on LGTM.com

new alerts:

  • 1 for Useless conditional

@kachkaev kachkaev marked this pull request as ready for review October 21, 2022 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/apps > hash* Affects HASH (a `hash-*` app) area/deps Relates to third-party dependencies (area) type/eng > frontend Owned by the @frontend team
Development

Successfully merging this pull request may close these issues.

4 participants