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.

Sign up for GitHub

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

Jump to bottom

Make sure to is 'now' and not the same as from #81524

Merged
merged 4 commits into from
Oct 26, 2020
Merged

Make sure to is 'now' and not the same as from #81524

merged 4 commits into from
Oct 26, 2020

Conversation

joelgriffith
Copy link
Contributor

@joelgriffith joelgriffith commented Oct 22, 2020 •
edited
Loading

Summary

Fixes an issue when exporting a Saved Search Visualization (inside of a Dashboard) to CSV returning blank rows. This only comes into play when 1 day or 1 week are selected as time ranges.

Excelsior

Prior fix/question

This fixes an issue where setting 1 week or Last day sets a from and to value of the same string. Downstream, saved-search visualizations that are exported to CSV via the Dashboard page, are returning blank because of this duplicative from/to.

I'm not sure why these were set to the same value prior, and I see them in a few other places (tests mostly), but this seems wrong to me. Is there historic reasons why the to and from for a range like 1 week share both the same start/end ranges?

In any case this fixes a bug in Saved Search viz's tables having blank rows when downloading them as a CSV. This seems a trivial change, but a scary one.

@joelgriffith joelgriffith requested a review from a team as a code owner October 22, 2020 18:46
@joelgriffith joelgriffith added Feature:Dashboard Dashboard related features (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:fix Team:AppArch v7.10.1 v8.0.0 labels Oct 22, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@joelgriffith
Copy link
Contributor Author

I anticipate that there's going to be some test failures, but I'm not going to go through the process of updating them as the code might be the way it was for good reason.

@flash1293
Copy link
Contributor

flash1293 commented Oct 22, 2020 •
edited
Loading

I think the missing bit here is that in client side Kibana the from is rounded down while the to is rounded up when “/< unit >” is used in the date math: https://github.com/elastic/kibana/blob/master/src/plugins/data/common/query/timefilter/get_time.ts#L33

I guess reporting should use the same logic for resolving the time range

@flash1293
Copy link
Contributor

Related discussion: #64125

@joelgriffith
Copy link
Contributor Author

Since this seems to have came up a few times now, should we make the from and to now have more semantic meaning? It definitely caused issues in reporting where we've used it: https://github.com/elastic/kibana/blob/master/x-pack/plugins/reporting/public/panel_actions/get_csv_panel_action.tsx#L99 and upon debugging the issue, it appeared to me to be an obvious bug.

@flash1293
Copy link
Contributor

Seems like the issue linked above is the right place to discuss. It’s not obvious to me what we can do though - currently “Today” means the current day (into the past and into the future). That’s not the same as “the current day up until now” if you have data in the future (which would be what this PR is doing) - I would consider that a breaking change.

I’m not part of the app arch team, so take it with a grain of salt, but my suggestion would be:

  • Short term use the function provided by the data plugin to resolve the time range to make sure it’s aligned with client side Kibana behavior
  • Longer term discuss on the issue how we can improve the behavior and make sure it’s less confusing

By the way, I totally get the confusion and I was confused about it myself. Maybe the smallest meaningful thing we can do is rename the properties of the TimeRange type to something reflecting how they should get resolved.

@joelgriffith
Copy link
Contributor Author

Gotcha, I felt something like this was brewing! I'll fix it in our codebase even though it does feel a bit left-handed to have the API return this.

Thanks @flash1293 for filling in some gaps of knowledge I had!

@joelgriffith
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

page load bundle size

id before after diff
reporting 72.0KB 72.0KB +15.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

tsullivan
tsullivan approved these changes Oct 26, 2020
View reviewed changes
Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewed the code changes

Based on the conversation, I see why this change is the best way to resolve the issue.

@joelgriffith joelgriffith merged commit 932e92b into elastic:master Oct 26, 2020
This was referenced Oct 26, 2020
Merged
Merged
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 27, 2020
@gmmorris
Merge branch 'master' into task-manager/health
314f3bc 
* master: (37 commits)
  [ILM] Migrate Warm phase to Form Lib (elastic#81323)
  [Security Solutions][Detection Engine] Fixes critical bug with error reporting that was doing a throw (elastic#81549)
  [Detection Rules] Add 7.10 rules (elastic#81676)
  [kbn/optimizer] ignore missing metrics when updating limits with --focus (elastic#81696)
  [SECURITY SOLUTIONS] Bugs overview page + investigate eql in timeline (elastic#81550)
  [Maps] fix unable to edit cluster vector styles styled by count when switching to super fine grid resolution (elastic#81525)
  Fixed migration issue for case specific actions, by extending email action migrator checks (elastic#81673)
  [CI] Preparation for APM tracking on CI (elastic#80399)
  [Home] Fixes Kibana app description order on home page and updates Canvas copy (elastic#80057)
  Make sure `to` is 'now' and not the same as `from` (elastic#81524)
  Nitpicking the 8.0 Breaking Change issue template (elastic#81678)
  [SECURITY_SOLUTION] Fix text on onboarding screen (elastic#81672)
  [data.search] Skip async search tests in build candidates and production builds (elastic#81547)
  Fix previousStartedAt by not changing when execution fails (elastic#81388)
  [Monitoring] Fix a couple of issues with the cpu usage alert (elastic#80737)
  Telemetry collection xpack to ts project references (elastic#81269)
  Elasticsearch: don't use url authentication for new client (elastic#81564)
  [App Search] Credentials: implement working flyout form (elastic#81541)
  Properly encode links to edit user page (elastic#81562)
  [Alerting UI] Don't wait for health check before showing Create Alert flyout (elastic#80996)
  ...
joelgriffith pushed a commit that referenced this pull request Oct 28, 2020
@kibanamachine
Make sure to is 'now' and not the same as from (#81524) (#81689)
be3b887 
* Make sure `to` is 'now' and not the same as `from`

* Revert "Make sure `to` is 'now' and not the same as `from`"

This reverts commit 48e8d08.

* Ensure `to` is properly rounded up to prevent `from` and `to` being identical

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
joelgriffith pushed a commit that referenced this pull request Oct 28, 2020
@kibanamachine
Make sure to is 'now' and not the same as from (#81524) (#81688)
8ef6166 
* Make sure `to` is 'now' and not the same as `from`

* Revert "Make sure `to` is 'now' and not the same as `from`"

This reverts commit 48e8d08.

* Ensure `to` is properly rounded up to prevent `from` and `to` being identical

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@streamich streamich streamich approved these changes

@tsullivan tsullivan tsullivan approved these changes

Assignees
No one assigned
Labels
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead Feature:Dashboard Dashboard related features release_note:fix v7.10.1 v7.11.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@joelgriffith @elasticmachine @flash1293 @kibanamachine @tsullivan @streamich