add support for aggregations on quickwit ui #4974

trinity-1686a · 2024-05-11T17:16:49Z

Description

fix #3223
add support for aggregation on quickwit ui

How was this PR tested?

manually. I have no idea how to properly test an ui

Disclaimer

this is my first time interacting with React, and I'm far from being an UI/UX person. What I've done might have huge pitfalls I can't comprehend.

trinity-1686a · 2024-05-11T17:21:00Z

first bug: the API URL link at the bottom is very broken with aggregations

trinity-1686a · 2024-05-11T21:10:30Z

~~term aggregation crash if no matching document~~
fixed

quickwit/quickwit-ui/src/components/QueryActionBar.tsx

quickwit/quickwit-ui/src/components/QueryEditor/AggregationEditor.tsx

ddelemeny

Nice job ! Spotted a few things to improve to avoid React/MUI complaints in console + a resource leak to fix.

quickwit/quickwit-ui/src/components/QueryEditor/QueryEditor.tsx

quickwit/quickwit-ui/src/utils/models.ts

quickwit/quickwit-ui/src/components/QueryEditor/AggregationEditor.tsx

ddelemeny

Second pass : some minor changes, some react arcane stuff and some safety boundaries required.

ddelemeny · 2024-05-16T19:58:03Z

quickwit/quickwit-ui/src/components/QueryActionBar.tsx

 import { TimeRangeSelect } from './TimeRangeSelect';
 import PlayArrowIcon from '@mui/icons-material/PlayArrow';
 import { SearchComponentProps } from "../utils/SearchComponentProps";

 export function QueryEditorActionBar(props: SearchComponentProps) {
  const timestamp_field_name = props.index?.metadata.index_config.doc_mapping.timestamp_field;
  const shouldDisplayTimeRangeSelect = timestamp_field_name ?? false;
+
+  const handleChange = (_event: React.SyntheticEvent, newTab: number) => {


I would expect this to trigger a "mode" change (conditional rendering of the UI) and an initializing run of the request. The current tab switch behavior is weirdly inconsequential.

ddelemeny · 2024-05-16T20:03:50Z

quickwit/quickwit-ui/src/components/QueryEditor/AggregationEditor.tsx

+              type="number"
+              onChange={(e) => handleTermCountChange(pos, e)}
+              value={agg.term.size}
+              sx={{ "margin-left": "10px" }}


Should be camelCased.

ddelemeny · 2024-05-16T20:04:14Z

quickwit/quickwit-ui/src/components/QueryEditor/AggregationEditor.tsx

+              variant="standard"
+              label="Field"
+              onChange={(e) => handleTermFieldChange(pos, e)}
+              sx={{ "margin-left": "10px" }}


Should be camelCased.

ddelemeny · 2024-05-16T20:05:39Z

quickwit/quickwit-ui/src/components/QueryEditor/AggregationEditor.tsx

+          break;
+        }
+        case "rm": {
+          console.log("av", newAggregations);


rm console logs

ddelemeny · 2024-05-16T20:39:02Z

quickwit/quickwit-ui/src/components/QueryEditor/AggregationEditor.tsx

+  if (aggregationConfig.histogram === null && aggregationConfig.term === null) {
+    const initialAggregation = Object.assign({}, ...aggregations);
+    const initialSearchRequest = {...props.searchRequest, aggregationConfig: initialAggregation};
+    props.onSearchRequestUpdate(initialSearchRequest);


This will cause a setState in the parent component from the render fn of the current one, which triggers a render somewhere else in the tree. Undesired side-effect inside a render, not allowed (react will warn in the console).

You can wrap this init block in a useEffect(()=>{/*init here*/},[/*deps*/]) call. Leave the deps array empty if the effect is to be executed once.

ddelemeny · 2024-05-16T20:49:07Z

quickwit/quickwit-ui/src/components/QueryEditor/AggregationEditor.tsx

+
+  const drawAdditional = (pos: number, aggs: ({term: TermAgg} | {histogram: HistogramAgg})[]) => {
+    const agg = aggs[pos]
+    if (isHistogram(agg)) {


This may very well crash the tab of the unsuspecting user, trying to render thousands of buckets.
Search params being serialized in the urls make it even worse (refresh won't help).

I'd favor computing a safe-to-render resolution based on the selected time range.

so i should remove that Select? What should i do when no time range is selected? Send 2 requests for the lowest/highest timestamp matching the query?

Yes, removing the option here is probably the best course, you can check how it's done on the grafana plugin here : https://github.com/quickwit-oss/quickwit-datasource/blob/main/src/utils/time.ts

On addressing the no-timerange issue, there are a few possibilities:

provide a default range -> quite simple, that's how grafana works in the context of log exploration but it comes with a slight change of expectations from the UI. Should probably be cleared on search in order to allow min/max queries without constraints.

fetch dataset bounds -> on the elastic api, this could be done in one round trip over the _msearch endpoint, not sure about the index api.

reissue modified request -> another interesting option would be to issue a first request as it is done now, check the result, and if necessary reissue the request with adjusted bounds and resolution. More of a gamble, success is fast, fail is slower.

forbid unbounded queries -> just show an alert prompting the user to select bounds. Not the most friendly approach but definitely simple to implement.

I think the 2nd option would be more friendly. Tutorials are made with "historical" data, where a default range would likely match nothing. 3 is a worst version of 2 imo. 4 sounds bad to use given the simple search allows not providing bounds

I started to implement the 2nd option on a private branch but it seems to add a lot of complexity, potential fetch errors, and lots of lignes of code to the merge requests. So went with the "removing the option here is probably the best course" solution. I did put 1day interval by default to reduce to probability of errors due to the number of buckets selected.

fulmicoton · 2024-06-11T09:43:09Z

@fmassot can you take over the review?

fix #3223

use FormControl properly don't use lists, or set a key for each element change casing of some css properties

CLAassistant · 2024-06-18T14:27:43Z

All committers have signed the CLA.

…for user. It's better to clic on Run

github-actions · 2024-06-20T12:48:10Z

On SSD:

Average search latency is 0.992x that of the reference (lower is better).
Ref run id: 2220, ref commit: 09888bb
Link

On GCS:

Average search latency is 1.02x that of the reference (lower is better).
Ref run id: 2221, ref commit: 09888bb
Link

Approved

trinity-1686a requested a review from fmassot May 11, 2024 17:16

fmassot requested review from ddelemeny and removed request for fmassot May 12, 2024 09:48

RaphaelMarinier reviewed May 15, 2024

View reviewed changes

quickwit/quickwit-ui/src/components/QueryActionBar.tsx Outdated Show resolved Hide resolved

quickwit/quickwit-ui/src/components/QueryEditor/AggregationEditor.tsx Outdated Show resolved Hide resolved

ddelemeny requested changes May 15, 2024

View reviewed changes

trinity-1686a requested a review from ddelemeny May 16, 2024 17:11

ddelemeny previously requested changes May 16, 2024

View reviewed changes

fulmicoton requested review from fmassot and ddelemeny and removed request for ddelemeny June 11, 2024 09:42

trinity-1686a and others added 13 commits June 13, 2024 15:12

make monaco editor respond to window resize

07abdc8

fix #3223

add selector for aggregation

cd73f35

support sending aggregations

1b18a46

show aggregation results

6283ac3

fix formating

bec3400

fix crash on empty result

6541393

try to make typing experience better

5fb5149

codestyle and fix typo

5b3705a

use 'in' instead of hasOwn

4967d92

fix most errors

af3cc58

use FormControl properly don't use lists, or set a key for each element change casing of some css properties

remove listener

d6c0b8c

Move jest mocks to dedicated folder

13d7848

Add x-charts LineChart mock

ed60163

JulesVautier-io force-pushed the trinity/improve-ui branch from 0ab72ed to ed60163 Compare June 13, 2024 13:14

JulesVautier-io force-pushed the trinity/improve-ui branch from 5955534 to 6639863 Compare June 18, 2024 14:38

Remove the auto aggregation featur, put 1day by default instead

746427d

JulesVautier-io added 2 commits June 18, 2024 18:34

Remove console logs and add camelCase

7799739

Run search and update UI when changing search tab

91e2256

JulesVautier-io force-pushed the trinity/improve-ui branch from 6639863 to 756968a Compare June 18, 2024 16:34

Fix the render side-effect by using the useEffect feature

bccdf7b

JulesVautier-io force-pushed the trinity/improve-ui branch from 756968a to bccdf7b Compare June 20, 2024 09:40

Do not re run search in each aggregation change. It can be too heavy …

2fb323f

…for user. It's better to clic on Run

fmassot approved these changes Jun 20, 2024

View reviewed changes

Merge branch 'main' into trinity/improve-ui

1a3701d

fmassot enabled auto-merge (squash) June 20, 2024 11:50

fmassot merged commit 22dea2d into main Jun 20, 2024
8 checks passed

fmassot deleted the trinity/improve-ui branch June 20, 2024 12:59

This was referenced Jun 20, 2024

Jules/fix pipeline #5144

Closed

Jules/fix ci #5145

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for aggregations on quickwit ui #4974

add support for aggregations on quickwit ui #4974

trinity-1686a commented May 11, 2024 •

edited

Loading

trinity-1686a commented May 11, 2024

trinity-1686a commented May 11, 2024 •

edited

Loading

ddelemeny left a comment

ddelemeny left a comment

ddelemeny May 16, 2024

ddelemeny May 16, 2024

ddelemeny May 16, 2024

ddelemeny May 16, 2024

ddelemeny May 16, 2024

ddelemeny May 16, 2024

trinity-1686a May 17, 2024

ddelemeny May 20, 2024

trinity-1686a May 27, 2024

JulesVautier-io Jun 18, 2024 •

edited

Loading

fulmicoton commented Jun 11, 2024

CLAassistant commented Jun 18, 2024 •

edited

Loading

github-actions bot commented Jun 20, 2024

add support for aggregations on quickwit ui #4974

add support for aggregations on quickwit ui #4974

Conversation

trinity-1686a commented May 11, 2024 • edited Loading

Description

How was this PR tested?

Disclaimer

trinity-1686a commented May 11, 2024

trinity-1686a commented May 11, 2024 • edited Loading

ddelemeny left a comment

Choose a reason for hiding this comment

ddelemeny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JulesVautier-io Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

fulmicoton commented Jun 11, 2024

CLAassistant commented Jun 18, 2024 • edited Loading

github-actions bot commented Jun 20, 2024

On SSD:

On GCS:

trinity-1686a commented May 11, 2024 •

edited

Loading

trinity-1686a commented May 11, 2024 •

edited

Loading

JulesVautier-io Jun 18, 2024 •

edited

Loading

CLAassistant commented Jun 18, 2024 •

edited

Loading