Skip to content

[No QA] feat: size tiers for account-size Sentry tags, add transactions_count and db_size tags - #102197

Merged
mountiny merged 3 commits into
Expensify:mainfrom
arekm213:arekm213/perf/98432-size-tier-tags
Oct 2, 2026
Merged

mountiny merged 3 commits into
Expensify:mainfrom
arekm213:arekm213/perf/98432-size-tier-tags

Conversation

@arekm213

@arekm213 arekm213 commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Explanation of Change

This is step 2 of the account-size telemetry work from the linked issue. Step 1 (#99068) added the exact values as numeric span attributes (reports_count_raw, transactions_count_raw, personal_details_count_raw, policies_count_raw, db_size_bytes, db_size_source) and left the bucketed tags untouched on purpose. The idea was to pick the new borders from real production data instead of guessing them. We now have three weeks of that data (7-day windows, all platforms, ~210K users a week with the attributes, adoption 90-100% per platform). This PR turns it into tags that a dashboard can group by.

What this PR does

  1. Replaces the ranges in the existing count tags with four named tiers: 1-small, 2-medium, 3-large, 4-xlarge. The borders sit where production latency actually bends, not at round numbers or quantiles. The numeric prefix is there because Sentry sorts tag values alphabetically and we want them in size order. The ranges are not part of the value, so if we ever move a border the dashboard history stays in one piece.
  2. Keeps the existing tag keys (reports_count, personal_details_count, policies_count) and only changes their values. No dashboard or alert uses them today, so nothing breaks and we don't need _v2 keys. Any saved filter on the old strings would stop matching, there just isn't one. Old values (0-60, 61-300, ...) and new values (1-small, ...) never overlap. Filtering on the tier values or on release:>=<this release> separates them, and the old values die out on their own as clients update (web the same day, native roughly a week to 90%).
  3. Adds two tags that didn't exist yet, transactions_count and db_size, with the same tier scheme. They carry information the report count doesn't. Search cost follows transactions, not reports, and db_size keeps growing where the count tags flatten out.
  4. Replaces the hand-written bucketing functions with a single getAccountSizeTier(tag, value). The borders live next to it in one table keyed by tag, so changing a border is a one-line change.
  5. Makes account size a normal dimension for performance dashboards. A string tag, unlike the raw attribute, works as a group-by and as an issue filter, so any existing or future dashboard can split its charts by account size with one extra clause. I'll propose a dashboard built on the tiers separately, once they have a week of production data behind them.

Proposed tiers

Borders are inclusive upper bounds. Each user is counted once, in the highest tier their spans reached during the last 7 days (Sep 16-23, production, all platforms, ~210K users with the attributes).

Tag Tier Range Users / week Share
reports_count 1-small 0-25 106 148 50.6%
2-medium 26-250 95 125 45.3%
3-large 251-1 000 6 908 3.3%
4-xlarge 1 001+ 1 734 0.8%
transactions_count 1-small 0-25 104 815 49.9%
2-medium 26-250 74 549 35.5%
3-large 251-1 000 24 470 11.6%
4-xlarge 1 001+ 6 420 3.1%
personal_details_count 1-small 0-100 138 935 66.3%
2-medium 101-1 000 62 821 30.0%
3-large 1 001+ 7 931 3.8%
policies_count 1-small 0-1 7 937 3.8%
2-medium 2 165 016 78.7%
3-large 3-10 35 153 16.8%
4-xlarge 11+ 1 688 0.8%
db_size 1-small < 5 MB 107 076 54.3%
2-medium 5-20 MB 79 940 40.6%
3-large 20-50 MB 8 183 4.2%
4-xlarge 50 MB+ 1 913 1.0%

Reports and transactions use the same ladder so the two tags read the same way. They still select different users: about three quarters of the accounts that are large by transactions are only medium by reports. Personal details gets three tiers because its latency curve only bends once, around 1 000.

What the data says

These are the findings from three weeks of production spans (p50 and p95 per slice, all platforms together and each platform separately). Each one either fixed a border or decided which tags stay.

  1. The distributions have a long tail. Small and medium together hold about 96% of users for every attribute, large is 3-12%, xlarge is 3% or less. Below 25 the only step you can see is between near-empty accounts (10 or fewer) and the rest, and that's not worth its own tier.
  2. 25 and 1 000 are where latency bends, on every platform. Search apply time climbs steeply up to about 25 reports or transactions and then flattens. Past 1 000 the search p95 roughly doubles (from under 2 s to close to 4 s), the StartupData.Apply p95 doubles too, and native cold start gets noticeably slower.
  3. 250 is a compromise, not a knee. Native search is flat between 100 and 1 000, so latency alone doesn't pick a border in that range. 250 is where Windows search and Android startup start to climb, and it keeps the medium/large split readable (45% vs 3% of users for reports). I think it's the right cut.
  4. Count tags saturate, db_size doesn't. Search p50 plateaus above 250 reports on native. StartupData.Apply keeps growing with database size on every platform and roughly doubles between the smallest and the largest tier (on Windows from about 340 ms to about 690 ms).
  5. Policies has one real step, between 1 and 2. Search apply p50 is 2-4x higher at 2 policies than at 0-1 on every platform (Windows 41 to 152 ms, iOS 30 to 68 ms), and 79% of users sit at exactly 2, so 2 is its own tier on purpose: it is the typical account and the cohort dashboards will compare against. Above 2 the curve is nearly flat on native and climbs slowly on web, so 3-10 and 11+ are there to keep the 17% and 0.8% tails visible rather than because latency bends again.

Changes in this PR

  • CONST.TELEMETRY.TAGS gets TRANSACTIONS_COUNT (transactions_count) and DB_SIZE (db_size). copyTagsToChildSpans copies Object.values(CONST.TELEMETRY.TAGS), so both new tags reach child spans without extra wiring.
  • CONST.TELEMETRY.SIZE_TIER holds the four tag values (1-small, 2-medium, 3-large, 4-xlarge).
  • New src/libs/telemetry/accountSizeTier.ts with getAccountSizeTier(tag, value) and a private border table, one array per tag. This is the only place where a range is written down. Borders are inclusive upper bounds. Two borders give three tiers (personal details), three borders give four. A tag without an entry in the table does not compile. It replaces bucketReportCount and bucketPolicyCount in TelemetrySynchronizer, both removed.
  • TelemetrySynchronizer sets reports_count, personal_details_count and policies_count through getAccountSizeTier in the same callbacks that register the raw attributes. It also starts setting transactions_count next to transactions_count_raw. An account with zero transactions gets 1-small.
  • databaseSizeTracker sets db_size wherever it sets db_size_bytes: for the value persisted from the previous session (applied at startup, so the startup span already has the tier) and after every new measurement. An unavailable measurement sets no tag, same as the attribute.
  • Nothing else changes. Raw attributes, measurement timing, debouncing and the sign-out handling from [No QA] feat: add account-size telemetry span attributes and database size measurement #99068 stay as they were: attributes are cleared on sign-out and account switch, tags keep their last value until the next account overwrites them, same as the old bucketed tags did. For db_size that window is longer, until the next account's first measurement, because the persisted size is wiped on sign-out.
  • Tests: new accountSizeTierTest checks 0, every border and every border plus one for every tag, the two-border ladder and the alphabetical order of the values. The existing raw-count tests in TelemetrySynchronizerTest now also assert the tier tag. databaseSizeTrackerTest asserts the tag for the persisted value at startup, for the first measurement and for a re-measurement that crosses the 20 MB border.

Fixed Issues

$ #98432
PROPOSAL: #98432

Tests

  1. Run npx jest tests/unit/accountSizeTierTest.ts tests/unit/TelemetrySynchronizerTest.ts tests/unit/databaseSizeTrackerTest.ts and verify all tests pass.
  2. With Sentry enabled in a dev build, sign in and trigger a traced flow (for example open a report). Verify the outgoing transaction envelope carries the reports_count, transactions_count, personal_details_count and policies_count tags with one of the values 1-small, 2-medium, 3-large, 4-xlarge, and that each tag matches its *_count_raw attribute against the borders in src/libs/telemetry/accountSizeTier.ts.
  3. Keep using the app for more than 10 s after the splash screen hides, trigger another traced flow and verify the envelope carries db_size next to db_size_bytes.
  4. Relaunch the app and verify the startup transaction already carries db_size (from the persisted measurement).
  • Verify that no errors appear in the JS console

Offline tests

Tag values come from local Onyx data and local storage size, so nothing changes offline. Verify the app behaves normally offline and no errors appear in the console.

QA Steps

No user-facing changes. Telemetry only. Validated by the unit tests and by the tag values showing up in Sentry after deploy.

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

No visible changes, telemetry only. Verified on web with the Sentry debug transport: outgoing transactions and their child spans carry all five tier tags and each tag matches its raw attribute (for example 716 reports -> 3-large, 4.4 MB -> 1-small).

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari

@arekm213

Copy link
Copy Markdown
Contributor Author

@MelvinBot review

@MelvinBot

Copy link
Copy Markdown
Contributor

🤖 Nice cleanup — the single border table is the right shape, and your claim about the new tags reaching child spans checks out: copyTagsToChildSpans.ts:26-31 iterates Object.values(CONST.TELEMETRY.TAGS), so transactions_count and db_size propagate with no extra wiring. All CI is green.

One thing I'd fix before this leaves draft, one question, two nits.

1. A 4th border silently emits undefined, and the test is built so it can't catch it

TIERS is a fixed 4-element array, but the index comes from the table's length:

src/libs/telemetry/accountSizeTier.ts:20-24

Three borders → TIERS[3] = 4-xlarge ✅. Two borders → TIERS[2] = 3-large ✅. Four borders → TIERS[4] → undefined. Typecheck won't catch it: noUncheckedIndexedAccess is not enabled (tsconfig.base.json sets only strict: true, which doesn't include it), so TIERS[i] types as SizeTier.

The unit test can't catch it either, because it derives the expected value with the same index arithmetic as the implementation:

tests/unit/accountSizeTierTest.ts:29

For a 4-border tag, the last row generates expect(getAccountSizeTier(tag, lastBorder + 1)).toBe(TIERS.at(4)) — i.e. expect(undefined).toBe(undefined), which passes.

Failure scenario: someone adds a border — the "one-line change" the file advertises — typecheck and the full suite stay green, and Sentry.setTag(tag, undefined) ships. Because copyTagsToChildSpans skips undefined values, the tag just vanishes from child spans for the biggest accounts, which is exactly the cohort these tags exist to measure. Silent gap in the dashboard rather than a visible break.

Cheapest fix is to let the table enforce the invariant, which also makes good on the "does not compile" guarantee your comment already promises:

type Borders = readonly [number, number] | readonly [number, number, number];

const BORDERS = {
    // ...unchanged
} as const satisfies Record<string, Borders>;

A fourth border is then a compile error. (Widening TIERS with a ?? XLARGE fallback also works, but the compile error is the stronger guarantee and keeps the tier list and border list provably in lockstep.)

2. What's the evidence behind the policies_count borders?

Your "What the data says" section justifies 25/250/1000 for reports and transactions, 1000 for personal details, and the db_size ladder — but not [1, 2, 10]. The resulting split puts 78.7% of users in 2-medium, which covers a single value (exactly 2 policies) and so has no internal resolution: four in five users land in a bucket a dashboard cannot split further.

Did latency actually bend at 1/2/10, or are these inherited from the old bucketPolicyCount edges (0-1, 2-10)? Either is defensible — worth stating in the description alongside the other four so the next person moving a border knows what they're trading against.

Nits

  • tests/unit/databaseSizeTrackerTest.ts:70 — there's no jest.clearAllMocks() in this file and the repo sets no global clearMocks, so this SMALL assertion is already satisfied by the identical call at line 45 and would pass even if this path stopped tagging. A beforeEach(() => jest.mocked(Sentry.setTag).mockClear()) makes it mean something. The LARGE assertion at line 98 is genuinely load-bearing and fine as-is.

  • db_size staleness on account switch — handleAccountChange clears the db_size_bytes attribute but not the new db_size tag. Unlike the count tags, which refresh as soon as the new account's Onyx collections repopulate, db_size only refreshes after the next debounced re-measurement (~5s), and Onyx.clear wipes the persisted value so the connect callback won't refresh it either. You do call the general "tags keep their last value" trade-off out in the description — just noting db_size has a longer stale window and is device-scoped rather than account-scoped, so a span can carry a tier with no matching raw attribute.


Next Steps: Reply with @MelvinBot <your feedback> if you'd like me to dig into any of these further.


view run

@arekm213

Copy link
Copy Markdown
Contributor Author

@MelvinBot review

@MelvinBot

Copy link
Copy Markdown
Contributor

🤖 Both fixes in 75a48e0 are correct and the open question is answered. This looks good to me now — one small leftover below, take it or leave it.

Re-checked:

  • accountSizeTier.ts:8 + :17 — as const satisfies Record<string, Borders> closes the hole. borders.length is now provably 2 or 3, so TIERS[borders.length] can't fall off the end, and a fourth border is a compile error instead of a silently dropped tag. as const is applied before satisfies checks it, so keyof typeof BORDERS stays the literal tag union and the "a tag without an entry does not compile" guarantee still holds. Typecheck is green on the new sha.
  • databaseSizeTrackerTest.ts:34-38 — the mockClear makes each setTag assertion stand on its own. All 8 test jobs pass.
  • Point 5 in the description answers the policies question properly, and I appreciate that it says out loud which borders are latency knees (the 1→2 step, 2-4x on every platform) and which are just there to keep the 17% and 0.8% tails visible. That's the useful version of that note.
  • The db_size stale-window caveat is now in the description too.

Only Check independent approval is red, which is expected on a draft.

Leftover: the transactions guard is the odd one out

TelemetrySynchronizer.ts:99 uses && where its three siblings use || (reports, for comparison):

if (!value && !session?.email) {

It only bails when both are falsy. Half of that is deliberate and the comment explains it: no value plus a live session is the zero-transactions case you want to count. The other half isn't — value present with no session also falls through, and none of the other three callbacks allow that.

Low stakes, and I'd call it a consistency wart rather than a bug: on sign-out Onyx.clear empties both, so it bails correctly. The reachable window is cold start, where the transaction collection can hydrate before the session key is read — and there the count is right anyway. Worth a mention only because this PR promotes that path from writing a span attribute to writing a Sentry tag, and tags are sticky on the scope until something overwrites them.

Splitting the guard keeps the intended behavior and makes the deliberate part obvious:

if (!session?.email) {
    return;
}
// An account can have zero transactions, which Onyx delivers as undefined. Count it as 0 so the zero cohort stays in the data.
const transactionsCount = Object.keys(value ?? {}).length;

Since it predates this PR, equally fine to leave it and let the four callbacks get cleaned up together later.


Next Steps: Reply with @MelvinBot <your feedback> if you want me to look at anything else here.


view run

@arekm213

Copy link
Copy Markdown
Contributor Author

@MelvinBot Leaving the && on purpose. At cold start the transaction collection can hydrate before the session key and nothing re-fires the callback once the session arrives, so requiring the session here would drop transactions_count for the whole session on those starts (reports and personal details already have that gap). The value in that window is the same device's cached collection, so the count is right. Sign-out still bails because Onyx.clear empties both

@MelvinBot

Copy link
Copy Markdown
Contributor

🤖 Agreed, keep the &&. I checked it against 75a48e0 and your reasoning holds.

  • The session callback never re-sends the counts. TelemetrySynchronizer.ts:42-57 only calls sendPoliciesContext(). If transactions hydrate first, || would drop the tag until the collection changes again.
  • A first session with an email doesn't call handleAccountChange(), so the value set during that window stays.

Optional: add half a line to the comment at :102 about the hydration order. That way the next reviewer won't flag it again.


view run

@arekm213
arekm213 marked this pull request as ready for review September 28, 2026 13:42
@arekm213
arekm213 requested review from a team as code owners September 28, 2026 13:42
@melvin-bot
melvin-bot Bot requested review from aimane-chnaif and flaviadefaria and removed request for a team September 28, 2026 13:42
@melvin-bot

melvin-bot Bot commented Sep 28, 2026

Copy link
Copy Markdown

@aimane-chnaif 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]

Comment thread src/libs/telemetry/accountSizeTier.ts
@aimane-chnaif

aimane-chnaif commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
  • I verified tests pass on all platforms & I tested again on:
    • Android: HybridApp
    • Android: mWeb Chrome
    • iOS: HybridApp
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: HybridApp
Android: mWeb Chrome
iOS: HybridApp
iOS: mWeb Safari
MacOS: Chrome / Safari
web1.mov
web2.mov

@MelvinBot

Copy link
Copy Markdown
Contributor

🤖 Looks good to me. Since my last review, d09ab4d only adds two comments and no logic. Nothing new to fix.

All CI checks pass except Check independent approval, which waits for a reviewer to approve.


view run

@melvin-bot
melvin-bot Bot requested a review from mountiny October 1, 2026 08:31
@MelvinBot

Copy link
Copy Markdown
Contributor

🤖 Code quality is good. The code is small, typed, and well tested. I have one suggestion and one nit, and neither blocks the PR.

1. Each tag key is written twice per call. All five call sites pass the same tag to Sentry.setTag and to getAccountSizeTier, for example TelemetrySynchronizer.ts:157. A mismatch like setTag(REPORTS_COUNT, getAccountSizeTier(TRANSACTIONS_COUNT, n)) would compile and quietly use the wrong borders. You could add a setAccountSizeTag(tag, value) helper to accountSizeTier.ts that calls Sentry.setTag(tag, getAccountSizeTier(tag, value)). Then each caller names the tag once.

All five call sites

2. Nit: move the hydration comment and fix its comma splice. The comment at TelemetrySynchronizer.ts:103 explains the && guard on line 99, but it sits below the guard, next to the count. Moving it above line 99 ties it to the code it explains. Also, "too, on cold start" joins two sentences with a comma. Use a period or a semicolon there.


view run

@MelvinBot

Copy link
Copy Markdown
Contributor

🤖 I found one likely regression. It is small. Nothing in the repo reads the old tag values, and CI is green.

A zero-transaction account can keep the previous account's transactions_count tier

This happens when someone signs out and signs in to a different account without restarting the app.

  1. Sentry tags survive sign-out by design (TelemetrySynchronizerTest.ts:273-298). Before this PR, transactions_count was only a span attribute, which sign-out clears.
  2. On sign-out, Onyx.clear sends undefined to the transaction callback while the session is empty, so it bails at TelemetrySynchronizer.ts:99-101.
  3. If the new account has no transactions, the collection probably never changes again, so the callback doesn't re-fire. The old tier (for example, 4-xlarge) stays on every span for that session. Meanwhile transactions_count_raw is missing.

Reports and personal details have the same gap, but a new account always loads some of those, so their tags get overwritten. Zero transactions is a common state.

Possible fix: Cache the count in the transaction callback before the session check. Then, when a session with an email arrives, call sendTransactionsCount with the cached count from the session callback (:42-57). This also closes the cold-start hydration gap you described, so you could drop the && workaround.

What else I checked (no regression found)
  • Old tag values: nothing in src, tests, or .github matches 0-60, 0-1, 1000+, and so on. Only copyTagsToChildSpans and the setters use the tag keys.
  • New tags on child spans: copyTagsToChildSpans.ts:8 copies them as intended. Each child span gets two extra short strings.
  • db_size after sign-out: LAST_MEASURED_DATABASE_SIZE isn't preserved, and the initial measurement runs once per app run. The tier refreshes on the next debounced remeasurement after the new account's data loads. The old tier stays only for that short window, or longer if that remeasurement returns unavailable. That's acceptable.
  • Sentry import in databaseSizeTracker: TelemetrySynchronizer already loads that module on every platform, and the tests mock setTag.

I found this by reading the code at d09ab4d. I didn't test it in the app because Sentry tags don't appear in the UI.


view run

@mountiny mountiny left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! looks good to me

@mountiny
mountiny merged commit eb783de into Expensify:main Oct 2, 2026
35 of 37 checks passed
@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here.

@OSBotify OSBotify mentioned this pull request Oct 4, 2026
59 tasks done
@OSBotify

OSBotify commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by https://github.com/mountiny in version: 9.5.2-0 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

@MelvinBot

Copy link
Copy Markdown
Contributor

No help site update needed: this PR only changes internal Sentry telemetry tags (account-size tiers), so users won't see any change in the app.


view run

@OSBotify

OSBotify commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by https://github.com/Julesssss in version: 9.5.2-4 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

Bundle Size Analysis (Sentry):

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.

5 participants