Browse Source

feat(ddm-alerts): Set chart granularity and period via url (#60895)

Pass the interval and period to the alert creation page when creating an
alert based on a DDM chart.

Alert create:
 - Collect `timeWindow` from `eventView`
 - Collect statsPeriod query param

DDM:
 - Calculate nearest matching alert `interval` & `statsPeriod`
 - Use it in preview chart and for creating the alert URL
ArthurKnaus 1 year ago
parent
commit
8c685eee84

+ 1 - 1
static/app/types/metrics.tsx

@@ -39,7 +39,7 @@ export type MetricsApiRequestQuery = MetricsApiRequestMetric & {
   orderBy?: string;
   per_page?: number;
   project?: number[];
-  star?: DateString;
+  start?: DateString;
   statsPeriod?: string;
   useNewMetricsLayer?: boolean;
 };

+ 4 - 4
static/app/utils/metrics/index.spec.tsx

@@ -140,15 +140,15 @@ describe('getMetricsInterval', () => {
 
 describe('formatMetricUsingFixedUnit', () => {
   it('should return the formatted value with the short form of the given unit', () => {
-    expect(formatMetricUsingFixedUnit(123456, 'millisecond')).toBe('123,456 ms');
-    expect(formatMetricUsingFixedUnit(2.1231245, 'kibibyte')).toBe('2.123 KiB');
-    expect(formatMetricUsingFixedUnit(1222.1231245, 'megabyte')).toBe('1,222.123 MB');
+    expect(formatMetricUsingFixedUnit(123456, 'millisecond')).toBe('123,456ms');
+    expect(formatMetricUsingFixedUnit(2.1231245, 'kibibyte')).toBe('2.123KiB');
+    expect(formatMetricUsingFixedUnit(1222.1231245, 'megabyte')).toBe('1,222.123MB');
   });
 
   it.each(formattingSupportedMetricUnits.filter(unit => unit !== 'none'))(
     'appends a unit for every supported one (except none)',
     unit => {
-      expect(formatMetricUsingFixedUnit(1234.56, unit)).toMatch(/1,234\.56 .+/);
+      expect(formatMetricUsingFixedUnit(1234.56, unit)).toMatch(/1,234\.56.+/);
     }
   );
 

+ 1 - 1
static/app/utils/metrics/index.tsx

@@ -315,7 +315,7 @@ const getShortMetricUnit = (unit: string): string => METRIC_UNIT_TO_SHORT[unit]
 
 export function formatMetricUsingFixedUnit(value: number | null, unit: string) {
   return value !== null
-    ? `${round(value, 3).toLocaleString()} ${getShortMetricUnit(unit)}`.trim()
+    ? `${round(value, 3).toLocaleString()}${getShortMetricUnit(unit)}`.trim()
     : '\u2015';
 }
 

+ 6 - 11
static/app/utils/metrics/useMetricsData.tsx

@@ -1,7 +1,7 @@
 import {useEffect, useState} from 'react';
 
 import {ApiResult} from 'sentry/api';
-import {DateString, MetricsApiResponse} from 'sentry/types';
+import {DateString, MetricsApiRequestQuery, MetricsApiResponse} from 'sentry/types';
 import {
   getMetricsApiRequestQuery,
   mapToMRIFields,
@@ -26,15 +26,10 @@ function getRefetchInterval(
   return 60 * 1000;
 }
 
-export function useMetricsData({
-  mri,
-  op,
-  datetime,
-  projects,
-  environments,
-  query,
-  groupBy,
-}: MetricsQuery) {
+export function useMetricsData(
+  {mri, op, datetime, projects, environments, query, groupBy}: MetricsQuery,
+  overrides: Partial<MetricsApiRequestQuery> = {}
+) {
   const organization = useOrganization();
 
   const useNewMetricsLayer = organization.features.includes(
@@ -50,7 +45,7 @@ export function useMetricsData({
       groupBy,
     },
     {datetime, projects, environments},
-    {useNewMetricsLayer}
+    {...overrides, useNewMetricsLayer}
   );
 
   const metricsApiRepsonse = useApiQuery<MetricsApiResponse>(

+ 28 - 2
static/app/views/alerts/rules/metric/constants.tsx

@@ -1,4 +1,5 @@
 import {t} from 'sentry/locale';
+import {parsePeriodToHours} from 'sentry/utils/dates';
 import EventView from 'sentry/utils/discover/eventView';
 import {AggregationKeyWithAlias, LooseFieldKey} from 'sentry/utils/discover/fields';
 import {AggregationKey} from 'sentry/utils/fields';
@@ -166,6 +167,30 @@ export function createDefaultRule(
   };
 }
 
+function getAlertTimeWindow(period: string | undefined): TimeWindow | undefined {
+  if (!period) {
+    return undefined;
+  }
+
+  const periodMinutes = parsePeriodToHours(period) * 60;
+  if (periodMinutes < 0) {
+    return undefined;
+  }
+
+  const timeWindows = Object.values(TimeWindow)
+    .filter((value): value is TimeWindow => typeof value === 'number')
+    .sort();
+
+  for (let index = 0; index < timeWindows.length; index++) {
+    const timeWindow = timeWindows[index];
+    if (periodMinutes <= timeWindow) {
+      return timeWindow;
+    }
+  }
+
+  return undefined;
+}
+
 /**
  * Create an unsaved alert from a discover EventView object
  */
@@ -183,12 +208,13 @@ export function createRuleFromEventView(eventView: EventView): UnsavedMetricRule
     // p95() -> p95(transaction.duration)
     aggregate = eventView.getYAxis().slice(0, 3) + '(transaction.duration)';
   }
-
+  const defaultRule = createDefaultRule();
   return {
-    ...createDefaultRule(),
+    ...defaultRule,
     ...datasetAndEventtypes,
     query: parsedQuery?.query ?? eventView.query,
     aggregate,
+    timeWindow: getAlertTimeWindow(eventView.interval) ?? defaultRule.timeWindow,
     environment: eventView.environment.length ? eventView.environment[0] : null,
   };
 }

+ 2 - 3
static/app/views/alerts/rules/metric/ruleForm.tsx

@@ -872,7 +872,7 @@ class RuleFormContainer extends DeprecatedAsyncComponent<Props, State> {
   }
 
   renderTriggerChart() {
-    const {organization, ruleId, rule} = this.props;
+    const {organization, ruleId, rule, location} = this.props;
 
     const {
       query,
@@ -889,10 +889,9 @@ class RuleFormContainer extends DeprecatedAsyncComponent<Props, State> {
       dataset,
       alertType,
       isQueryValid,
-      location,
     } = this.state;
 
-    const isMigration = this.props.location?.query?.migration === '1';
+    const isMigration = location?.query?.migration === '1';
     // TODO(telemetry-experience): Remove this and all connected logic once the migration is complete
     const isTransactionMigration = isMigration && ruleNeedsMigration(rule);
     const isOnDemand = isOnDemandMetricAlert(dataset, aggregate, query);

+ 27 - 2
static/app/views/alerts/rules/metric/triggers/chart/index.tsx

@@ -33,6 +33,7 @@ import type {
   Project,
 } from 'sentry/types';
 import type {Series} from 'sentry/types/echarts';
+import {parsePeriodToHours} from 'sentry/utils/dates';
 import {DiscoverDatasets} from 'sentry/utils/discover/types';
 import {getForceMetricsLayerQueryExtras} from 'sentry/utils/metrics/features';
 import {formatMRIField} from 'sentry/utils/metrics/mri';
@@ -107,7 +108,7 @@ const MOST_TIME_PERIODS: readonly TimePeriod[] = [
  * TimeWindow determines data available in TimePeriod
  * If TimeWindow is small, lower TimePeriod to limit data points
  */
-const AVAILABLE_TIME_PERIODS: Record<TimeWindow, readonly TimePeriod[]> = {
+export const AVAILABLE_TIME_PERIODS: Record<TimeWindow, readonly TimePeriod[]> = {
   [TimeWindow.ONE_MINUTE]: [
     TimePeriod.SIX_HOURS,
     TimePeriod.ONE_DAY,
@@ -147,13 +148,37 @@ type State = {
   totalCount: number | null;
 };
 
+const getStatsPeriodFromQuery = (
+  queryParam: string | string[] | null | undefined
+): TimePeriod => {
+  if (typeof queryParam !== 'string') {
+    return TimePeriod.SEVEN_DAYS;
+  }
+  const inMinutes = parsePeriodToHours(queryParam || '') * 60;
+
+  switch (inMinutes) {
+    case 6 * 60:
+      return TimePeriod.SIX_HOURS;
+    case 24 * 60:
+      return TimePeriod.ONE_DAY;
+    case 3 * 24 * 60:
+      return TimePeriod.THREE_DAYS;
+    case 9999:
+      return TimePeriod.SEVEN_DAYS;
+    case 14 * 24 * 60:
+      return TimePeriod.FOURTEEN_DAYS;
+    default:
+      return TimePeriod.SEVEN_DAYS;
+  }
+};
+
 /**
  * This is a chart to be used in Metric Alert rules that fetches events based on
  * query, timewindow, and aggregations.
  */
 class TriggersChart extends PureComponent<Props, State> {
   state: State = {
-    statsPeriod: TimePeriod.SEVEN_DAYS,
+    statsPeriod: getStatsPeriodFromQuery(this.props.location.query.statsPeriod),
     totalCount: null,
     sampleRate: 1,
   };

+ 83 - 10
static/app/views/ddm/createAlertModal.tsx

@@ -6,6 +6,7 @@ import {ModalRenderProps} from 'sentry/actionCreators/modal';
 import {Button} from 'sentry/components/button';
 import {AreaChart} from 'sentry/components/charts/areaChart';
 import {HeaderTitleLegend} from 'sentry/components/charts/styles';
+import {getInterval} from 'sentry/components/charts/utils';
 import CircleIndicator from 'sentry/components/circleIndicator';
 import SelectControl from 'sentry/components/forms/controls/selectControl';
 import ProjectBadge from 'sentry/components/idBadge/projectBadge';
@@ -16,7 +17,8 @@ import PanelBody from 'sentry/components/panels/panelBody';
 import {Tooltip} from 'sentry/components/tooltip';
 import {t} from 'sentry/locale';
 import {space} from 'sentry/styles/space';
-import {Project} from 'sentry/types';
+import {PageFilters, Project} from 'sentry/types';
+import {parsePeriodToHours, statsPeriodToDays} from 'sentry/utils/dates';
 import {
   formatMetricUsingFixedUnit,
   MetricDisplayType,
@@ -27,7 +29,13 @@ import {useMetricsData} from 'sentry/utils/metrics/useMetricsData';
 import useOrganization from 'sentry/utils/useOrganization';
 import useProjects from 'sentry/utils/useProjects';
 import useRouter from 'sentry/utils/useRouter';
-import {Dataset, EventTypes} from 'sentry/views/alerts/rules/metric/types';
+import {AVAILABLE_TIME_PERIODS} from 'sentry/views/alerts/rules/metric/triggers/chart';
+import {
+  Dataset,
+  EventTypes,
+  TimePeriod,
+  TimeWindow,
+} from 'sentry/views/alerts/rules/metric/types';
 import {AlertWizardAlertNames} from 'sentry/views/alerts/wizard/options';
 import {getChartSeries} from 'sentry/views/ddm/widget';
 
@@ -54,6 +62,56 @@ function getInitialFormState(metricsQuery: MetricsQuery): FormState {
   };
 }
 
+function getAlertPeriod(metricsQuery: MetricsQuery) {
+  const {period, start, end} = metricsQuery.datetime;
+  const inHours = statsPeriodToDays(period, start, end) * 24;
+
+  switch (true) {
+    case inHours <= 6:
+      return TimePeriod.SIX_HOURS;
+    case inHours <= 24:
+      return TimePeriod.ONE_DAY;
+    case inHours <= 3 * 24:
+      return TimePeriod.THREE_DAYS;
+    case inHours <= 7 * 24:
+      return TimePeriod.SEVEN_DAYS;
+    case inHours <= 14 * 24:
+      return TimePeriod.FOURTEEN_DAYS;
+    default:
+      return TimePeriod.SEVEN_DAYS;
+  }
+}
+
+const TIME_WINDOWS_TO_CHECK = [
+  TimeWindow.ONE_MINUTE,
+  TimeWindow.FIVE_MINUTES,
+  TimeWindow.TEN_MINUTES,
+  TimeWindow.FIFTEEN_MINUTES,
+  TimeWindow.THIRTY_MINUTES,
+  TimeWindow.ONE_HOUR,
+  TimeWindow.TWO_HOURS,
+  TimeWindow.FOUR_HOURS,
+  TimeWindow.ONE_DAY,
+];
+
+function getAlertInterval(metricsQuery, period: TimePeriod) {
+  const interval = getInterval(metricsQuery.datetime, 'metrics');
+  const inMinutes = parsePeriodToHours(interval) * 60;
+
+  function toInterval(timeWindow: TimeWindow) {
+    return `${timeWindow}m`;
+  }
+
+  for (let index = 0; index < TIME_WINDOWS_TO_CHECK.length; index++) {
+    const timeWindow = TIME_WINDOWS_TO_CHECK[index];
+    if (inMinutes <= timeWindow && AVAILABLE_TIME_PERIODS[timeWindow].includes(period)) {
+      return toInterval(timeWindow);
+    }
+  }
+
+  return toInterval(TimeWindow.ONE_HOUR);
+}
+
 export function CreateAlertModal({Header, Body, Footer, metricsQuery}: Props) {
   const router = useRouter();
   const organization = useOrganization();
@@ -65,14 +123,25 @@ export function CreateAlertModal({Header, Body, Footer, metricsQuery}: Props) {
   const selectedProject = projects.find(p => p.id === formState.project);
   const isFormValid = formState.project !== null;
 
-  const {data, isLoading, refetch, isError} = useMetricsData({
-    mri: metricsQuery.mri,
-    op: metricsQuery.op,
-    projects: formState.project ? [parseInt(formState.project, 10)] : [],
-    environments: formState.environment ? [formState.environment] : [],
-    datetime: metricsQuery.datetime,
-    query: metricsQuery.query,
-  });
+  const alertPeriod = useMemo(() => getAlertPeriod(metricsQuery), [metricsQuery]);
+  const alertInterval = useMemo(
+    () => getAlertInterval(metricsQuery, alertPeriod),
+    [metricsQuery, alertPeriod]
+  );
+
+  const {data, isLoading, refetch, isError} = useMetricsData(
+    {
+      mri: metricsQuery.mri,
+      op: metricsQuery.op,
+      projects: formState.project ? [parseInt(formState.project, 10)] : [],
+      environments: formState.environment ? [formState.environment] : [],
+      datetime: {period: alertPeriod} as PageFilters['datetime'],
+      query: metricsQuery.query,
+    },
+    {
+      interval: alertInterval,
+    }
+  );
 
   const chartSeries = useMemo(
     () =>
@@ -141,6 +210,8 @@ export function CreateAlertModal({Header, Body, Footer, metricsQuery}: Props) {
         dataset: Dataset.GENERIC_METRICS,
         eventTypes: EventTypes.TRANSACTION,
         aggregate: MRIToField(metricsQuery.mri, metricsQuery.op!),
+        interval: alertInterval,
+        statsPeriod: alertPeriod,
         referrer: 'ddm',
         // Event type also needs to be added to the query
         query: `${metricsQuery.query}  event.type:transaction`.trim(),
@@ -149,6 +220,8 @@ export function CreateAlertModal({Header, Body, Footer, metricsQuery}: Props) {
       })}`
     );
   }, [
+    alertInterval,
+    alertPeriod,
     formState.environment,
     metricsQuery.mri,
     metricsQuery.op,