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

Update Sentry release param to use app version #96

Merged
merged 1 commit into from
May 7, 2024

Conversation

derekblank
Copy link
Contributor

Related to https://github.com/Automattic/dotcom-forge/issues/6671

Ensures Sentry events reference the release version, when available.

Proposed Changes

Updates Sentry.init params to pass the release version (via app.getVersion()). If there is no release version, the commit hash is used (matching current behavior).

Testing Instructions

Log Sentry.init values locally to observe app release version.

studio/src/index.ts

Lines 28 to 33 in ecc7e62

Sentry.init( {
dsn: 'https://97693275b2716fb95048c6d12f4318cf@o248881.ingest.sentry.io/4506612776501248',
debug: true,
enabled: process.env.NODE_ENV !== 'development',
release: app.getVersion() ? app.getVersion() : COMMIT_HASH,
} );

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@derekblank derekblank requested review from fluiddot and a team May 7, 2024 09:47
@derekblank derekblank self-assigned this May 7, 2024
@@ -29,7 +29,7 @@ Sentry.init( {
dsn: 'https://97693275b2716fb95048c6d12f4318cf@o248881.ingest.sentry.io/4506612776501248',
debug: true,
enabled: process.env.NODE_ENV !== 'development',
release: COMMIT_HASH,
release: app.getVersion() ? app.getVersion() : COMMIT_HASH,
Copy link
Contributor Author

@derekblank derekblank May 7, 2024

Choose a reason for hiding this comment

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

app.getVersion() is an Electron method that returns the version of the loaded application found in the app package.json file.

Copy link
Member

Choose a reason for hiding this comment

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

I paused to think if we should/need to update/remove the existing COMMIT_HASH references here (or elsewhere). My thought is that retaining the references still makes sense.

Many of the other references appear to log both values, so having both likely is intentional and helpful context. I'm not sure under what circumstances this particular line would use the COMMIT_HASH.

Copy link
Member

@dcalhoun dcalhoun left a comment

Choose a reason for hiding this comment

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

This will be very helpful for evaluating release stability. Thank you!

@@ -29,7 +29,7 @@ Sentry.init( {
dsn: 'https://97693275b2716fb95048c6d12f4318cf@o248881.ingest.sentry.io/4506612776501248',
debug: true,
enabled: process.env.NODE_ENV !== 'development',
release: COMMIT_HASH,
release: app.getVersion() ? app.getVersion() : COMMIT_HASH,
Copy link
Member

Choose a reason for hiding this comment

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

I paused to think if we should/need to update/remove the existing COMMIT_HASH references here (or elsewhere). My thought is that retaining the references still makes sense.

Many of the other references appear to log both values, so having both likely is intentional and helpful context. I'm not sure under what circumstances this particular line would use the COMMIT_HASH.

@derekblank
Copy link
Contributor Author

derekblank commented May 7, 2024

I paused to think if we should/need to update/remove the existing COMMIT_HASH references here (or elsewhere). My thought is that retaining the references still makes sense.

@dcalhoun Thanks for the feedback -- I had also considered this. In fact, I initially removed COMMIT_HASH entirely from the Sentry config init, as both the version and the commit hash are logged again in app.on('ready').

After reviewing the Sentry docs on the release param, I decided to keep COMMIT_HASH as a fallback . Reviewing some existing references from the Studio build scripts where the version and commit refs are combined, perhaps there may also be a case where we'd still want any ad-hoc CI builds to reference a commit hash as the release param specifically when reported to Sentry, for example. I think if we find that app.getVersion is adequate enough alone after a few semver releases, we could consider it safe to remove the COMMIT_HASH fallback.

@derekblank derekblank merged commit 6a3287a into trunk May 7, 2024
11 checks passed
@derekblank derekblank deleted the task/update-sentry-release-param branch May 7, 2024 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants