From 9f967e124ef16312b10f4e5d7a2a0a36bc032fb7 Mon Sep 17 00:00:00 2001 From: Shruthilaya Date: Mon, 25 Oct 2021 08:02:46 -0400 Subject: [PATCH 1/2] fix(discover): Correct display when unfurling top events charts Some top events charts (like equations, some aggregations) should unfurl line charts to mimic discover. --- src/sentry/charts/types.py | 1 + .../integrations/slack/unfurl/discover.py | 29 ++++++++++ static/app/chartcuterie/discover.tsx | 56 +++++++++++++++++++ static/app/chartcuterie/types.tsx | 1 + static/app/utils/discover/fields.tsx | 2 +- 5 files changed, 88 insertions(+), 1 deletion(-) diff --git a/src/sentry/charts/types.py b/src/sentry/charts/types.py index b0b33933682fa9..06a865a77851b0 100644 --- a/src/sentry/charts/types.py +++ b/src/sentry/charts/types.py @@ -14,5 +14,6 @@ class ChartType(Enum): SLACK_DISCOVER_TOTAL_PERIOD = "slack:discover.totalPeriod" SLACK_DISCOVER_TOTAL_DAILY = "slack:discover.totalDaily" SLACK_DISCOVER_TOP5_PERIOD = "slack:discover.top5Period" + SLACK_DISCOVER_TOP5_PERIOD_LINE = "slack:discover.top5PeriodLine" SLACK_DISCOVER_TOP5_DAILY = "slack:discover.top5Daily" SLACK_DISCOVER_PREVIOUS_PERIOD = "slack:discover.previousPeriod" diff --git a/src/sentry/integrations/slack/unfurl/discover.py b/src/sentry/integrations/slack/unfurl/discover.py index 5a22df69ceacae..1e937c05ec7eb1 100644 --- a/src/sentry/integrations/slack/unfurl/discover.py +++ b/src/sentry/integrations/slack/unfurl/discover.py @@ -25,10 +25,28 @@ "default": ChartType.SLACK_DISCOVER_TOTAL_PERIOD, "daily": ChartType.SLACK_DISCOVER_TOTAL_DAILY, "top5": ChartType.SLACK_DISCOVER_TOP5_PERIOD, + "top5line": ChartType.SLACK_DISCOVER_TOP5_PERIOD_LINE, "dailytop5": ChartType.SLACK_DISCOVER_TOP5_DAILY, "previous": ChartType.SLACK_DISCOVER_PREVIOUS_PERIOD, } +line_plot_fields = { + "count_unique", + "failure_count", + "min", + "max", + "p50", + "p75", + "p95", + "p99", + "p100", + "percentile", + "avg", + "apdex", + "user_misery", + "failure_rate", +} + TOP_N = 5 MAX_PERIOD_DAYS_INCLUDE_PREVIOUS = 45 DEFAULT_PERIOD = "14d" @@ -48,6 +66,13 @@ def get_double_period(period: str) -> str: return f"{value * 2}{unit}" +def get_top5_display_mode(field: str) -> str: + if is_equation(field): + return "top5line" + + return "top5line" if field.split("(")[0] in line_plot_fields else "top5" + + def is_aggregate(field: str) -> bool: field_match = re.match(AGGREGATE_PATTERN, field) if field_match: @@ -141,6 +166,10 @@ def unfurl_discover( ) else: params.setlist("topEvents", [f"{TOP_N}"]) + + y_axis = params.getlist("yAxis")[0] + display_mode = get_top5_display_mode(y_axis) + else: # topEvents param persists in the URL in some cases, we want to discard # it if it's not a top n display type. diff --git a/static/app/chartcuterie/discover.tsx b/static/app/chartcuterie/discover.tsx index c5a052950d2468..0d4117a15f3002 100644 --- a/static/app/chartcuterie/discover.tsx +++ b/static/app/chartcuterie/discover.tsx @@ -200,6 +200,62 @@ discoverCharts.push({ ...slackChartSize, }); +discoverCharts.push({ + key: ChartType.SLACK_DISCOVER_TOP5_PERIOD_LINE, + getOption: ( + data: {stats: Record} | {seriesName?: string; stats: EventsStats} + ) => { + if (isArray(data.stats.data)) { + const color = theme.charts.getColorPalette(data.stats.data.length - 2); + + const lineSeries = LineSeries({ + data: data.stats.data.map(([timestamp, countsForTimestamp]) => [ + timestamp * 1000, + countsForTimestamp.reduce((acc, {count}) => acc + count, 0), + ]), + lineStyle: {color: color?.[0], opacity: 1}, + itemStyle: {color: color?.[0]}, + }); + + return { + ...slackChartDefaults, + useUTC: true, + color, + series: [lineSeries], + }; + } + + const stats = Object.values(data.stats); + const hasOther = Object.keys(data.stats).includes('Other'); + const color = theme.charts.getColorPalette(stats.length - 2 - (hasOther ? 1 : 0)); + if (hasOther) { + color.push(theme.chartOther); + } + + const series = stats + .sort((a, b) => (a.order ?? 0) - (b.order ?? 0)) + .map((topSeries, i) => + LineSeries({ + data: topSeries.data.map(([timestamp, countsForTimestamp]) => [ + timestamp * 1000, + countsForTimestamp.reduce((acc, {count}) => acc + count, 0), + ]), + lineStyle: {color: color?.[i], opacity: 1}, + itemStyle: {color: color?.[i]}, + }) + ); + + return { + ...slackChartDefaults, + xAxis: discoverxAxis, + useUTC: true, + color, + series, + }; + }, + ...slackChartSize, +}); + discoverCharts.push({ key: ChartType.SLACK_DISCOVER_TOP5_DAILY, getOption: ( diff --git a/static/app/chartcuterie/types.tsx b/static/app/chartcuterie/types.tsx index 8bc592ca445366..f3108afcea0ae7 100644 --- a/static/app/chartcuterie/types.tsx +++ b/static/app/chartcuterie/types.tsx @@ -11,6 +11,7 @@ export enum ChartType { SLACK_DISCOVER_TOTAL_PERIOD = 'slack:discover.totalPeriod', SLACK_DISCOVER_TOTAL_DAILY = 'slack:discover.totalDaily', SLACK_DISCOVER_TOP5_PERIOD = 'slack:discover.top5Period', + SLACK_DISCOVER_TOP5_PERIOD_LINE = 'slack:discover.top5PeriodLine', SLACK_DISCOVER_TOP5_DAILY = 'slack:discover.top5Daily', SLACK_DISCOVER_PREVIOUS_PERIOD = 'slack:discover.previousPeriod', } diff --git a/static/app/utils/discover/fields.tsx b/static/app/utils/discover/fields.tsx index 8829a7e13f9c38..4c73c667bfb2ff 100644 --- a/static/app/utils/discover/fields.tsx +++ b/static/app/utils/discover/fields.tsx @@ -402,7 +402,7 @@ export const AGGREGATIONS = { ], outputType: 'number', isSortable: true, - multiPlotType: 'area', + multiPlotType: 'line', }, failure_rate: { parameters: [], From 1a9268e90ddf2692f5fc8e2731cf087dd74310f3 Mon Sep 17 00:00:00 2001 From: Shruthilaya Date: Mon, 25 Oct 2021 08:42:15 -0400 Subject: [PATCH 2/2] tests and comments --- src/sentry/integrations/slack/unfurl/discover.py | 1 + src/sentry/web/frontend/debug/debug_chart_renderer.py | 2 ++ tests/sentry/integrations/slack/test_unfurl.py | 3 ++- 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/sentry/integrations/slack/unfurl/discover.py b/src/sentry/integrations/slack/unfurl/discover.py index 1e937c05ec7eb1..413e93260bfa51 100644 --- a/src/sentry/integrations/slack/unfurl/discover.py +++ b/src/sentry/integrations/slack/unfurl/discover.py @@ -30,6 +30,7 @@ "previous": ChartType.SLACK_DISCOVER_PREVIOUS_PERIOD, } +# All `multiPlotType: line` fields in /static/app/utils/discover/fields.tsx line_plot_fields = { "count_unique", "failure_count", diff --git a/src/sentry/web/frontend/debug/debug_chart_renderer.py b/src/sentry/web/frontend/debug/debug_chart_renderer.py index 5151c949b5e04b..9931f3ee67f71c 100644 --- a/src/sentry/web/frontend/debug/debug_chart_renderer.py +++ b/src/sentry/web/frontend/debug/debug_chart_renderer.py @@ -281,6 +281,8 @@ def get(self, request): charts.append(generate_chart(ChartType.SLACK_DISCOVER_TOTAL_DAILY, discover_empty)) charts.append(generate_chart(ChartType.SLACK_DISCOVER_TOP5_PERIOD, discover_top5)) charts.append(generate_chart(ChartType.SLACK_DISCOVER_TOP5_PERIOD, discover_empty)) + charts.append(generate_chart(ChartType.SLACK_DISCOVER_TOP5_PERIOD_LINE, discover_top5)) + charts.append(generate_chart(ChartType.SLACK_DISCOVER_TOP5_PERIOD_LINE, discover_empty)) charts.append(generate_chart(ChartType.SLACK_DISCOVER_TOP5_DAILY, discover_top5)) charts.append(generate_chart(ChartType.SLACK_DISCOVER_TOP5_DAILY, discover_empty)) charts.append( diff --git a/tests/sentry/integrations/slack/test_unfurl.py b/tests/sentry/integrations/slack/test_unfurl.py index 298d704aa638e1..0c7d3a83b47227 100644 --- a/tests/sentry/integrations/slack/test_unfurl.py +++ b/tests/sentry/integrations/slack/test_unfurl.py @@ -292,7 +292,8 @@ def test_unfurl_discover_short_url(self, mock_generate_chart): ) assert len(mock_generate_chart.mock_calls) == 1 - assert mock_generate_chart.call_args[0][0] == ChartType.SLACK_DISCOVER_TOP5_PERIOD + # Line chart expected since yAxis is count_unique(user) + assert mock_generate_chart.call_args[0][0] == ChartType.SLACK_DISCOVER_TOP5_PERIOD_LINE chart_data = mock_generate_chart.call_args[0][1] assert chart_data["seriesName"] == "count_unique(user)" # 2 + 1 cause of Other