Skip to content

chore(comp workflows): Hint for Distributions #102761

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 5, 2025
Merged

chore(comp workflows): Hint for Distributions #102761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d12c906
tooltip name
aayush-se Nov 5, 2025
251b04c
hint for cohorts
aayush-se Nov 5, 2025
1bd0396
am/pm and utc indication
aayush-se Nov 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 75 additions & 1 deletion static/app/views/explore/components/attributeBreakdowns/content.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {Fragment, useEffect, useMemo, useState} from 'react';
import {useTheme} from '@emotion/react';
import styled from '@emotion/styled';
import moment from 'moment-timezone';

import emptyTraceImg from 'sentry-images/spot/performance-empty-trace.svg';

Expand All @@ -17,18 +18,21 @@ import {IconChevron} from 'sentry/icons/iconChevron';
import {IconMegaphone} from 'sentry/icons/iconMegaphone';
import {t} from 'sentry/locale';
import {space} from 'sentry/styles/space';
import {getUserTimezone} from 'sentry/utils/dates';
import {useDebouncedValue} from 'sentry/utils/useDebouncedValue';
import {useFeedbackForm} from 'sentry/utils/useFeedbackForm';
import type {ChartInfo} from 'sentry/views/explore/components/chart/types';
import useAttributeBreakdowns from 'sentry/views/explore/hooks/useAttributeBreakdowns';
import type {BoxSelectOptions} from 'sentry/views/explore/hooks/useChartBoxSelect';
import {prettifyAggregation} from 'sentry/views/explore/utils';

import {Chart} from './chart';
import {useChartSelection} from './chartSelectionContext';
import {SortingToggle, type SortingMethod} from './sortingToggle';

const CHARTS_COLUMN_COUNT = 3;
const CHARTS_PER_PAGE = CHARTS_COLUMN_COUNT * 4;
const PERCENTILE_FUNCTION_PREFIXES = ['p50', 'p75', 'p90', 'p95', 'p99', 'avg'];
Copy link
Contributor

@cursor cursor bot Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Avg erroneously treated as percentile indicator bug

The constant PERCENTILE_FUNCTION_PREFIXES incorrectly includes 'avg' as a percentile function. Average (avg) is a mean calculation, not a percentile function. This causes the selection hint to incorrectly show "and is greater than or equal to avg(...)" for average functions, when it should only show the time range without the threshold clause. Only actual percentile functions (p50, p75, p90, p95, p99) should include the "greater than or equal to" language. This is a known issue that was previously fixed in #72046 ("Don't mark avg as a percentile").

Fix in Cursor Fix in Web


function FeedbackButton() {
const openForm = useFeedbackForm();
Expand Down Expand Up @@ -70,7 +74,7 @@ function EmptyState() {
</Flex>
<Text>
{t(
"Drag to select an area on the chart and click 'Find Attribute Breakdowns' to analyze differences between selected and unselected (baseline) data. Attributes that differ most in frequency appear first, making it easier to identify key differences:"
"Drag to select an area on the chart and click 'Compare Attribute Breakdowns' to analyze differences between selected and unselected (baseline) data. Attributes that differ most in frequency appear first, making it easier to identify key differences:"
)}
</Text>
<IllustrationWrapper>
Expand Down Expand Up @@ -133,6 +137,44 @@ function ContentImpl({
setPage(0);
}, [filteredRankedAttributes]);

const selectionHint = useMemo(() => {
if (!boxSelectOptions.xRange) {
return null;
}

const [x1, x2] = boxSelectOptions.xRange;

let startTimestamp = Math.floor(x1 / 60_000) * 60_000;
const endTimestamp = Math.ceil(x2 / 60_000) * 60_000;
startTimestamp = Math.min(startTimestamp, endTimestamp - 60_000);
Comment on lines +145 to +149
Copy link

@sentry sentry bot Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: boxSelectOptions.xRange is treated as timestamps but contains categorical axis positions, leading to incorrect date formatting.
Severity: CRITICAL | Confidence: 1.00

🔍 Detailed Analysis

The code at static/app/views/explore/components/attributeBreakdowns/content.tsx:144-151 incorrectly assumes boxSelectOptions.xRange contains timestamp values in milliseconds. However, xRange actually holds categorical axis positions (indices) from the chart's x-axis, which is of type 'category'. This leads to x1 and x2 being small numbers (e.g., 0-40). When these small numbers are divided by 60_000 and then passed to moment(), they result in epoch dates like "Jan 1 1970 00:00" being displayed in the selection hint UI.

💡 Suggested Fix

Modify the logic at static/app/views/explore/components/attributeBreakdowns/content.tsx:144-151 to correctly interpret boxSelectOptions.xRange as categorical indices, not timestamps, when formatting the selection hint.

🤖 Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: static/app/views/explore/components/attributeBreakdowns/content.tsx#L144-L148

Potential issue: The code at
`static/app/views/explore/components/attributeBreakdowns/content.tsx:144-151`
incorrectly assumes `boxSelectOptions.xRange` contains timestamp values in milliseconds.
However, `xRange` actually holds categorical axis positions (indices) from the chart's
x-axis, which is of type 'category'. This leads to `x1` and `x2` being small numbers
(e.g., 0-40). When these small numbers are divided by `60_000` and then passed to
`moment()`, they result in epoch dates like "Jan 1 1970 00:00" being displayed in the
selection hint UI.

Did we get this right? 👍 / 👎 to inform future reviews.


const userTimezone = getUserTimezone() || moment.tz.guess();
const startDate = moment
.tz(startTimestamp, userTimezone)
.format('MMM D YYYY h:mm A z');
const endDate = moment.tz(endTimestamp, userTimezone).format('MMM D YYYY h:mm A z');

// Check if yAxis is a percentile function (only these functions should include "and is greater than or equal to")
const yAxisLower = chartInfo.yAxis.toLowerCase();
const isPercentileFunction = PERCENTILE_FUNCTION_PREFIXES.some(prefix =>
yAxisLower.startsWith(prefix)
);

const formattedFunction = prettifyAggregation(chartInfo.yAxis) ?? chartInfo.yAxis;

return {
selection: isPercentileFunction
? t(
`Selection is data between %s - %s and is greater than or equal to %s`,
startDate,
endDate,
formattedFunction
)
Copy link
Contributor

@cursor cursor bot Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Percentile Selection Hint Lacks Numeric Thresholds

The selection hint for percentile functions displays the function name (e.g., "p50") instead of an actual value threshold. The message "Selection is data between %s - %s and is greater than or equal to %s" uses formattedFunction (the function name like "p50(span.duration)") as the third parameter, but this doesn't convey meaningful information to users. The hint should either display an actual numeric threshold value from the y-axis selection, or the boxSelectOptions should capture and provide yRange data to make this message accurate. Currently, boxSelectOptions only captures xRange (time) but not the y-axis value threshold needed for percentile comparisons.

Fix in Cursor Fix in Web

Copy link
Member Author

@aayush-se aayush-se Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid (and im surprised bugbot suggests things like this), but lets get feedback first and see

: t(`Selection is data between %s - %s`, startDate, endDate),
baseline: t('Baseline is all other spans from your query'),
};
}, [boxSelectOptions.xRange, chartInfo.yAxis]);

return (
<Flex direction="column" gap="xl" padding="xl">
{isLoading ? (
Expand All @@ -152,6 +194,14 @@ function ContentImpl({
/>
<SortingToggle value={sortingMethod} onChange={setSortingMethod} />
</ControlsContainer>
{selectionHint && (
<SelectionHintContainer>
<SelectionHint color={theme.chart.getColorPalette(0)?.[0]}>
{selectionHint.selection}
</SelectionHint>
<SelectionHint color="#A29FAA">{selectionHint.baseline}</SelectionHint>
</SelectionHintContainer>
)}
{filteredRankedAttributes.length > 0 ? (
<Fragment>
<ChartsGrid>
Expand Down Expand Up @@ -269,3 +319,27 @@ const PaginationContainer = styled('div')`
justify-content: end;
align-items: center;
`;

const SelectionHintContainer = styled('div')`
display: flex;
flex-direction: column;
gap: ${space(0.5)};
margin-bottom: ${space(1)};
`;

const SelectionHint = styled(Text)<{color?: string}>`
display: flex;
align-items: center;
color: ${p => p.theme.subText};
font-size: ${p => p.theme.fontSize.sm};

&::before {
content: '';
width: 8px;
height: 8px;
border-radius: 50%;
background-color: ${p => p.color || p.theme.gray400};
margin-right: ${space(0.5)};
flex-shrink: 0;
}
`;
2 changes: 1 addition & 1 deletion static/app/views/explore/components/attributeBreakdowns/floatingTrigger.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ export function FloatingTrigger({
<List>
<ListItem onClick={handleZoomIn}>{t('Zoom in')}</ListItem>
<ListItem onClick={handleFindAttributeBreakdowns}>
{t('Find Attribute Breakdowns')}
{t('Compare Attribute Breakdowns')}
</ListItem>
</List>
</div>,
Expand Down
Loading