fix(telemetry): configure Sentry build as dist - #97700
Conversation
|
@youssef-lr Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@MelvinBot create an issue in E/App for this PR |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
![chatgpt-codex-connector[bot]](https://cdn.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 512a9eff90
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| integrations, | ||
| environment: CONFIG.ENVIRONMENT, | ||
| release: `${pkg.name}@${pkg.version}`, | ||
| release: `${pkg.name}@${semanticVersion}`, |
There was a problem hiding this comment.
For production or staging web builds with a build suffix such as 9.4.48-2, events now use release new.expensify@9.4.48 with dist 2, while config/rsbuild/rsbuild.common.ts lines 547-550 still creates the Sentry release and associates commits under new.expensify@9.4.48-2. Because these are distinct Sentry release identifiers, the release receiving events loses the uploaded release metadata and commit association; update the webpack plugin's release name/dist in the same change.
Useful? React with 👍 / 👎.
| environment: CONFIG.ENVIRONMENT, | ||
| release: `${pkg.name}@${pkg.version}`, | ||
| release: `${pkg.name}@${semanticVersion}`, | ||
| dist: buildNumber, |
There was a problem hiding this comment.
On Android and iOS builds, the SDK is initialized before JavaScript and autoInitializeNativeSdk remains disabled, but SentryNativeSDKManager.kt line 49 and SentryNativeSDKManager.m lines 69-76 still configure the full 9.4.48-2 value as the release without a dist. Consequently, native errors and JS errors from the same mobile build are assigned to different releases, and native releases retain the sorting problem this change is intended to fix; configure the semantic release and build dist in both native initializers as well.
Useful? React with 👍 / 👎.
| environment: CONFIG.ENVIRONMENT, | ||
| release: `${pkg.name}@${pkg.version}`, | ||
| release: `${pkg.name}@${semanticVersion}`, | ||
| dist: buildNumber, |
There was a problem hiding this comment.
Once events use this release/dist split, scripts/sentry/release-adoption-older-than.ts lines 432-447 still queries only release, and its parser requires the release suffix to match major.minor.patch-build. New rows such as new.expensify@9.4.48 therefore fail parsing and are silently excluded, causing the report to undercount users on older builds; query and combine dist with the semantic release before comparing it to the threshold.
Useful? React with 👍 / 👎.
|
No product review needed. |
2 participants
Explanation of Change
Currently in the Sentry dashboard, releases cannot correctly be sorted/filtered based on their semantic version, because the build number of the release (e.g. 9.4.47**-10**) is passed in the
releasefield instead ofdistin theSentry.init({ ... })call. Instead, the releases will be sorted by a simple string comparison, which leads to invalid ordering.E.g., version
9.4.5would appear after9.4.47, since by string comparison5is bigger than4on the same string position. Same with the "build number part" of the release version,9.4.47-2would appear after9.4.47-10,2is bigger than1In the according Sentry docs it is advised to pass the build number separately.
Fixed Issues
$
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari