Skip to content

Update Sentry release param to use app version - #96

Merged
derekblank merged 1 commit into
trunkfrom
task/update-sentry-release-param
May 7, 2024
Merged

Update Sentry release param to use app version#96
derekblank merged 1 commit into
trunkfrom
task/update-sentry-release-param

Conversation

@derekblank

Copy link
Copy Markdown
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 a team and fluiddot May 7, 2024 09:47
@derekblank derekblank self-assigned this May 7, 2024
Comment thread src/index.ts
debug: true,
enabled: process.env.NODE_ENV !== 'development',
release: COMMIT_HASH,
release: app.getVersion() ? app.getVersion() : COMMIT_HASH,

@derekblank derekblank May 7, 2024

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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.

@dcalhoun dcalhoun left a comment

Copy link
Copy Markdown
Member

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!

Comment thread src/index.ts
debug: true,
enabled: process.env.NODE_ENV !== 'development',
release: COMMIT_HASH,
release: app.getVersion() ? app.getVersion() : COMMIT_HASH,

Copy link
Copy Markdown
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

derekblank commented May 7, 2024

Copy link
Copy Markdown
Contributor Author

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
@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

Labels

None yet

2 participants