Skip to content
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.

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

feat(alerts): use extrapolated data during creation #53428

Merged
merged 32 commits into from
Jul 31, 2023
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
2aad496
feat(alerts): use extrapolated data druing creation
obostjancic Jul 21, 2023
a6324c8
restore vscode settings
obostjancic Jul 24, 2023
1ba1543
:hammer_and_wrench: apply pre-commit fixes
getsantry[bot] Jul 24, 2023
cfaca6a
:hammer_and_wrench: apply pre-commit fixes
getsantry[bot] Jul 24, 2023
daa4b51
remove backend changes
obostjancic Jul 24, 2023
e990bc1
test fixes
obostjancic Jul 25, 2023
1e6981f
add extrapolation styles
obostjancic Jul 25, 2023
16bac5a
added sample rate fetching
obostjancic Jul 26, 2023
214f8b1
wip
obostjancic Jul 27, 2023
4753f2f
use sample rate when extrapolating
obostjancic Jul 27, 2023
deaebdb
type fix
obostjancic Jul 27, 2023
54c8505
margin fix
obostjancic Jul 27, 2023
f80086f
tsc fix
obostjancic Jul 27, 2023
0a9ffdc
Merge branch 'master' into ogi/feat/on-demand-alerts-chart-extrapolation
obostjancic Jul 27, 2023
ccc85e4
minor fixes
obostjancic Jul 27, 2023
f701941
fix extrapolation
obostjancic Jul 27, 2023
5bc317c
Merge branch 'master' into ogi/feat/on-demand-alerts-chart-extrapolation
obostjancic Jul 31, 2023
745a046
Update static/app/views/alerts/rules/metric/ruleConditionsForm.tsx
obostjancic Jul 31, 2023
7320c5b
Update static/app/components/charts/eventsRequest.tsx
obostjancic Jul 31, 2023
7929807
:hammer_and_wrench: apply eslint style fixes
getsantry[bot] Jul 31, 2023
0be17ac
Update static/app/components/charts/onDemandMetricRequest.tsx
obostjancic Jul 31, 2023
4270b5b
Update static/app/views/alerts/rules/metric/triggers/chart/index.tsx
obostjancic Jul 31, 2023
6cc08d4
Update static/app/components/charts/onDemandMetricRequest.tsx
obostjancic Jul 31, 2023
70d5191
:hammer_and_wrench: apply eslint style fixes
getsantry[bot] Jul 31, 2023
9c9e0c0
PR comments
obostjancic Jul 31, 2023
039b3d1
remove span element
obostjancic Jul 31, 2023
0d2e11a
added tests
obostjancic Jul 31, 2023
ca0d341
Merge branch 'master' into ogi/feat/on-demand-alerts-chart-extrapolation
obostjancic Jul 31, 2023
6ed7823
PR comments
obostjancic Jul 31, 2023
d36fc34
add assert
obostjancic Jul 31, 2023
7fc0b2d
test fix
obostjancic Jul 31, 2023
0094fd2
test fix
obostjancic Jul 31, 2023
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
14 changes: 12 additions & 2 deletions static/app/components/charts/eventsRequest.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ type LoadingStatus = {

// Can hold additional data from the root an events stat object (eg. start, end, order, isMetricsData).
interface AdditionalSeriesInfo {
isExtrapolatedData?: boolean;
isMetricsData?: boolean;
}

Expand Down Expand Up @@ -196,6 +197,10 @@ type EventsRequestPartialProps = {
* A unique name for what's triggering this request, see organization_events_stats for an allowlist
*/
referrer?: string;
/**
* Sample rate used for data extrapolation in OnDemandMetricsRequest
*/
sampleRate?: number;
/**
* Should loading be shown.
*/
Expand Down Expand Up @@ -480,7 +485,7 @@ class EventsRequest extends PureComponent<EventsRequestProps, EventsRequestState
}

processData(response: EventsStats, seriesIndex: number = 0, seriesName?: string) {
const {data, isMetricsData, totals, meta} = response;
const {data, isMetricsData, totals, meta, isExtrapolatedData} = response;
const {
includeTransformedData,
includeTimeAggregation,
Expand Down Expand Up @@ -537,6 +542,7 @@ class EventsRequest extends PureComponent<EventsRequestProps, EventsRequestState
previousData,
timeAggregatedData,
timeframe,
isExtrapolatedData,
};

return processedData;
Expand Down Expand Up @@ -639,10 +645,14 @@ class EventsRequest extends PureComponent<EventsRequestProps, EventsRequestState
timeAggregatedData,
timeframe,
isMetricsData,
isExtrapolatedData,
} = this.processData(timeseriesData);

const seriesAdditionalInfo = {
[this.props.currentSeriesNames?.[0] ?? 'current']: {isMetricsData},
[this.props.currentSeriesNames?.[0] ?? 'current']: {
isMetricsData,
isExtrapolatedData,
},
};

return children({
Expand Down
202 changes: 202 additions & 0 deletions static/app/components/charts/onDemandMetricRequest.spec.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,202 @@
import {render, waitFor} from 'sentry-test/reactTestingLibrary';

import {doEventsRequest} from 'sentry/actionCreators/events';
import {OnDemandMetricRequest} from 'sentry/components/charts/onDemandMetricRequest';

const SAMPLE_RATE = 0.5;

const COUNT_OBJ = {
count: 123,
};

const COUNT_OBJ_SCALED = {
count: COUNT_OBJ.count / SAMPLE_RATE,
};

jest.mock('sentry/actionCreators/events', () => ({
doEventsRequest: jest.fn(),
}));

describe('OnDemandMetricRequest', function () {
const organization = TestStubs.Organization();
const mock = jest.fn(() => null);

const DEFAULTS = {
api: new MockApiClient(),
period: '24h',
organization,
includePrevious: false,
interval: '24h',
limit: 30,
query: 'transaction.duration:>1',
children: () => null,
partial: false,
includeTransformedData: true,
sampleRate: SAMPLE_RATE,
};

describe('with props changes', function () {
beforeAll(function () {
(doEventsRequest as jest.Mock).mockImplementation(() =>
Promise.resolve({
isMetricsData: true,
data: [],
})
);
});

it('makes requests', async function () {
render(<OnDemandMetricRequest {...DEFAULTS}>{mock}</OnDemandMetricRequest>);
expect(mock).toHaveBeenNthCalledWith(
1,
expect.objectContaining({
loading: true,
})
);

await waitFor(() =>
expect(mock).toHaveBeenLastCalledWith(
expect.objectContaining({
loading: false,
seriesAdditionalInfo: {
current: {
isExtrapolatedData: true,
isMetricsData: false,
},
},
timeseriesData: [
{
seriesName: expect.anything(),
data: [],
},
],
originalTimeseriesData: [],
})
)
);

expect(doEventsRequest).toHaveBeenCalledTimes(2);
});

it('makes a new request if projects prop changes', function () {
const {rerender} = render(
<OnDemandMetricRequest {...DEFAULTS}>{mock}</OnDemandMetricRequest>
);
(doEventsRequest as jest.Mock).mockClear();

rerender(
<OnDemandMetricRequest {...DEFAULTS} project={[123]}>
{mock}
</OnDemandMetricRequest>
);
expect(doEventsRequest).toHaveBeenCalledWith(
obostjancic marked this conversation as resolved.
Show resolved Hide resolved
expect.anything(),
expect.objectContaining({
project: [123],
useOnDemandMetrics: true,
})
);
});

it('makes a new request if environments prop changes', function () {
const {rerender} = render(
<OnDemandMetricRequest {...DEFAULTS}>{mock}</OnDemandMetricRequest>
);
(doEventsRequest as jest.Mock).mockClear();

rerender(
<OnDemandMetricRequest {...DEFAULTS} environment={['dev']}>
{mock}
</OnDemandMetricRequest>
);

expect(doEventsRequest).toHaveBeenCalledTimes(1);
obostjancic marked this conversation as resolved.
Show resolved Hide resolved
expect(doEventsRequest).toHaveBeenCalledWith(
expect.anything(),
expect.objectContaining({
environment: ['dev'],
useOnDemandMetrics: true,
})
);
});

it('makes a new request if period prop changes', function () {
const {rerender} = render(
<OnDemandMetricRequest {...DEFAULTS}>{mock}</OnDemandMetricRequest>
);
(doEventsRequest as jest.Mock).mockClear();

rerender(
<OnDemandMetricRequest {...DEFAULTS} period="7d">
{mock}
</OnDemandMetricRequest>
);

expect(doEventsRequest).toHaveBeenCalledWith(
Copy link
Member

Choose a reason for hiding this comment

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

done we need to check for the number of calls here too? expect(doEventsRequest).toHaveBeenCalledTimes(2);

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, we can, added in d36fc34

expect.anything(),
expect.objectContaining({
period: '7d',
useOnDemandMetrics: true,
})
);
});
});

describe('transforms', function () {
beforeEach(function () {
(doEventsRequest as jest.Mock).mockClear();
});

it('applies sample rate to indexed if there is no metrics data`', async function () {
(doEventsRequest as jest.Mock)
.mockImplementation(() =>
Promise.resolve({
isMetricsData: true,
data: [],
})
)
.mockImplementation(() =>
Promise.resolve({
isMetricsData: false,
data: [[new Date(), [COUNT_OBJ]]],
})
);

render(<OnDemandMetricRequest {...DEFAULTS}>{mock}</OnDemandMetricRequest>);

expect(mock).toHaveBeenNthCalledWith(
1,
expect.objectContaining({
loading: true,
})
);

await waitFor(() =>
expect(mock).toHaveBeenLastCalledWith(
expect.objectContaining({
loading: false,
seriesAdditionalInfo: {
current: {
isExtrapolatedData: true,
isMetricsData: false,
},
},
timeseriesData: [
{
seriesName: expect.anything(),
data: [
expect.objectContaining({
name: expect.any(Number),
value: COUNT_OBJ_SCALED.count,
}),
],
},
],
})
)
);

expect(doEventsRequest).toHaveBeenCalledTimes(2);
});
});
});
121 changes: 121 additions & 0 deletions static/app/components/charts/onDemandMetricRequest.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
import {doEventsRequest} from 'sentry/actionCreators/events';
import {addErrorMessage} from 'sentry/actionCreators/indicator';
import EventsRequest from 'sentry/components/charts/eventsRequest';
import {t} from 'sentry/locale';
import {EventsStats, MultiSeriesEventsStats} from 'sentry/types';
import {DiscoverDatasets} from 'sentry/utils/discover/types';

function numOfEvents(timeseriesData) {
return timeseriesData.data.reduce((acc, item) => {
const count = item[1][0].count;
return acc + count;
}, 0);
}

function applySampleRate(timeseriesData, sampleRate = 1) {
const scaledData = timeseriesData.data.map(([timestamp, value]) => {
return [timestamp, value.map(item => ({count: Math.round(item.count / sampleRate)}))];
});

return {
...timeseriesData,
isExtrapolatedData: true,
isMetricsData: false,
data: scaledData,
};
}

export class OnDemandMetricRequest extends EventsRequest {
fetchMetricsData = async () => {
const {api, ...props} = this.props;

try {
const timeseriesData = await doEventsRequest(api, {
...props,
useOnDemandMetrics: true,
});

return timeseriesData;
} catch {
return {
data: [],
isMetricsData: false,
};
}
};

fetchIndexedData = async () => {
const {api, sampleRate, ...props} = this.props;

const timeseriesData = await doEventsRequest(api, {
...props,
useOnDemandMetrics: false,
queryExtras: {dataset: DiscoverDatasets.DISCOVER},
});

return applySampleRate(timeseriesData, sampleRate);
};

fetchData = async () => {
const {api, confirmedQuery, onError, expired, name, hideError, ...props} = this.props;
let timeseriesData: EventsStats | MultiSeriesEventsStats | null = null;

if (confirmedQuery === false) {
return;
}
this.setState(state => ({
reloading: state.timeseriesData !== null,
errored: false,
errorMessage: undefined,
}));
let errorMessage;
if (expired) {
errorMessage = t(
'%s has an invalid date range. Please try a more recent date range.',
name
);
addErrorMessage(errorMessage, {append: true});
this.setState({
errored: true,
errorMessage,
});
} else {
try {
api.clear();

timeseriesData = await this.fetchMetricsData();

const fallbackToIndexed =
!timeseriesData.isMetricsData || numOfEvents(timeseriesData) === 0;

if (fallbackToIndexed) {
timeseriesData = await this.fetchIndexedData();
}
} catch (resp) {
if (resp && resp.responseJSON && resp.responseJSON.detail) {
errorMessage = resp.responseJSON.detail;
} else {
errorMessage = t('Error loading chart data');
}
if (!hideError) {
addErrorMessage(errorMessage);
}
onError?.(errorMessage);
this.setState({
errored: true,
errorMessage,
});
}

this.setState({
reloading: false,
timeseriesData,
fetchedWithPrevious: props.includePrevious,
});
}

if (props.dataLoadedCallback) {
props.dataLoadedCallback(timeseriesData);
}
};
}
9 changes: 7 additions & 2 deletions static/app/types/echarts.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
import type {AxisPointerComponentOption, ECharts, LineSeriesOption} from 'echarts';
import type {
AxisPointerComponentOption,
ECharts,
LineSeriesOption,
PatternObject,
} from 'echarts';
import type ReactEchartsCore from 'echarts-for-react/lib/core';

export type SeriesDataUnit = {
Expand All @@ -15,7 +20,7 @@ export type Series = {
data: SeriesDataUnit[];
seriesName: string;
areaStyle?: {
color: string;
color: string | PatternObject;
opacity: number;
};
color?: string;
Expand Down
Loading
Loading