Repository navigation
Allow Trace Metrics Explore ranges based on subscription retention - #127237
jayantkageri wants to merge 8 commits into
Conversation
…ange-high Trace Metrics in Explore had a hardcoded 30-day limit (maxPickableDays and maxUpgradableDays both set to 30), with no path for orgs with the visibility-explore-range-high feature flag to access longer ranges. This aligns Trace Metrics with Spans and Errors: 30 days selectable by default, upgradable to 90 days (upgradable via upsell) or 90/90 with the flag. Files changed: - static/app/utils/useMaxPickableDays.tsx - static/app/utils/useMaxPickableDays.spec.tsx
github-actions
Bot
added
the
Scope: Frontend
jayantkageri
changed the title
JoshuaKGoldberg
left a comment
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
👋 Hi @jayantkageri, thanks for picking this up!
This change is a good start but isn't quite enough. It won't affect sentry.io: the SaaS build replaces this function via the react-hook:use-max-pickable-days override in static/gsApp/overrides/useMaxPickableDays.tsx, which still hardcodes TRACE_METRICS to 30/30.
The issue also asks for making it clear that ranges past 30 days come back at reduced fidelity, which this PR doesn't cover yet.
Do you have bandwidth to take all that on?
| const maxPickableDays = organization.features.includes( | ||
| 'visibility-explore-range-high' | ||
| ) | ||
| ? MAX_PICKABLE_DAYS | ||
| : 30; |
There was a problem hiding this comment.
visibility-explore-range-high is a spans/errors flag, and 90 days isn't the actual metrics limit. Downsampled metrics retention comes from the subscription (effectiveRetentions.traceMetric.downsampled), so the range should be based on that rather than this flag.
There was a problem hiding this comment.
I will remove the visibility-explore-range-high flag check from static/app/utils/useMaxPickableDays.tsx so self-hosted stays at the default 30 days. The gsApp override will derive the range directly from subscription.effectiveRetentions.traceMetric.downsampled instead.
There was a problem hiding this comment.
[Bug] The flag check is still here. As you mentioned, this case (and the flagged test in useMaxPickableDays.spec.tsx) should go back to the original 30/30 alongside logs.
|
Hi, Thanks for the review and for spelling out what’s missing! 🙏 |
The gsApp override for TRACE_METRICS was a hardcoded placeholder (30/30 days) with a TODO comment. This updates it to use subscription-based retention tiers from effectiveRetentions.traceMetric, matching the pattern used by SPANS and LOGS: - maxPickableDays from subscription retention or 30-day default - maxUpgradableDays to 90-day baseline (business plans get downsampled) Tests updated to reflect new behavior in both test sections.
… days Addresses review feedback about clarifying that queries past 30 days return downsampled data with reduced fidelity. Added documentation comments to both the base implementation and gsApp override.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0d41c00. Configure here.
JoshuaKGoldberg
left a comment
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
Thanks for the update @jayantkageri, moving the fix into the gsApp override is the right direction. A few things are still needed before this is ready:
- [Bug] The
visibility-explore-range-highchange in the self-hosted default is still here. - [Bug] Orgs on the default 30 days see a disabled "Last 90 days" option with no explanation.
- [Docs] Could you also refresh the PR description? It still describes the flag-based approach. Please also switch "Fixes #114069" to "Refs #114069".
Making it clear that ranges past 30 days come back at reduced fidelity will likely need design input, so we'll handle that separately rather than in this PR.
| maxUpgradableDays: 30, | ||
| maxPickableDays, | ||
| maxUpgradableDays: Math.max( | ||
| 90, // use 90 as a placeholder, business plans get downsampled retention |
There was a problem hiding this comment.
[Bug] With a 90 placeholder and no upsellFooter, orgs on the default 30 days see a disabled "Last 90 days" option with nothing explaining why. Metrics doesn't have an upgrade path to point to yet, so this should use 30 as the placeholder like the logs case below.
| const maxPickableDays = organization.features.includes( | ||
| 'visibility-explore-range-high' | ||
| ) | ||
| ? MAX_PICKABLE_DAYS | ||
| : 30; |
There was a problem hiding this comment.
[Bug] The flag check is still here. As you mentioned, this case (and the flagged test in useMaxPickableDays.spec.tsx) should go back to the original 30/30 alongside logs.
jayantkageri
changed the title
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you add the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
|
@JoshuaKGoldberg The requested changes are now on this PR: Trace Metrics uses subscription retention in the SaaS override, self-hosted stays at 30/30, the disabled 90-day placeholder is gone, and the description references the issue without closing it. I also synced the branch with current The |
|
Hi @JoshuaKGoldberg. The requested changes are done. Could you re-review when you have a chance? |
Overview
Trace Metrics in Explore was limited to 30 days even when an organization's subscription retained metrics longer. The sentry.io override now uses
effectiveRetentions.traceMetric.standardand.downsampledto set the selectable range, with a 30-day fallback. The self-hosted default remains 30 days.The date picker no longer shows an unexplained disabled 90-day option for organizations limited to 30 days. Tests cover the default and extended retention cases, plus the self-hosted behavior with the unrelated
visibility-explore-range-highflag.The user-facing notice about reduced fidelity after 30 days needs design input and will be handled separately.
Refs #114069
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.