add support for aggregations on quickwit ui #4974
Conversation
|
first bug: the |
|
|
quickwit/quickwit-ui/src/components/QueryEditor/AggregationEditor.tsx
Outdated
Show resolved
Hide resolved
quickwit/quickwit-ui/src/components/QueryEditor/AggregationEditor.tsx
Outdated
Show resolved
Hide resolved
quickwit/quickwit-ui/src/components/QueryEditor/AggregationEditor.tsx
Outdated
Show resolved
Hide resolved
use FormControl properly don't use lists, or set a key for each element change casing of some css properties
| 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) => { |
There was a problem hiding this comment.
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.
| type="number" | ||
| onChange={(e) => handleTermCountChange(pos, e)} | ||
| value={agg.term.size} | ||
| sx={{ "margin-left": "10px" }} |
| variant="standard" | ||
| label="Field" | ||
| onChange={(e) => handleTermFieldChange(pos, e)} | ||
| sx={{ "margin-left": "10px" }} |
| break; | ||
| } | ||
| case "rm": { | ||
| console.log("av", newAggregations); |
| if (aggregationConfig.histogram === null && aggregationConfig.term === null) { | ||
| const initialAggregation = Object.assign({}, ...aggregations); | ||
| const initialSearchRequest = {...props.searchRequest, aggregationConfig: initialAggregation}; | ||
| props.onSearchRequestUpdate(initialSearchRequest); |
There was a problem hiding this comment.
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.
|
|
||
| const drawAdditional = (pos: number, aggs: ({term: TermAgg} | {histogram: HistogramAgg})[]) => { | ||
| const agg = aggs[pos] | ||
| if (isHistogram(agg)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
_msearchendpoint, 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.
Reorganize jest mocks, add `@mui/x-charts` mock
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.