-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
feat(insights): Hide healthy Session Health counts & rates by default #87252
Conversation
@@ -471,7 +471,7 @@ export default storyBook('TimeSeriesWidgetVisualization', (story, APIReference) | |||
|
|||
story('Legends', () => { | |||
const [legendSelection, setLegendSelection] = useState<LegendSelection>({ | |||
'p99(span.duration)': false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did this get changed 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the series in the chart is called p99
, so having p99(span.duration)
in the state by default did nothing to hide data.
Now when the story loads you can see the json printed {p99: false}
while the chart has 2 series in the legend, but only one series is rendered.
I think it's more obvious this way that the legendSelection
prop is working properly without having to click first.
static/app/views/insights/sessions/charts/userHealthCountChart.tsx
Outdated
Show resolved
Hide resolved
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
@@ -1,27 +1,21 @@ | |||
import ExternalLink from 'sentry/components/links/externalLink'; | |||
import {t, tct} from 'sentry/locale'; | |||
import {InsightsLineChartWidget} from 'sentry/views/insights/common/components/insightsLineChartWidget'; | |||
import {FRONTEND_LANDING_SUB_PATH} from 'sentry/views/insights/pages/frontend/settings'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still used in other places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. a few spots
|
||
const aliases = { | ||
healthy_session_count: t('Healthy session count'), | ||
crashed_session_count: frontendPath | ||
? t('Unhandled error session count') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think @jas-kas wanted to keep this distinction b/t frontend + mobile wording intentional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okk np, i like that it matches. just ccing @jas-kas so she's aware!
This PR re-labels the charts and series so that the series names are shorter, less redundant. As a consequence the chart titles are longer again.
The charts also hide the "healthy" series of data by default, users can click the legend to toggle that series though.
Toggling a series in one chart will affect all charts when the series name is the same. All the names are consistent now between all the charts.