-
Notifications
You must be signed in to change notification settings - Fork 11
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
feature(graphs): filter data points by date filters #491
feature(graphs): filter data points by date filters #491
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, just one comment about timezones and some recommendations about packages available.
parameters.append(end_date) | ||
|
||
if start_date or end_date: | ||
raw_query += " ALLOW FILTERING" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have some estimation of the performance of those queries ? before/after ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but in general should be faster as there will be less data sent to the backend.
Possibly this whole table could be improved by making SUT_TIMESTAMP a static field - as we don't support different SUT versions for one test run. I'd consider doing this in separate PR. WDYT?
maybe it's time to do a session with a designer, about the UI of those ? |
In order to narrow down graphs range, date filters were added (by start and end date) with ability to quickly switch between 1,3 and 6 recent months. Last 3 months filter is on by default. closes: scylladb#474
5b59348
to
cf0309d
Compare
I can look into the UI/UX after this PR, we've discussed some changes in #489. |
if not test_id: | ||
raise Exception("No testId provided") | ||
|
||
start_date = datetime.fromisoformat(start_date_str).astimezone(timezone.utc) if start_date_str else None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the input string here look like? My concern is potential shift of the requested data +/- offset from UTC if input string doesn't have Z
at the end or timezone specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
input string is just the date e.g. 2024-10-30
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, other than one comment
Yes, I tried improving UI/UX but I'd rather do it in separate PR to discuss it there out of this context. |
In order to narrow down graphs range, date filters were added (by start and end date) with ability to quickly switch between 1,3 and 6 recent months. Last 3 months filter is on by default.
closes: #474