Browse Source

feat(discover): Add logic to disable the baseline toggle based on query (#38723)

This PR just adds logic to disable the processed events
toggle if we know that the discover query is going to hit
the errors dataset. Metrics is only available for transactions.

Note this logic is _not_ checking for metrics compatibility.
Shruthi 2 years ago
parent
commit
a3afa78e55

+ 66 - 3
static/app/views/eventsV2/chartFooter.spec.tsx

@@ -6,6 +6,7 @@ import {render, screen, userEvent} from 'sentry-test/reactTestingLibrary';
 import {t} from 'sentry/locale';
 import {Organization} from 'sentry/types/organization';
 import {Project} from 'sentry/types/project';
+import EventView from 'sentry/utils/discover/eventView';
 import {DisplayModes} from 'sentry/utils/discover/types';
 import {MetricsCardinalityProvider} from 'sentry/utils/performance/contexts/metricsCardinality';
 import ChartFooter from 'sentry/views/eventsV2/chartFooter';
@@ -40,12 +41,18 @@ function metricsCardinalityWrapped(
 describe('EventsV2 > ChartFooter', function () {
   const features = ['discover-basic'];
   const yAxisValue = ['count()', 'failure_count()'];
-  const yAxisOptions = [
+  let yAxisOptions = [
     {label: 'count()', value: 'count()'},
     {label: 'failure_count()', value: 'failure_count()'},
   ];
-
   const project = TestStubs.Project();
+  const eventView = EventView.fromSavedQuery({
+    id: '',
+    name: 'test query',
+    version: 2,
+    fields: ['transaction', 'count()'],
+    projects: [project.id],
+  });
 
   afterEach(function () {});
 
@@ -78,6 +85,7 @@ describe('EventsV2 > ChartFooter', function () {
         topEvents="5"
         showBaseline={false}
         setShowBaseline={() => undefined}
+        eventView={eventView}
       />
     );
 
@@ -121,6 +129,7 @@ describe('EventsV2 > ChartFooter', function () {
         topEvents="5"
         showBaseline={false}
         setShowBaseline={() => undefined}
+        eventView={eventView}
       />
     );
 
@@ -156,6 +165,7 @@ describe('EventsV2 > ChartFooter', function () {
         topEvents="5"
         showBaseline={false}
         setShowBaseline={() => undefined}
+        eventView={eventView}
       />
     );
 
@@ -185,6 +195,7 @@ describe('EventsV2 > ChartFooter', function () {
         topEvents="5"
         showBaseline={false}
         setShowBaseline={() => undefined}
+        eventView={eventView}
       />
     );
 
@@ -210,11 +221,16 @@ describe('EventsV2 > ChartFooter', function () {
       ],
     });
 
+    yAxisOptions = [
+      {label: 'count()', value: 'count()'},
+      {label: 'p50(measurements.lcp)', value: 'p50(measurements.lcp)'},
+    ];
+
     const chartFooter = (
       <ChartFooter
         organization={organization}
         total={100}
-        yAxisValue={['count()']}
+        yAxisValue={['p50(measurements.lcp)']}
         yAxisOptions={yAxisOptions}
         onAxisChange={jest.fn}
         displayMode={DisplayModes.DEFAULT}
@@ -224,6 +240,7 @@ describe('EventsV2 > ChartFooter', function () {
         topEvents="5"
         showBaseline={false}
         setShowBaseline={() => undefined}
+        eventView={eventView}
       />
     );
 
@@ -233,6 +250,51 @@ describe('EventsV2 > ChartFooter', function () {
     expect(screen.getByTestId('processed-events-toggle')).toBeEnabled();
   });
 
+  it('disables toggle if discover hits the events dataset', function () {
+    addMetricsDataMock({
+      metricsCount: 100,
+      nullCount: 0,
+      unparamCount: 1,
+    });
+
+    yAxisOptions = [
+      {label: 'count()', value: 'count()'},
+      {label: 'failure_count()', value: 'failure_count()'},
+    ];
+
+    const organization = TestStubs.Organization({
+      features: [
+        ...features,
+        'discover-metrics-baseline',
+        'performance-transaction-name-only-search',
+        'organizations:performance-transaction-name-only-search',
+      ],
+    });
+
+    const chartFooter = (
+      <ChartFooter
+        organization={organization}
+        total={100}
+        yAxisValue={['count()']}
+        yAxisOptions={yAxisOptions}
+        onAxisChange={jest.fn}
+        displayMode={DisplayModes.DEFAULT}
+        displayOptions={[{label: DisplayModes.DEFAULT, value: DisplayModes.DEFAULT}]}
+        onDisplayChange={() => undefined}
+        onTopEventsChange={() => undefined}
+        topEvents="5"
+        showBaseline={false}
+        setShowBaseline={() => undefined}
+        eventView={eventView}
+      />
+    );
+
+    render(metricsCardinalityWrapped(chartFooter, organization, project));
+
+    expect(screen.getByText(/Processed events/i)).toBeInTheDocument();
+    expect(screen.getByTestId('processed-events-toggle')).toBeDisabled();
+  });
+
   it('renders multi value y-axis dropdown selector on a non-Top display', function () {
     const organization = TestStubs.Organization({
       features,
@@ -253,6 +315,7 @@ describe('EventsV2 > ChartFooter', function () {
         topEvents="5"
         showBaseline={false}
         setShowBaseline={() => undefined}
+        eventView={eventView}
       />
     );
 

+ 10 - 1
static/app/views/eventsV2/chartFooter.tsx

@@ -12,12 +12,16 @@ import {
 import Switch from 'sentry/components/switchButton';
 import {t} from 'sentry/locale';
 import {Organization, SelectValue} from 'sentry/types';
+import EventView from 'sentry/utils/discover/eventView';
 import {TOP_EVENT_MODES} from 'sentry/utils/discover/types';
 import {useMetricsCardinalityContext} from 'sentry/utils/performance/contexts/metricsCardinality';
 
+import {usesTransactionsDataset} from './utils';
+
 type Props = {
   displayMode: string;
   displayOptions: SelectValue<string>[];
+  eventView: EventView;
   onAxisChange: (value: string[]) => void;
   onDisplayChange: (value: string) => void;
   onTopEventsChange: (value: string) => void;
@@ -43,6 +47,7 @@ export default function ChartFooter({
   setShowBaseline,
   showBaseline,
   organization,
+  eventView,
 }: Props) {
   const metricsCardinality = useMetricsCardinalityContext();
   const elements: React.ReactNode[] = [];
@@ -72,7 +77,11 @@ export default function ChartFooter({
             <Switch
               data-test-id="processed-events-toggle"
               isActive={showBaseline}
-              isDisabled={metricsCardinality.outcome?.forceTransactionsOnly}
+              isDisabled={
+                metricsCardinality.outcome?.forceTransactionsOnly ||
+                displayMode !== 'default' ||
+                !usesTransactionsDataset(eventView, yAxisValue)
+              }
               size="lg"
               toggle={() => setShowBaseline(!showBaseline)}
             />

+ 1 - 0
static/app/views/eventsV2/resultsChart.tsx

@@ -275,6 +275,7 @@ class ResultsChartContainer extends Component<ContainerProps, ContainerState> {
           total={total}
           yAxisValue={yAxis}
           yAxisOptions={yAxisOptions}
+          eventView={eventView}
           onAxisChange={onAxisChange}
           displayOptions={displayOptions}
           displayMode={eventView.getDisplayMode()}

+ 29 - 0
static/app/views/eventsV2/utils.tsx

@@ -22,6 +22,7 @@ import {
   explodeFieldString,
   Field,
   getAggregateAlias,
+  getAggregateArg,
   getColumnsAndAggregates,
   getEquation,
   isAggregateEquation,
@@ -380,6 +381,34 @@ function generateAdditionalConditions(
   return conditions;
 }
 
+/**
+ * Discover queries can query either Errors, Transactions or a combination
+ * of the two datasets. This is a util to determine if the query will excusively
+ * hit the Transactions dataset.
+ */
+export function usesTransactionsDataset(eventView: EventView, yAxisValue: string[]) {
+  let usesTransactions: boolean = false;
+  const parsedQuery = new MutableSearch(eventView.query);
+  for (let index = 0; index < yAxisValue.length; index++) {
+    const yAxis = yAxisValue[index];
+    const aggregateArg = getAggregateArg(yAxis) ?? '';
+    if (isMeasurement(aggregateArg) || aggregateArg === 'transaction.duration') {
+      usesTransactions = true;
+      break;
+    }
+    const eventTypeFilter = parsedQuery.getFilterValues('event.type');
+    if (
+      eventTypeFilter.length > 0 &&
+      eventTypeFilter.every(filter => filter === 'transaction')
+    ) {
+      usesTransactions = true;
+      break;
+    }
+  }
+
+  return usesTransactions;
+}
+
 function generateExpandedConditions(
   eventView: EventView,
   additionalConditions: Record<string, string>,