From f94fb765b5b15496acad28b29bd1d6784cec5aec Mon Sep 17 00:00:00 2001 From: Alexander Weaver Date: Wed, 28 Jun 2023 13:32:28 -0500 Subject: [PATCH] Alerting: Add limit query parameter to Loki-based ASH api, drop default limit from 5000 to 1000, extend visible time range for new ASH UI (#70769) * Add limit query parameter * Drop copy paste comment * Extend history query limit to 30 days and 250 entries * Fix history log entries ordering * Update no history message, add empty history test --------- Co-authored-by: Konrad Lalik --- pkg/services/ngalert/api/api_ruler_history.go | 2 + pkg/services/ngalert/models/history.go | 1 + pkg/services/ngalert/state/historian/loki.go | 4 +- .../ngalert/state/historian/loki_http.go | 13 +++- .../ngalert/state/historian/loki_http_test.go | 64 ++++++++++++++++++- .../alerting/unified/api/stateHistoryApi.ts | 8 +-- .../rules/state-history/LogRecordViewer.tsx | 28 ++++---- .../state-history/LokiStateHistory.test.tsx | 15 +++++ .../rules/state-history/LokiStateHistory.tsx | 37 +++++++++-- 9 files changed, 142 insertions(+), 30 deletions(-) diff --git a/pkg/services/ngalert/api/api_ruler_history.go b/pkg/services/ngalert/api/api_ruler_history.go index 8a54bb72cbe..f9aca50c7c1 100644 --- a/pkg/services/ngalert/api/api_ruler_history.go +++ b/pkg/services/ngalert/api/api_ruler_history.go @@ -27,6 +27,7 @@ const labelQueryPrefix = "labels_" func (srv *HistorySrv) RouteQueryStateHistory(c *contextmodel.ReqContext) response.Response { from := c.QueryInt64("from") to := c.QueryInt64("to") + limit := c.QueryInt("limit") ruleUID := c.Query("ruleUID") labels := make(map[string]string) @@ -42,6 +43,7 @@ func (srv *HistorySrv) RouteQueryStateHistory(c *contextmodel.ReqContext) respon SignedInUser: c.SignedInUser, From: time.Unix(from, 0), To: time.Unix(to, 0), + Limit: limit, Labels: labels, } frame, err := srv.hist.Query(c.Req.Context(), query) diff --git a/pkg/services/ngalert/models/history.go b/pkg/services/ngalert/models/history.go index ed390d8f736..3bdfe4b993d 100644 --- a/pkg/services/ngalert/models/history.go +++ b/pkg/services/ngalert/models/history.go @@ -13,5 +13,6 @@ type HistoryQuery struct { Labels map[string]string From time.Time To time.Time + Limit int SignedInUser *user.SignedInUser } diff --git a/pkg/services/ngalert/state/historian/loki.go b/pkg/services/ngalert/state/historian/loki.go index 2f12155e195..0ab4d88c221 100644 --- a/pkg/services/ngalert/state/historian/loki.go +++ b/pkg/services/ngalert/state/historian/loki.go @@ -43,7 +43,7 @@ const defaultQueryRange = 6 * time.Hour type remoteLokiClient interface { ping(context.Context) error push(context.Context, []stream) error - rangeQuery(ctx context.Context, logQL string, start, end int64) (queryRes, error) + rangeQuery(ctx context.Context, logQL string, start, end, limit int64) (queryRes, error) } // RemoteLokibackend is a state.Historian that records state history to an external Loki instance. @@ -126,7 +126,7 @@ func (h *RemoteLokiBackend) Query(ctx context.Context, query models.HistoryQuery } // Timestamps are expected in RFC3339Nano. - res, err := h.client.rangeQuery(ctx, logQL, query.From.UnixNano(), query.To.UnixNano()) + res, err := h.client.rangeQuery(ctx, logQL, query.From.UnixNano(), query.To.UnixNano(), int64(query.Limit)) if err != nil { return nil, err } diff --git a/pkg/services/ngalert/state/historian/loki_http.go b/pkg/services/ngalert/state/historian/loki_http.go index 003f9ae05d3..399707d541f 100644 --- a/pkg/services/ngalert/state/historian/loki_http.go +++ b/pkg/services/ngalert/state/historian/loki_http.go @@ -17,7 +17,8 @@ import ( "github.com/weaveworks/common/http/client" ) -const defaultPageSize = 5000 +const defaultPageSize = 1000 +const maximumPageSize = 5000 func NewRequester() client.Requester { return &http.Client{} @@ -225,11 +226,17 @@ func (c *httpLokiClient) setAuthAndTenantHeaders(req *http.Request) { } } -func (c *httpLokiClient) rangeQuery(ctx context.Context, logQL string, start, end int64) (queryRes, error) { +func (c *httpLokiClient) rangeQuery(ctx context.Context, logQL string, start, end, limit int64) (queryRes, error) { // Run the pre-flight checks for the query. if start > end { return queryRes{}, fmt.Errorf("start time cannot be after end time") } + if limit < 1 { + limit = defaultPageSize + } + if limit > maximumPageSize { + limit = maximumPageSize + } queryURL := c.cfg.ReadPathURL.JoinPath("/loki/api/v1/query_range") @@ -237,7 +244,7 @@ func (c *httpLokiClient) rangeQuery(ctx context.Context, logQL string, start, en values.Set("query", logQL) values.Set("start", fmt.Sprintf("%d", start)) values.Set("end", fmt.Sprintf("%d", end)) - values.Set("limit", fmt.Sprintf("%d", defaultPageSize)) + values.Set("limit", fmt.Sprintf("%d", limit)) queryURL.RawQuery = values.Encode() diff --git a/pkg/services/ngalert/state/historian/loki_http_test.go b/pkg/services/ngalert/state/historian/loki_http_test.go index 0f1c6fe7f90..d47313e5c46 100644 --- a/pkg/services/ngalert/state/historian/loki_http_test.go +++ b/pkg/services/ngalert/state/historian/loki_http_test.go @@ -145,13 +145,73 @@ func TestLokiHTTPClient(t *testing.T) { now := time.Now().UTC().UnixNano() q := `{from="state-history"}` - _, err := client.rangeQuery(context.Background(), q, now-100, now) + _, err := client.rangeQuery(context.Background(), q, now-100, now, 1100) + + require.NoError(t, err) + params := req.lastRequest.URL.Query() + require.True(t, params.Has("limit"), "query params did not contain 'limit': %#v", params) + require.Equal(t, fmt.Sprint(1100), params.Get("limit")) + }) + + t.Run("uses default page size if limit not provided", func(t *testing.T) { + req := NewFakeRequester().WithResponse(&http.Response{ + Status: "200 OK", + StatusCode: 200, + Body: io.NopCloser(bytes.NewBufferString(`{}`)), + ContentLength: int64(0), + Header: make(http.Header, 0), + }) + client := createTestLokiClient(req) + now := time.Now().UTC().UnixNano() + q := `{from="state-history"}` + + _, err := client.rangeQuery(context.Background(), q, now-100, now, 0) + + require.NoError(t, err) + params := req.lastRequest.URL.Query() + require.True(t, params.Has("limit"), "query params did not contain 'limit': %#v", params) + require.Equal(t, fmt.Sprint(defaultPageSize), params.Get("limit")) + }) + + t.Run("uses default page size if limit invalid", func(t *testing.T) { + req := NewFakeRequester().WithResponse(&http.Response{ + Status: "200 OK", + StatusCode: 200, + Body: io.NopCloser(bytes.NewBufferString(`{}`)), + ContentLength: int64(0), + Header: make(http.Header, 0), + }) + client := createTestLokiClient(req) + now := time.Now().UTC().UnixNano() + q := `{from="state-history"}` + + _, err := client.rangeQuery(context.Background(), q, now-100, now, -100) require.NoError(t, err) params := req.lastRequest.URL.Query() require.True(t, params.Has("limit"), "query params did not contain 'limit': %#v", params) require.Equal(t, fmt.Sprint(defaultPageSize), params.Get("limit")) }) + + t.Run("uses maximum page size if limit too big", func(t *testing.T) { + req := NewFakeRequester().WithResponse(&http.Response{ + Status: "200 OK", + StatusCode: 200, + Body: io.NopCloser(bytes.NewBufferString(`{}`)), + ContentLength: int64(0), + Header: make(http.Header, 0), + }) + client := createTestLokiClient(req) + now := time.Now().UTC().UnixNano() + q := `{from="state-history"}` + + _, err := client.rangeQuery(context.Background(), q, now-100, now, maximumPageSize+1000) + + require.NoError(t, err) + params := req.lastRequest.URL.Query() + require.True(t, params.Has("limit"), "query params did not contain 'limit': %#v", params) + require.Equal(t, fmt.Sprint(maximumPageSize), params.Get("limit")) + }) }) } @@ -208,7 +268,7 @@ func TestLokiHTTPClient_Manual(t *testing.T) { end := time.Now().UnixNano() // Authorized request should not fail against Grafana Cloud. - res, err := client.rangeQuery(context.Background(), logQL, start, end) + res, err := client.rangeQuery(context.Background(), logQL, start, end, defaultPageSize) require.NoError(t, err) require.NotNil(t, res) }) diff --git a/public/app/features/alerting/unified/api/stateHistoryApi.ts b/public/app/features/alerting/unified/api/stateHistoryApi.ts index 1374595da4b..3a78bcd8425 100644 --- a/public/app/features/alerting/unified/api/stateHistoryApi.ts +++ b/public/app/features/alerting/unified/api/stateHistoryApi.ts @@ -1,15 +1,13 @@ -import { getUnixTime } from 'date-fns'; - import { DataFrameJSON } from '@grafana/data'; import { alertingApi } from './alertingApi'; export const stateHistoryApi = alertingApi.injectEndpoints({ endpoints: (build) => ({ - getRuleHistory: build.query({ - query: ({ ruleUid, from, to = getUnixTime(new Date()) }) => ({ + getRuleHistory: build.query({ + query: ({ ruleUid, from, to, limit = 100 }) => ({ url: '/api/v1/rules/history', - params: { ruleUID: ruleUid, from, to }, + params: { ruleUID: ruleUid, from, to, limit }, }), }), }), diff --git a/public/app/features/alerting/unified/components/rules/state-history/LogRecordViewer.tsx b/public/app/features/alerting/unified/components/rules/state-history/LogRecordViewer.tsx index 45fe627aa0c..734734c2cb0 100644 --- a/public/app/features/alerting/unified/components/rules/state-history/LogRecordViewer.tsx +++ b/public/app/features/alerting/unified/components/rules/state-history/LogRecordViewer.tsx @@ -19,21 +19,27 @@ interface LogRecordViewerProps { onLabelClick?: (label: string) => void; } +function groupRecordsByTimestamp(records: LogRecord[]) { + // groupBy has been replaced by the reduce to avoid back and forth conversion of timestamp from number to string + const groupedLines = records.reduce((acc, current) => { + const tsGroup = acc.get(current.timestamp); + if (tsGroup) { + tsGroup.push(current); + } else { + acc.set(current.timestamp, [current]); + } + + return acc; + }, new Map()); + + return new Map([...groupedLines].sort((a, b) => b[0] - a[0])); +} + export const LogRecordViewerByTimestamp = React.memo( ({ records, commonLabels, onLabelClick, onRecordsRendered }: LogRecordViewerProps) => { const styles = useStyles2(getStyles); - // groupBy has been replaced by the reduce to avoid back and forth conversion of timestamp from number to string - const groupedLines = records.reduce((acc, current) => { - const tsGroup = acc.get(current.timestamp); - if (tsGroup) { - tsGroup.push(current); - } else { - acc.set(current.timestamp, [current]); - } - - return acc; - }, new Map()); + const groupedLines = groupRecordsByTimestamp(records); const timestampRefs = new Map(); useEffect(() => { diff --git a/public/app/features/alerting/unified/components/rules/state-history/LokiStateHistory.test.tsx b/public/app/features/alerting/unified/components/rules/state-history/LokiStateHistory.test.tsx index 865e87cb4a5..24380f45f3f 100644 --- a/public/app/features/alerting/unified/components/rules/state-history/LokiStateHistory.test.tsx +++ b/public/app/features/alerting/unified/components/rules/state-history/LokiStateHistory.test.tsx @@ -81,6 +81,7 @@ const ui = { loadingIndicator: byText('Loading...'), timestampViewer: byRole('list', { name: 'State history by timestamp' }), record: byRole('listitem'), + noRecords: byText('No state transitions have occurred in the last 30 days'), }; describe('LokiStateHistory', () => { @@ -96,4 +97,18 @@ describe('LokiStateHistory', () => { expect(timestampViewerElement).toHaveTextContent('/api/live/ws'); expect(timestampViewerElement).toHaveTextContent('/api/folders/:uid/'); }); + + it('should render no entries message when no records are returned', async () => { + server.use( + rest.get('/api/v1/rules/history', (req, res, ctx) => + res(ctx.json({ data: { values: [] }, schema: { fields: [] } })) + ) + ); + + render(, { wrapper: TestProvider }); + + await waitFor(() => expect(ui.loadingIndicator.query()).not.toBeInTheDocument()); + + expect(ui.noRecords.get()).toBeInTheDocument(); + }); }); diff --git a/public/app/features/alerting/unified/components/rules/state-history/LokiStateHistory.tsx b/public/app/features/alerting/unified/components/rules/state-history/LokiStateHistory.tsx index 36c3937a496..8ceb39ce0b7 100644 --- a/public/app/features/alerting/unified/components/rules/state-history/LokiStateHistory.tsx +++ b/public/app/features/alerting/unified/components/rules/state-history/LokiStateHistory.tsx @@ -29,20 +29,28 @@ const LokiStateHistory = ({ ruleUID }: Props) => { const { getValues, setValue, register, handleSubmit } = useForm({ defaultValues: { query: '' } }); const { useGetRuleHistoryQuery } = stateHistoryApi; - const timeRange = useMemo(() => getDefaultTimeRange(), []); + + // We prefer log count-based limit rather than time-based, but the API doesn't support it yet + const queryTimeRange = useMemo(() => getDefaultTimeRange(), []); + const { currentData: stateHistory, isLoading, isError, error, - } = useGetRuleHistoryQuery({ ruleUid: ruleUID, from: timeRange.from.unix(), to: timeRange.to.unix() }); + } = useGetRuleHistoryQuery({ + ruleUid: ruleUID, + from: queryTimeRange.from.unix(), + to: queryTimeRange.to.unix(), + limit: 250, + }); const { dataFrames, historyRecords, commonLabels, totalRecordsCount } = useRuleHistoryRecords( stateHistory, instancesFilter ); - const { frameSubset, frameSubsetTimestamps } = useFrameSubset(dataFrames); + const { frameSubset, frameSubsetTimestamps, frameTimeRange } = useFrameSubset(dataFrames); const onLogRecordLabelClick = useCallback( (label: string) => { @@ -83,7 +91,7 @@ const LokiStateHistory = ({ ruleUID }: Props) => { const emptyStateMessage = totalRecordsCount > 0 ? `No matches were found for the given filters among the ${totalRecordsCount} instances` - : 'No state transitions have occurred in the last 60 minutes'; + : 'No state transitions have occurred in the last 30 days'; return (
@@ -120,7 +128,7 @@ const LokiStateHistory = ({ ruleUID }: Props) => { ) : ( <>
- +
{hasMoreInstances && (
@@ -147,7 +155,22 @@ function useFrameSubset(frames: DataFrame[]) { const frameSubset = take(frames, MAX_TIMELINE_SERIES); const frameSubsetTimestamps = sortBy(uniq(frameSubset.flatMap((frame) => frame.fields[0].values))); - return { frameSubset, frameSubsetTimestamps }; + const minTs = Math.min(...frameSubsetTimestamps); + const maxTs = Math.max(...frameSubsetTimestamps); + + const rangeStart = dateTime(minTs); + const rangeStop = dateTime(maxTs); + + const frameTimeRange: TimeRange = { + from: rangeStart, + to: rangeStop, + raw: { + from: rangeStart, + to: rangeStop, + }, + }; + + return { frameSubset, frameSubsetTimestamps, frameTimeRange }; }, [frames]); } @@ -199,7 +222,7 @@ const SearchFieldInput = React.forwardRef