Skip to content

Commit fc5f7a8

Browse files
Zylphrexandrewshie-sentry
authored andcommitted
feat(explore): Remove group by timestamp from explore (#92546)
Grouping by timestamp produces a high cardinality result that is not very useful so disallow it in the UI.
1 parent b4beb8e commit fc5f7a8

File tree

2 files changed

+27
-25
lines changed

2 files changed

+27
-25
lines changed

static/app/views/explore/hooks/useGroupByFields.tsx

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,14 +16,17 @@ interface UseGroupByFieldsProps {
1616
*/
1717
groupBys: string[];
1818
tags: TagCollection;
19-
prefix?: string;
19+
hideEmptyOption?: boolean;
2020
}
2121

22-
export function useGroupByFields({tags, groupBys}: UseGroupByFieldsProps) {
22+
export function useGroupByFields({
23+
tags,
24+
groupBys,
25+
hideEmptyOption,
26+
}: UseGroupByFieldsProps) {
2327
const options: Array<SelectOption<string>> = useMemo(() => {
2428
const potentialOptions = [
25-
// We do not support grouping by span id, we have a dedicated sample mode for that
26-
...Object.keys(tags).filter(key => key !== 'id'),
29+
...Object.keys(tags).filter(key => !DISALLOWED_GROUP_BY_FIELDS.has(key)),
2730

2831
// These options aren't known to exist on this project but it was inserted into
2932
// the group bys somehow so it should be a valid options in the group bys.
@@ -36,11 +39,15 @@ export function useGroupByFields({tags, groupBys}: UseGroupByFieldsProps) {
3639

3740
return [
3841
// hard code in an empty option
39-
{
40-
label: <Disabled>{t('\u2014')}</Disabled>,
41-
value: UNGROUPED,
42-
textValue: t('\u2014'),
43-
},
42+
...(hideEmptyOption
43+
? []
44+
: [
45+
{
46+
label: <Disabled>{t('\u2014')}</Disabled>,
47+
value: UNGROUPED,
48+
textValue: t('\u2014'),
49+
},
50+
]),
4451
...potentialOptions.map(key => {
4552
const kind = FieldKind.TAG;
4653
return {
@@ -53,11 +60,15 @@ export function useGroupByFields({tags, groupBys}: UseGroupByFieldsProps) {
5360
};
5461
}),
5562
];
56-
}, [tags, groupBys]);
63+
}, [tags, groupBys, hideEmptyOption]);
5764

5865
return options;
5966
}
6067

68+
// Some fields don't make sense to allow users to group by as they create
69+
// very high cardinality groupings and is not useful.
70+
const DISALLOWED_GROUP_BY_FIELDS = new Set(['id', 'timestamp']);
71+
6172
const Disabled = styled('span')`
6273
color: ${p => p.theme.subText};
6374
`;

static/app/views/explore/multiQueryMode/queryConstructors/groupBy.tsx

Lines changed: 6 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
1-
import {useMemo} from 'react';
21
import styled from '@emotion/styled';
32

43
import {CompactSelect, type SelectOption} from 'sentry/components/core/compactSelect';
54
import {Tooltip} from 'sentry/components/core/tooltip';
65
import {t} from 'sentry/locale';
76
import {useSpanTags} from 'sentry/views/explore/contexts/spanTagsContext';
7+
import {useGroupByFields} from 'sentry/views/explore/hooks/useGroupByFields';
88
import {
99
type ReadableExploreQueryParts,
1010
useUpdateQueryAtIndex,
@@ -22,20 +22,11 @@ export function GroupBySection({query, index}: Props) {
2222

2323
const updateGroupBys = useUpdateQueryAtIndex(index);
2424

25-
const enabledOptions: Array<SelectOption<string>> = useMemo(() => {
26-
const potentialOptions = Object.keys(tags).filter(key => key !== 'id');
27-
potentialOptions.sort((a, b) => {
28-
if (query.groupBys.includes(a) === query.groupBys.includes(b)) {
29-
return a.localeCompare(b);
30-
}
31-
if (query.groupBys.includes(a)) {
32-
return -1;
33-
}
34-
return 1;
35-
});
36-
37-
return potentialOptions.map(key => ({label: key, value: key, textValue: key}));
38-
}, [tags, query.groupBys]);
25+
const enabledOptions: Array<SelectOption<string>> = useGroupByFields({
26+
groupBys: [],
27+
tags,
28+
hideEmptyOption: true,
29+
});
3930

4031
return (
4132
<Section data-test-id={`section-group-by-${index}`}>

0 commit comments

Comments
 (0)