-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
[SIP-133] Update "Time Range" filter in dashboard #28489
Comments
This doesn't seem like a SIP... and if it is, you'd need to fill out the rest of the template for it to be considered. That said, I kind of agree here, and feel this could be interpreted as a bug. Superset offers both "last" and "previous" options, and we have a bit of an inconsistency here. In "last" all the choices are essentially "last {granularity} until now" where "previous" is effectively "previous complete {granularity}. For example, "last month" is 2024-04-14 to 2024-05-14, whereas "previous month" is 2024-04-01 to 2024-05-01. To me, these seem sensible. When it comes to the day option(s) however, we're a bit inconsistent. The "last day" option is really what should be called "previous calendar day" under the "previous" menu. For "last day" it seems like it should be "24-hours-before now" As a workaround, and potential inroads to a PR, you can currently do what you're seeking under the "Custom" tab, like so: But I think I agree with you that this should be what "last day" actually does, and the current "last day" range should be moved over to a (new) "previous calendar day" option under "Previous". Curious what others (@kasiazjc @villebro @michael-s-molina ) think about this. It might not be a hard feature to add, but could be a tricky migration for existing filters/charts/etc. |
@rusackas Thank you for your comment. Yes, we can get from "Custom", but it's not a single click. We have to select "custom" and need tp select "Now" for END and modify days to 1 in START. We can add new option like "Last 24hrs" where time range will be last 24 hours. It won't require more changes I guess. For my usecase I added new option "Last 24hrs" and in time_range fetch api I just sending the time_range for last 24hrs in place of query params.
|
I think adding that option would be a fantastic PR, and not be considered a "breaking change" or need a migration. Once we have "Last 24 Hours" in place, I think it would make sense to move the current "Last Day" implementation over to the "Previous" tab as "Previous calendar day" and assess what migrations (if any) need to be done for existing charts using that option. |
I'm not sure this requires a SIP... I think you can just go straight to a PR with the above idea, but I'll ping @michael-s-molina @kasiazjc @eschutho for a few more opinions on the matter. If you do want to carry this forward as a SIP, you should open a "Discuss" thread on the dev@ mailing list... say the word if you need help pursuing that. |
I agree that this is inconsistent and that we should move the current "last day" to "previous day" when adding the new option. If a migration could do that automatically for existing charts, that would be fantastic. Regarding naming, I think it's okay that "last 24 hours" and "previous day" don't have the same terminology because the definitions are different, if I understand. "Last 24 hours" is just like what it sounds like, "previous day" is the the most recent completed calendar day from midnight to midnight. Is that right? I agree this should not be controversial or have enough different options to require the conversation of a SIP. |
@rusackas Today- Should consider today morning 00:00 to present time. |
I think the "Current" option being proposed in the PR linked right above this comment seems to scratch that itch. |
I think the "Current" PR merged... can we call this one closed, or is there more that needs to be discussed/implemented? |
Closing for now since it hasn't been brought up as a [DISCUSS] thread on the mailing list, and the "Current" range PR has already merged, which I believe scratches this itch. Correct me if I'm wrong and this needs to be reopened. |
[SIP-133] Proposal for Update "Time Range" filter in dashboard<title>
Motivation
Time Range filter contains "Last day" option it provides time range for yesterday.
I want the superset team to include "Today" in time range.
For "Today" time range should be
From Yesterday date at current time -To today date at current time
It will be useful for getting last 24hrs data from current time.
Proposed Change
Include "Today" in frontend dropdown list for time range,
Modify "api/v1/time_range" api to get time range for "TODAY"
New dependencies
Migration Plan and Compatibility
Rejected Alternatives
The text was updated successfully, but these errors were encountered: