Browse Source

fix(perf): Transaction name search wildcard, parsing and metrics incompatible filters (#44639)

This PR fixes:
- Wildcard search. Previously, free text would get interpreted as a
wildcard query. This PR brings that back
<img width="1191" alt="Screen Shot 2023-02-14 at 4 31 15 PM"
src="https://user-images.githubusercontent.com/23648387/218895509-426992a7-ed94-43a9-9323-c535b704de8e.png">

- Previously, we'd only take a first element of the parsed query. This
change now accounts for transaction names with a space in them so we
correctly apply the transaction filter

- If any filters were applied in transaction summary, navigating back to
the landing page wouldn't clear those filters correctly so the metrics
query would fail.
**Before:**
<img width="1743" alt="Screen Shot 2023-02-14 at 4 30 41 PM"
src="https://user-images.githubusercontent.com/23648387/218895505-7f16298d-2fe9-4cbd-84b1-56c7cca1feeb.png">
<img width="1817" alt="Screen Shot 2023-02-14 at 4 30 31 PM"
src="https://user-images.githubusercontent.com/23648387/218895500-ddd9123f-c739-44d4-8c8e-fd383408b24b.png">

**After:**
<img width="1202" alt="Screen Shot 2023-02-14 at 4 30 59 PM"
src="https://user-images.githubusercontent.com/23648387/218895507-17a2ad44-e1e9-4d11-95d7-646f294cd9ee.png">

Fixes PERF-1960, PERF-1939
Dameli Ushbayeva 2 years ago
parent
commit
276bf87c31

+ 2 - 2
static/app/components/performance/searchBar.spec.tsx

@@ -139,7 +139,7 @@ describe('SearchBar', () => {
     expect(onSearch).toHaveBeenCalledWith('transaction:clients.fetch');
   });
 
-  it('Submits wildcard searches', async () => {
+  it('Submits wildcard searches as raw text searches', async () => {
     const onSearch = jest.fn();
     eventsMock = MockApiClient.addMockResponse({
       url: `/organizations/${organization.slug}/events/`,
@@ -163,7 +163,7 @@ describe('SearchBar', () => {
 
     expect(screen.queryByTestId('smart-search-dropdown')).not.toBeInTheDocument();
     expect(onSearch).toHaveBeenCalledTimes(1);
-    expect(onSearch).toHaveBeenCalledWith('transaction:client*');
+    expect(onSearch).toHaveBeenCalledWith('client*');
   });
 
   it('closes the search dropdown when clicked outside of', () => {

+ 7 - 6
static/app/components/performance/searchBar.tsx

@@ -116,7 +116,7 @@ function SearchBar(props: SearchBarProps) {
       if (currentItem?.value) {
         handleChooseItem(currentItem.value);
       } else {
-        handleSearch(searchString);
+        handleSearch(searchString, true);
       }
     }
   };
@@ -182,7 +182,7 @@ function SearchBar(props: SearchBarProps) {
 
   const handleChooseItem = (value: string) => {
     const item = decodeValueToItem(value);
-    handleSearch(item.transaction);
+    handleSearch(item.transaction, false);
   };
 
   const handleClickItemIcon = (value: string) => {
@@ -190,10 +190,11 @@ function SearchBar(props: SearchBarProps) {
     navigateToItemTransactionSummary(item);
   };
 
-  const handleSearch = (query: string) => {
+  const handleSearch = (query: string, asRawText: boolean) => {
     setSearchResults([]);
     setSearchString(query);
-    onSearch(query ? `transaction:${query}` : '');
+    const fullQuery = asRawText ? query : `transaction:${query}`;
+    onSearch(query ? fullQuery : '');
     closeDropdown();
   };
 
@@ -254,8 +255,8 @@ interface DataItem {
   'count()'?: number;
 }
 
-const wrapQueryInWildcards = (query: string) => {
-  if (!query.startsWith('* ')) {
+export const wrapQueryInWildcards = (query: string) => {
+  if (!query.startsWith('*')) {
     query = '*' + query;
   }
 

+ 29 - 137
static/app/views/performance/data.tsx

@@ -1,15 +1,11 @@
 import {Location} from 'history';
 
 import {COL_WIDTH_UNDEFINED} from 'sentry/components/gridEditable';
+import {wrapQueryInWildcards} from 'sentry/components/performance/searchBar';
 import {ALL_ACCESS_PROJECTS} from 'sentry/constants/pageFilters';
 import {t} from 'sentry/locale';
 import {NewQuery, Organization, Project, SelectValue} from 'sentry/types';
 import EventView from 'sentry/utils/discover/eventView';
-import {
-  MEPState,
-  METRIC_SEARCH_SETTING_PARAM,
-  METRIC_SETTING_PARAM,
-} from 'sentry/utils/performance/contexts/metricsEnhancedSetting';
 import {WEB_VITAL_DETAILS} from 'sentry/utils/performance/vitals/constants';
 import {decodeScalar} from 'sentry/utils/queryString';
 import {MutableSearch} from 'sentry/utils/tokenizeSearch';
@@ -396,14 +392,28 @@ export function getTermHelp(
   return PERFORMANCE_TERMS[term](organization);
 }
 
-function isUsingLimitedSearch(location: Location, withStaticFilters: boolean) {
-  const {query} = location;
-  const mepSearchState = decodeScalar(query[METRIC_SEARCH_SETTING_PARAM], '');
-  const mepSettingState = decodeScalar(query[METRIC_SETTING_PARAM], ''); // TODO: Can be removed since it's for dev ui only.
-  return (
-    withStaticFilters &&
-    (mepSearchState === MEPState.metricsOnly || mepSettingState === MEPState.metricsOnly)
-  );
+function prepareQueryForLandingPage(searchQuery, withStaticFilters) {
+  const conditions = new MutableSearch(searchQuery);
+
+  // If there is a bare text search, we want to treat it as a search
+  // on the transaction name.
+  if (conditions.freeText.length > 0) {
+    const parsedFreeText = conditions.freeText.join(' ');
+
+    // the query here is a user entered condition, no need to escape it
+    conditions.setFilterValues(
+      'transaction',
+      [wrapQueryInWildcards(parsedFreeText)],
+      false
+    );
+    conditions.freeText = [];
+  }
+  if (withStaticFilters) {
+    conditions.tokens = conditions.tokens.filter(
+      token => token.key && TOKEN_KEYS_SUPPORTED_IN_LIMITED_SEARCH.includes(token.key)
+    );
+  }
+  return conditions.formatString();
 }
 
 function generateGenericPerformanceEventView(
@@ -447,31 +457,7 @@ function generateGenericPerformanceEventView(
   savedQuery.orderby = decodeScalar(query.sort, '-tpm');
 
   const searchQuery = decodeScalar(query.query, '');
-  const conditions = new MutableSearch(searchQuery);
-  const isLimitedSearch = isUsingLimitedSearch(location, withStaticFilters);
-
-  // If there is a bare text search, we want to treat it as a search
-  // on the transaction name.
-  if (conditions.freeText.length > 0) {
-    const parsedFreeText = isLimitedSearch
-      ? decodeScalar(conditions.freeText, '')
-      : conditions.freeText.join(' ');
-
-    if (isLimitedSearch) {
-      // the query here is a user entered condition, no need to escape it
-      conditions.setFilterValues('transaction', [`${parsedFreeText}`], false);
-    } else {
-      // the query here is a user entered condition, no need to escape it
-      conditions.setFilterValues('transaction', [`*${parsedFreeText}*`], false);
-    }
-    conditions.freeText = [];
-  }
-  if (isLimitedSearch) {
-    conditions.tokens = conditions.tokens.filter(
-      token => token.key && TOKEN_KEYS_SUPPORTED_IN_LIMITED_SEARCH.includes(token.key)
-    );
-  }
-  savedQuery.query = conditions.formatString();
+  savedQuery.query = prepareQueryForLandingPage(searchQuery, withStaticFilters);
 
   const eventView = EventView.fromNewQueryWithLocation(savedQuery, location);
   eventView.additionalConditions.addFilterValues('event.type', ['transaction']);
@@ -531,26 +517,7 @@ function generateBackendPerformanceEventView(
   savedQuery.orderby = decodeScalar(query.sort, '-tpm');
 
   const searchQuery = decodeScalar(query.query, '');
-  const conditions = new MutableSearch(searchQuery);
-  const isLimitedSearch = isUsingLimitedSearch(location, withStaticFilters);
-
-  // If there is a bare text search, we want to treat it as a search
-  // on the transaction name.
-  if (conditions.freeText.length > 0) {
-    const parsedFreeText = isLimitedSearch
-      ? decodeScalar(conditions.freeText, '')
-      : conditions.freeText.join(' ');
-
-    if (isLimitedSearch) {
-      // the query here is a user entered condition, no need to escape it
-      conditions.setFilterValues('transaction', [`${parsedFreeText}`], false);
-    } else {
-      // the query here is a user entered condition, no need to escape it
-      conditions.setFilterValues('transaction', [`*${parsedFreeText}*`], false);
-    }
-    conditions.freeText = [];
-  }
-  savedQuery.query = conditions.formatString();
+  savedQuery.query = prepareQueryForLandingPage(searchQuery, withStaticFilters);
 
   const eventView = EventView.fromNewQueryWithLocation(savedQuery, location);
 
@@ -614,27 +581,7 @@ function generateMobilePerformanceEventView(
   savedQuery.orderby = decodeScalar(query.sort, '-tpm');
 
   const searchQuery = decodeScalar(query.query, '');
-  const conditions = new MutableSearch(searchQuery);
-  const isLimitedSearch = isUsingLimitedSearch(location, withStaticFilters);
-
-  // If there is a bare text search, we want to treat it as a search
-  // on the transaction name.
-  if (conditions.freeText.length > 0) {
-    const parsedFreeText = isLimitedSearch
-      ? // pick first element to search transactions by name
-        decodeScalar(conditions.freeText, '')
-      : conditions.freeText.join(' ');
-
-    if (isLimitedSearch) {
-      // the query here is a user entered condition, no need to escape it
-      conditions.setFilterValues('transaction', [`${parsedFreeText}`], false);
-    } else {
-      // the query here is a user entered condition, no need to escape it
-      conditions.setFilterValues('transaction', [`*${parsedFreeText}*`], false);
-    }
-    conditions.freeText = [];
-  }
-  savedQuery.query = conditions.formatString();
+  savedQuery.query = prepareQueryForLandingPage(searchQuery, withStaticFilters);
 
   const eventView = EventView.fromNewQueryWithLocation(savedQuery, location);
 
@@ -684,27 +631,7 @@ function generateFrontendPageloadPerformanceEventView(
   savedQuery.orderby = decodeScalar(query.sort, '-tpm');
 
   const searchQuery = decodeScalar(query.query, '');
-  const conditions = new MutableSearch(searchQuery);
-  const isLimitedSearch = isUsingLimitedSearch(location, withStaticFilters);
-
-  // If there is a bare text search, we want to treat it as a search
-  // on the transaction name.
-  if (conditions.freeText.length > 0) {
-    const parsedFreeText = isLimitedSearch
-      ? // pick first element to search transactions by name
-        decodeScalar(conditions.freeText, '')
-      : conditions.freeText.join(' ');
-
-    if (isLimitedSearch) {
-      // the query here is a user entered condition, no need to escape it
-      conditions.setFilterValues('transaction', [`${parsedFreeText}`], false);
-    } else {
-      // the query here is a user entered condition, no need to escape it
-      conditions.setFilterValues('transaction', [`*${parsedFreeText}*`], false);
-    }
-    conditions.freeText = [];
-  }
-  savedQuery.query = conditions.formatString();
+  savedQuery.query = prepareQueryForLandingPage(searchQuery, withStaticFilters);
 
   const eventView = EventView.fromNewQueryWithLocation(savedQuery, location);
 
@@ -755,28 +682,7 @@ function generateFrontendOtherPerformanceEventView(
   savedQuery.orderby = decodeScalar(query.sort, '-tpm');
 
   const searchQuery = decodeScalar(query.query, '');
-  const conditions = new MutableSearch(searchQuery);
-  const isLimitedSearch = isUsingLimitedSearch(location, withStaticFilters);
-
-  // If there is a bare text search, we want to treat it as a search
-  // on the transaction name.
-  if (conditions.freeText.length > 0 && !isLimitedSearch) {
-    const parsedFreeText = isLimitedSearch
-      ? // pick first element to search transactions by name
-        decodeScalar(conditions.freeText, '')
-      : conditions.freeText.join(' ');
-
-    if (isLimitedSearch) {
-      // the query here is a user entered condition, no need to escape it
-      conditions.setFilterValues('transaction', [`${parsedFreeText}`], false);
-    } else {
-      // the query here is a user entered condition, no need to escape it
-      conditions.setFilterValues('transaction', [`*${parsedFreeText}*`], false);
-    }
-
-    conditions.freeText = [];
-  }
-  savedQuery.query = conditions.formatString();
+  savedQuery.query = prepareQueryForLandingPage(searchQuery, withStaticFilters);
 
   const eventView = EventView.fromNewQueryWithLocation(savedQuery, location);
 
@@ -869,21 +775,7 @@ export function generatePerformanceVitalDetailView(
   savedQuery.orderby = decodeScalar(query.sort, '-count');
 
   const searchQuery = decodeScalar(query.query, '');
-  const conditions = new MutableSearch(searchQuery);
-
-  // If there is a bare text search, we want to treat it as a search
-  // on the transaction name.
-  if (conditions.freeText.length > 0) {
-    // the query here is a user entered condition, no need to escape it
-    conditions.setFilterValues(
-      'transaction',
-      [`*${conditions.freeText.join(' ')}*`],
-      false
-    );
-    conditions.freeText = [];
-  }
-  conditions.setFilterValues('event.type', ['transaction']);
-  savedQuery.query = conditions.formatString();
+  savedQuery.query = prepareQueryForLandingPage(searchQuery, false);
 
   const eventView = EventView.fromNewQueryWithLocation(savedQuery, location);
 

+ 5 - 0
static/app/views/performance/landing/index.tsx

@@ -119,6 +119,11 @@ export function PerformanceLanding(props: Props) {
     if (transactionValues.length) {
       return transactionValues[0];
     }
+    if (conditions.freeText.length > 0) {
+      // raw text query will be wrapped in wildcards in generatePerformanceEventView
+      // so no need to wrap it here
+      return conditions.freeText.join(' ');
+    }
     return '';
   };