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

[EuiSuperDatePicker] Add "Round to" flag on commonly used section and expose a property to enable it by default #4537

Closed
cauemarcondes opened this issue Feb 17, 2021 · 23 comments

Comments

@cauemarcondes
Copy link

On the Relative tab there's a flag called Round to the ${unit} that when enabled rounds the selected time.

Screenshot 2021-02-17 at 13 25 08

I would like to have a similar flag on the commonly used section:

Screenshot 2021-02-17 at 13 46 54

When that is enabled any option selected would be rounded.

I would also like to have a new property to enable it by default.

@hetanthakkar
Copy link
Contributor

@cchaos Can i take this issues ?

@cchaos
Copy link
Contributor

cchaos commented Feb 23, 2021

This issue is marked as needs design as it will have to implement a new UI element to handle the feature request. I don't think we have a good path forward just yet.

@cchaos cchaos changed the title [Date Picker] Add "Round to" flag on commonly used section and expose a property to enable it by default [EuiSuperDatePicker] Add "Round to" flag on commonly used section and expose a property to enable it by default Feb 23, 2021
@hetanthakkar
Copy link
Contributor

@cchaos okay i will check it out once the design implementation is ready

@github-actions
Copy link

👋 Hey there. This issue hasn't had any activity for 180 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

@cchaos
Copy link
Contributor

cchaos commented Aug 23, 2021

Still valid

@cchaos
Copy link
Contributor

cchaos commented Aug 23, 2021

Ideally, we'd just be able to pull that toggle out of being under the "Relative" tab specifically, and add to the popover as a footer.

image

Then it would show up for every tab (even "Now") but it will also have to be respected no matter which way the user has chosen to select a date.

image

@dgieselaar
Copy link
Member

@cchaos curious if we can prioritise this? I think it'd be helpful for increasing shard request cache hit ratio. Especially with elastic/elasticsearch#79610, this seems like relatively low-hanging fruit with potentially a high positive impact on search performance/CPU usage in a cluster. I can also pick it up myself if these specs are final. This would allow us to move forward with elastic/kibana#94280 as well.

@dgieselaar
Copy link
Member

I took a stab at this, but IMO the current handling of date rounding in the date picker leads to confusing behaviour. E.g., I actually think we want to round up, not down in most cases, and we want to apply date rounding on both the start and end of the date range. Right now I think the lower end will be rounded down and the upper end will be rounded up which leads to a bigger date range being selected than I would intuitively guess. E.g. if I select from: now-15m/m, to: now/m, 16 minutes will be selected instead of 15 (as far as I can tell). Maybe the date picker itself should not apply date rounding, and it should be left up to the consumer, only propagating a round property. Thoughts?

@dgieselaar
Copy link
Member

Can someone elaborate why we round down by default? I see that we set date ranges like { from: 'now/d', to: 'now/d' }, but I don't understand why we're not setting those ranges to be { from: 'now-1d/d', to: 'now/d' } or { from: 'now/d', to: 'now+1d/d' }?

@cchaos
Copy link
Contributor

cchaos commented Nov 22, 2021

I think it actually depends on whether you're adjusting the start or end date. It looks like it rounds down for start and up for end.
Screen Shot 2021-11-22 at 10 59 27 AM
Screen Shot 2021-11-22 at 10 59 45 AM

@cchaos
Copy link
Contributor

cchaos commented Nov 22, 2021

Also, @nreese was instrumental in building this component. I wonder if he's got further insight?

@nreese
Copy link
Contributor

nreese commented Nov 22, 2021

The EUI date picker components just followed the model of the existing Kibana date picker components so all of these rounding decisions pre-date the EUI components.

The reason the start date is rounded down and the end date is rounded up can be seen in an example like today, where today is Nov 22nd.

  • With { from: 'now/d', to: 'now/d' }, rounding down for the start provides a value like 2021-11-22T00:00:00.000 and rounding up for the end provides a value like 2021-11-22T23:59:59.999. The time period only covers Today.
  • If you used { from: 'now-1d/d', to: 'now/d' } the start day would be Nov 21st even when rounded up and the time period would cover a sliver of Yesterday.
  • If you used { from: 'now/d', to: 'now+1d/d' } the end day would be Nov23rd even when rounded down and the time period would cover a sliver of Tomorrow.

Maybe @spalger or @lukasolson can provide more history?

@nreese
Copy link
Contributor

nreese commented Nov 22, 2021

One more thought, changing the rounding would be a breaking change as { from: 'now/d', to: 'now/d' } would no longer mean "today" if the rounding changed. Users can customize the quick select values in advanced setting so these can be custom values and I am not sure it would be possible to programmatically discover the user's intent with the time ranges and modify them correctly.

For better or worse, I don't think it's possible to change the rounding without breaking existing users.

@dgieselaar
Copy link
Member

dgieselaar commented Nov 23, 2021

Yeah I don't expect this to change anytime soon given the impact. But it's very counterintuitive that { from: 'now-15m/m', to: 'now/m' } results in a 16m range, not a 15m range. e.g. @cchaos has selected 29 minutes in her screenshot even though the selected range runs from now-30m until now-2m, which I would expect to represent a range of 28m.

@dgieselaar
Copy link
Member

dgieselaar commented Nov 23, 2021

Maybe a good workaround is to change the default values for date ranges? E.g.:

[
  {
    "from": "now/d",
    "to": "now/d",
    "display": "Today"
  },
  {
    "from": "now/w",
    "to": "now/w",
    "display": "This week"
  },
  {
    "from": "now-14m/m",
    "to": "now/m",
    "display": "Last 15 minutes"
  },
  {
    "from": "now-29m/m",
    "to": "now/m",
    "display": "Last 30 minutes"
  },
  {
    "from": "now-59m/m",
    "to": "now/m",
    "display": "Last 1 hour"
  },
  {
    "from": "now-23h/h",
    "to": "now/h",
    "display": "Last 24 hours"
  },
  {
    "from": "now-6d/d",
    "to": "now/d",
    "display": "Last 7 days"
  },
  {
    "from": "now-29d/d",
    "to": "now/d",
    "display": "Last 30 days"
  },
  {
    "from": "now-89d/d",
    "to": "now/d",
    "display": "Last 90 days"
  },
  {
    "from": "now-1y/d",
    "to": "now/d",
    "display": "Last 1 year"
  }
]

@nreese
Copy link
Contributor

nreese commented Nov 23, 2021

Maybe a good workaround is to change the default values for date ranges?

Any changes to the defaults should be coordinated with the Kibana repo. https://github.com/elastic/kibana/blob/main/src/plugins/data/server/ui_settings.ts#L356 is where Kibana's defaults come from.

How about opening this issue in the Kibana repo and get a discussion about defaults going there?

cchaos added a commit that referenced this issue Dec 6, 2021
* [EuiSuperDatePicker] Sizing updates
  - Adds `width` and `isQuickSelectOnly` props
  - Adjusts styles accordingly with sensible min-widths
  - Updated Save Queries example to show auto width and reducing to quick select only when KQL is in focus
  - Fixed `dataTestSubj` -> `data-test-subj`
  - Removed ‘Show dates’ in favor of automatically expanding pretty format and opening start date popover
* [EuiIcon] Added `timeRefresh`
* [EuiFormControlLayout] Fix background of button prepend/appends with `readOnly`
  - And fixed border-radius for EuiSuperDatePicker with `append`
* [EuiRefreshInterval] Using a toggle for start/stop
  - disabling inputs if paused
  - invalidating value if 0 or less
  - always rendering (moved render logic to quick select)
  - render in one row
* [New] EuiAutoRefresh & EuiAutoRefreshButton components
* [EuiSuperDatePicker] Simplify `isAutoRefreshOnly` to render the new EuiAutoRefresh component
  - Increases the default `refreshInterval` to `1000` / `1s`
* [EuiSuperDatePicker] Added EuiAutoRefreshButton as an `append` when turned on
  - [prettyInterval()] added `shortHand` parameter
* Fix color of prepend/append backgrounds in dark mode
* Moved round switch to popover footer to prepare for #4537
* [EuiSuperDatePicker] Now supports `compressed`
* [EuiSuperUpdateButton] Add support for `iconOnly` version and move `responsive` to customizable prop
  - Support in EuiSuperDatePicker with `showUpdateButton = ‘iconOnly’`
  - Fix min-widths on smaller screens
* [EuiSuperUpdateButton] Splitting up prop types between internal and configurable
@github-actions
Copy link

👋 Hey there. This issue hasn't had any activity for 180 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

@sorenlouv
Copy link
Member

sorenlouv commented May 23, 2022

How about opening this issue in the Kibana repo and get a discussion about defaults going there?

Kibana issue follow-up: elastic/kibana#94280

@github-actions
Copy link

👋 Hey there. This issue hasn't had any activity for 180 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

@sorenlouv
Copy link
Member

sorenlouv commented Nov 20, 2022

I think it'd be helpful for increasing shard request cache hit ratio. Especially with elastic/elasticsearch#79610, this seems like relatively low-hanging fruit with potentially a high positive impact on search performance/CPU usage in a cluster.

...

Can someone elaborate why we round down by default? I see that we set date ranges like { from: 'now/d', to: 'now/d' }, but I don't understand why we're not setting those ranges to be { from: 'now-1d/d', to: 'now/d' } or { from: 'now/d', to: 'now+1d/d' }?

@dgieselaar Do we still need this from EUI or can we close this and instead start recommending users that they should change their defaults (perhaps through a warning in APM UI)?

@JasonStoltz
Copy link
Member

We lost quite a bit of context for this issue recently. Is there still something that Kibana needs from EUI here?

@dgieselaar
Copy link
Member

@JasonStoltz It's not a priority for us anymore, we've abandoned the idea of improving cache efficiency. I do think something is fundamentally broken with date rounding in the date picker, but I can see how changing that is a significant effort.

@daveyholler
Copy link
Contributor

Since this is no longer a priority, we'll close this issue. If this changes, please open a new issue.

@daveyholler daveyholler closed this as not planned Won't fix, can't repro, duplicate, stale Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants