Skip to content

Commit

Permalink
feat(widget-builder): Allow other functions in sort for spans (#83505)
Browse files Browse the repository at this point in the history
A user can add a new series to the explore UI that sorts on a different
field than it's showing. This allows us to transfer the query over to
the dashboard by applying the sorting to the fields and then allowing
the sort by selectors to show that option (i.e. unlike tables that need
you to choose a column that's represented)
  • Loading branch information
narsaynorath authored Jan 15, 2025
1 parent 1353939 commit dca0f48
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 14 deletions.
18 changes: 15 additions & 3 deletions static/app/views/dashboards/datasetConfig/spans.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
} from 'sentry/views/dashboards/datasetConfig/base';
import {
getTableSortOptions,
getTimeseriesSortOptions,
transformEventsResponseToSeries,
transformEventsResponseToTable,
} from 'sentry/views/dashboards/datasetConfig/errorsAndTransactions';
Expand Down Expand Up @@ -73,9 +74,6 @@ const EAP_AGGREGATIONS = ALLOWED_EXPLORE_VISUALIZE_AGGREGATES.reduce((acc, aggre
return acc;
}, {});

// getTimeseriesSortOptions is undefined because we want to restrict the
// sort options to the same behaviour as tables. i.e. we are only able
// to sort by fields that have already been selected
export const SpansConfig: DatasetConfig<
EventsStats | MultiSeriesEventsStats,
TableData | EventsTableData
Expand All @@ -86,8 +84,11 @@ export const SpansConfig: DatasetConfig<
SearchBar: SpansSearchBar,
filterYAxisAggregateParams: () => filterAggregateParams,
filterYAxisOptions,
filterSeriesSortOptions,
getTableFieldOptions: getPrimaryFieldOptions,
getTableSortOptions,
getTimeseriesSortOptions: (organization, widgetQuery, tags) =>
getTimeseriesSortOptions(organization, widgetQuery, tags, getPrimaryFieldOptions),
getGroupByFieldOptions,
handleOrderByReset,
supportedDisplayTypes: [
Expand Down Expand Up @@ -281,3 +282,14 @@ function getSeriesRequest(

return doEventsRequest<true>(api, requestData);
}

// Filters the primary options in the sort by selector
export function filterSeriesSortOptions(columns: Set<string>) {
return (option: FieldValueOption) => {
if (option.value.kind === FieldValueKind.FUNCTION) {
return true;
}

return columns.has(option.value.meta.name);
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ export function SortBySelectors({
disableSortDirection,
widgetQuery,
displayType,
tags,
}: Props) {
const datasetConfig = getDatasetConfig(widgetType);
const organization = useOrganization();
Expand Down Expand Up @@ -103,7 +104,7 @@ export function SortBySelectors({
title={disableSortReason}
disabled={!disableSort || (disableSortDirection && disableSort)}
>
{displayType === DisplayType.TABLE || widgetType === WidgetType.SPANS ? (
{displayType === DisplayType.TABLE ? (
<SelectControl
name="sortBy"
aria-label={t('Sort by')}
Expand Down Expand Up @@ -135,7 +136,8 @@ export function SortBySelectors({
}
fieldOptions={datasetConfig.getTimeseriesSortOptions!(
organization,
widgetQuery
widgetQuery,
tags
)}
filterPrimaryOptions={
datasetConfig.filterSeriesSortOptions
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -937,7 +937,7 @@ describe('WidgetBuilder', function () {
});

describe('spans dataset timeseries', function () {
it('returns only the selected aggregates and group by as options', async function () {
it('returns group by and aggregate as primary options', async function () {
const widget: Widget = {
id: '1',
title: 'Test Widget',
Expand Down Expand Up @@ -967,16 +967,16 @@ describe('WidgetBuilder', function () {
});

await screen.findByText('Sort by a y-axis');
await selectEvent.openMenu(await screen.findByText('count(span.duration)'));
await selectEvent.openMenu(screen.getAllByText('count(\u2026)')[1]!);

// 3 options in the dropdown
expect(screen.queryAllByTestId('menu-list-item-label')).toHaveLength(3);
// 12 options in the dropdown
expect(screen.queryAllByTestId('menu-list-item-label')).toHaveLength(12);

// Appears once in the dropdown and once in the sort by field
expect(await screen.findAllByText('count(span.duration)')).toHaveLength(2);
// Appears once in the y-axis section, dropdown, and in the sort by field
expect(await screen.findAllByText('count(\u2026)')).toHaveLength(3);

// Appears once in the dropdown
expect(await screen.findAllByText('avg(span.duration)')).toHaveLength(1);
// Appears once in the y-axis section and in the dropdown
expect(await screen.findAllByText('avg(\u2026)')).toHaveLength(2);

// Appears once in the dropdown and once in the group by field
expect(await screen.findAllByText('transaction')).toHaveLength(2);
Expand Down
4 changes: 3 additions & 1 deletion static/app/views/explore/hooks/useAddToDashboard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,9 @@ export function useAddToDashboard() {
fields = sampleFields.filter(Boolean);
}
} else {
fields = [...groupBys, ...yAxes].filter(Boolean);
fields = [
...new Set([...groupBys, ...yAxes, ...sortBys.map(sort => sort.field)]),
].filter(Boolean);
}

const search = new MutableSearch(query);
Expand Down

0 comments on commit dca0f48

Please sign in to comment.