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

Add a predefined TimeRange filter if there is at least one DateTime* column #304

Merged
merged 8 commits into from
Mar 1, 2023

Conversation

slvrtrn
Copy link
Contributor

@slvrtrn slvrtrn commented Feb 22, 2023

Demo:

Screen.Recording.2023-02-23.at.00.16.05.mov

Full description:

There was a request from our users to restrict the query to the dashboard range automatically.

After some consideration, we decided to add a (removable) pre-defined filter that appears when we switch the tables if and only if we have at least one DateTime column. If we find some, the first DateTime column is picked, and we add the WithInGrafanaTimeRange filter automatically.

What is interesting about this filter is that the column selection there is restricted to DateTime (could be also *64 or Nullable) types only, so it is convenient to switch to another column if we failed to pick the right one, and the type of the filter is WithInGrafanaTimeRange by default.

If a user does not need this type of filter, they can delete it and just add another one.

I also added a convenience ClickHouse container to the docker-compose.yml.

@slvrtrn slvrtrn requested a review from a team as a code owner February 22, 2023 23:29
@mshustov
Copy link
Collaborator

mshustov commented Feb 23, 2023

IMO the timerange filter should be applied by default without the necessity to introduce another UI element.
Ok, we ClickHouse doesn't have a convention for timestamp field for time series data, so we have to provide an option to select a field, but have sensible defaults for time-series data.

The default behavior of other datasources is to restrict timerange to the current value. For example, elasticsearch:
UI
2023-02-23_12-05-06

query:

{"size":0,"query":{"bool":{"filter":[{"range":{"timestamp":{"gte":1677128427354,"lte":1677150027354,"format":"epoch_millis"}}}]}}

@bossinc what if we change the default query builder behavior to detect a first time/datetime field and apply WITHIN DASHBOARD TIME RANGE to it? The current implementation limited the filter value to WITHIN DASHBOARD TIME RANGE, which looks like a limitation for complex queries (with the combination of >, >=, OUTSIDE DASHBOARD TIME RANGE, etc.)
2023-02-23_12-45-13 (2)
If you agree, could you point out the proper integration points?

@slvrtrn
Copy link
Contributor Author

slvrtrn commented Feb 23, 2023

@mshustov, how do you select the DateTime column in that case?

@mshustov
Copy link
Collaborator

how do you select the DateTime column in that case?

I suggest providing a column selection and setting the value to WITHIN DASHBOARD TIME RANGE. The current implementation in the PR doesn't allow a user to change value from WITHIN DASHBOARD TIME RANGE.

@slvrtrn
Copy link
Contributor Author

slvrtrn commented Feb 23, 2023

@mshustov

Now it is like this:

Screen.Recording.2023-02-23.at.17.20.16.mov

WDYT?

@mshustov
Copy link
Collaborator

@slvrtrn. Should the user have the ability to remove the filter? The current implementation doesn't support deletion.

@slvrtrn
Copy link
Contributor Author

slvrtrn commented Feb 24, 2023

I simplified the logic now; the deletion bug should be gone.

mshustov
mshustov previously approved these changes Feb 24, 2023
Copy link
Contributor

@asimpson asimpson left a comment

Choose a reason for hiding this comment

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

This is good!

I think I found one area where we have a user experience hole: removing all the filters. We should account for a user who removes the filters but then changes their mind.

src/components/queryBuilder/QueryBuilder.tsx Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
Copy link
Contributor

@asimpson asimpson left a comment

Choose a reason for hiding this comment

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

Nicely done! 🏆

src/components/queryBuilder/QueryBuilder.tsx Show resolved Hide resolved
@asimpson asimpson merged commit 5f25fb4 into grafana:main Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants