[Discover] Fix csv export with relative time filter from discover main view #123206

dimaanj · 2022-01-18T10:47:27Z

Summary

Fixes #122575

This PR redundant absolute filter from csv export generated from discover main view.

Test notes

Create an index pattern which contains data during the last N days.
Choose Last 24h in the timepicker (note the URL contains only (filters:!(),refreshInterval:(pause:!t,value:0),time:(from:now-24h%2Fh,to:now))&_a=(columns:!(),filters:!(),index:'10881820-3b02-11eb-a646-7340c405390d',interval:auto,query:(language:kuery,query:''),sort:!(!(timestamp,desc))))
Save the Saved Search - The URL still shows (filters:!(),refreshInterval:(pause:!t,value:0),time:(from:now-24h%2Fh,to:now))&_a=(columns:!(),filters:!(),grid:(),hideChart:!f,index:'10881820-3b02-11eb-a646-7340c405390d',interval:auto,query:(language:kuery,query:''),sort:!(!(timestamp,desc)))
Go on Share / CSV Export / Copy POST URL
Decode URL
Decoded URL should contain only one relative (last 24 hours) time range query, like that query:(range:(timestamp:(format:strict_date_optional_time,gte:now-24h/h,lte:now))))

… page

elasticmachine · 2022-01-18T12:24:59Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

src/plugins/discover/public/utils/get_sharing_data.ts

dimaanj · 2022-01-20T10:06:57Z

@elasticmachine merge upstream

kertal · 2022-01-20T17:43:32Z

src/plugins/discover/public/utils/get_sharing_data.ts

+      const absoluteFilter = data.query.timefilter.timefilter.createFilter(index);
      const timeFilter = absoluteTime
-        ? data.query.timefilter.timefilter.createFilter(index)
+        ? absoluteFilter
        : data.query.timefilter.timefilter.createRelativeFilter(index);

+      // remove timeFilter from existing filter
+      if (Array.isArray(existingFilter)) {
+        existingFilter = existingFilter.filter(
+          (current) => !isEqualFilters(current, absoluteFilter)
+        );
+      } else if (isEqualFilters(existingFilter, absoluteFilter)) {
+        existingFilter = undefined;
+      }
+


a first thought about this ... now is converted to absolute time when creating a filter, are we sure the generated time here matches to the existing time of the existing filter, which might also be converted, but it was converted before the creation of absoluteFilter. I'm not 100% sure here how this works, so I'd need to dive a bit deeper then I can this week. in any case we'd need test coverage for this.

@kertal it does look like there could be a race condition.

This is quite tricky to reason about -- I agree tests for the different cases would be really useful (even as a way to document all this!). AFAICT there are 2 scenarios we want to handle:

Share data is requested for immediate use (e.g. CSV of exactly now)

Share data is requested for deferred use (e.g. CSV whenever + rolling window)

In case 1 we want a search source containing an absolute time range so that we can give the user an accurate picture of their data in that moment.

In case 2 we want to give users a "rolling window" so they can get their data over time (unless they've explicitly set an absolute time filter, then RIP relative time filter).

A complication is in both cases there could be cascading time range filters, themselves either relative or absolute 🤪

therefore I think this code should try to "only" concern itself with the 2 scenarios AND not overriding existing filters. broadly something like:

if (absoluteTime === true) { // we want to generate a search source that will generate a picture of the data now // append the absolute time filter } else { // we want to generate a search source that will provide a rolling window // append the relative time filter }

What do you guys think?

Ah, one piece I missed is "extracting" the current time range filter in Discover because we are converting to either relative or absolute, so some overriding is required 🤔 are these perhaps not separated by SearchSource.getParent?

Alright, I think we agree then we should improve code here, but since we also need to backport I'd suggest to keep the scope small, just suggested a way we can be sure that there are no race conditions. If this is fine, it would fix the issue, and we could think about improving stuff in a future PR targeting simplification in this area. How does that sound

dimaanj · 2022-01-24T19:16:01Z

@elasticmachine merge upstream

…-for-relative-timefilter

dimaanj · 2022-01-26T13:30:11Z

@elasticmachine merge upstream

dimaanj · 2022-01-27T20:26:07Z

@elasticmachine merge upstream

dimaanj · 2022-01-31T11:37:16Z

@elasticmachine merge upstream

tsullivan

LGTM

dimaanj · 2022-03-16T05:03:14Z

@elasticmachine merge upstream

kibana-ci · 2022-03-16T06:32:11Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`discover`	376.6KB	380.3KB	+3.7KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`discover`	50.4KB	50.4KB	+22.0B

History

💔 Build #30439 failed d1a345a
💛 Build #29842 was flaky 17927ed
💚 Build #29821 succeeded 2040d86
💚 Build #28675 succeeded 2831c1c
💚 Build #25551 succeeded 25a87bb

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

cc @dmitriynj

kertal

LGTM, thx for the fix and the functional coverage 👍

…n view (#123206) * [Discover] fix relative time filter for csv export from discover main page * [Discover] fix array assignment * [Discover] fix functional test * [Discover] add test coverage for the issue * [Discover] add debug points for functional test * [Discover] try to get clipboard permissions * [Discover] fix functional test * [Discover] apply suggestion * [Discover] apply suggestion * [Discover] apply naming suggestion Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit deb7099)

kibanamachine · 2022-03-16T07:35:44Z

💔 Some backports could not be created

Status	Branch	Result
❌	7.17	Backport failed because of merge conflicts
✅	8.1

Manual backport

To create the backport manually run:

node scripts/backport --pr 123206

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…n view (#123206) (#127847) * [Discover] fix relative time filter for csv export from discover main page * [Discover] fix array assignment * [Discover] fix functional test * [Discover] add test coverage for the issue * [Discover] add debug points for functional test * [Discover] try to get clipboard permissions * [Discover] fix functional test * [Discover] apply suggestion * [Discover] apply suggestion * [Discover] apply naming suggestion Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit deb7099) Co-authored-by: Dmitry Tomashevich <39378793+Dmitriynj@users.noreply.github.com>

…n view (elastic#123206) * [Discover] fix relative time filter for csv export from discover main page * [Discover] fix array assignment * [Discover] fix functional test * [Discover] add test coverage for the issue * [Discover] add debug points for functional test * [Discover] try to get clipboard permissions * [Discover] fix functional test * [Discover] apply suggestion * [Discover] apply suggestion * [Discover] apply naming suggestion Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

…n view (elastic#123206) * [Discover] fix relative time filter for csv export from discover main page * [Discover] fix array assignment * [Discover] fix functional test * [Discover] add test coverage for the issue * [Discover] add debug points for functional test * [Discover] try to get clipboard permissions * [Discover] fix functional test * [Discover] apply suggestion * [Discover] apply suggestion * [Discover] apply naming suggestion Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit deb7099) # Conflicts: # src/plugins/discover/public/application/apps/main/utils/get_sharing_data.ts # x-pack/test/functional/apps/discover/reporting.ts

…n view (elastic#123206) * [Discover] fix relative time filter for csv export from discover main page * [Discover] fix array assignment * [Discover] fix functional test * [Discover] add test coverage for the issue * [Discover] add debug points for functional test * [Discover] try to get clipboard permissions * [Discover] fix functional test * [Discover] apply suggestion * [Discover] apply suggestion * [Discover] apply naming suggestion Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit deb7099) # Conflicts: # src/plugins/discover/public/application/apps/main/utils/get_sharing_data.ts

…n view (#123206) (#133506) * [Discover] fix relative time filter for csv export from discover main page * [Discover] fix array assignment * [Discover] fix functional test * [Discover] add test coverage for the issue * [Discover] add debug points for functional test * [Discover] try to get clipboard permissions * [Discover] fix functional test * [Discover] apply suggestion * [Discover] apply suggestion * [Discover] apply naming suggestion Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit deb7099) # Conflicts: # src/plugins/discover/public/application/apps/main/utils/get_sharing_data.ts # x-pack/test/functional/apps/discover/reporting.ts Co-authored-by: Dmitry Tomashevich <39378793+Dmitriynj@users.noreply.github.com>

kibanamachine · 2022-06-06T12:06:19Z

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

[Discover] fix relative time filter for csv export from discover main…

6f282a7

… page

dimaanj added Feature:Discover Discover Application release_note:fix v8.1.0 Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Jan 18, 2022

dimaanj self-assigned this Jan 18, 2022

dimaanj marked this pull request as ready for review January 18, 2022 12:24

dimaanj requested a review from a team as a code owner January 18, 2022 12:24

vadimkibana reviewed Jan 19, 2022

View reviewed changes

src/plugins/discover/public/utils/get_sharing_data.ts Outdated Show resolved Hide resolved

dimaanj added 2 commits January 19, 2022 15:07

[Discover] fix array assignment

b1c777d

[Discover] fix functional test

403132c

Merge branch 'main' into fix-csv-for-relative-timefilter

b5c36d9

kertal reviewed Jan 20, 2022

View reviewed changes

kibanamachine and others added 3 commits January 24, 2022 14:16

Merge branch 'main' into fix-csv-for-relative-timefilter

cb16297

Merge branch 'main' of https://github.com/elastic/kibana into fix-csv…

5666be9

…-for-relative-timefilter

[Discover] add test coverage for the issue

22c12c4

kibanamachine and others added 4 commits January 26, 2022 08:30

Merge branch 'main' into fix-csv-for-relative-timefilter

1199e7d

[Discover] add debug points for functional test

c914287

[Discover] try to get clipboard permissions

0565dd7

[Discover] fix functional test

762d51c

Merge branch 'main' into fix-csv-for-relative-timefilter

b2c7fbb

kertal requested a review from tsullivan January 28, 2022 09:45

Merge branch 'main' into fix-csv-for-relative-timefilter

23f21ef

tsullivan approved these changes Mar 16, 2022

View reviewed changes

[Discover] apply naming suggestion

135fb18

Merge branch 'main' into fix-csv-for-relative-timefilter

4198cb8

kertal self-requested a review March 16, 2022 06:33

kertal approved these changes Mar 16, 2022

View reviewed changes

dimaanj added auto-backport Deprecated - use backport:version if exact versions are needed v7.17.1 v8.1.1 and removed v8.1.0 labels Mar 16, 2022

dimaanj merged commit deb7099 into elastic:main Mar 16, 2022

kibanamachine mentioned this pull request Mar 16, 2022

[8.1] [Discover] Fix csv export with relative time filter from discover main view (#123206) #127847

Merged

dimaanj mentioned this pull request Mar 21, 2022

[Discover] Extend Elasticsearch query rule with search source based data fetching #124534

Merged

5 tasks

dimaanj mentioned this pull request Jun 3, 2022

[7.17] [Discover] Fix csv export with relative time filter from discover main view (#123206) #133506

Merged

dimaanj mentioned this pull request Jun 3, 2022

[8.0] [Discover] Fix csv export with relative time filter from discover main view (#123206) #133507

Closed

dimaanj added v7.17.5 v8.0.2 and removed v7.17.1 labels Jun 3, 2022

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 6, 2022

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Fix csv export with relative time filter from discover main view #123206

[Discover] Fix csv export with relative time filter from discover main view #123206

dimaanj commented Jan 18, 2022 •

edited

Loading

elasticmachine commented Jan 18, 2022

dimaanj commented Jan 20, 2022

kertal Jan 20, 2022

jloleysens Feb 3, 2022 •

edited

Loading

jloleysens Feb 3, 2022 •

edited

Loading

kertal Feb 23, 2022

dimaanj commented Jan 24, 2022

dimaanj commented Jan 26, 2022

dimaanj commented Jan 27, 2022

dimaanj commented Jan 31, 2022

tsullivan left a comment

dimaanj commented Mar 16, 2022

kibana-ci commented Mar 16, 2022

kertal left a comment

kibanamachine commented Mar 16, 2022

kibanamachine commented Jun 6, 2022

[Discover] Fix csv export with relative time filter from discover main view #123206

[Discover] Fix csv export with relative time filter from discover main view #123206

Conversation

dimaanj commented Jan 18, 2022 • edited Loading

Summary

Test notes

elasticmachine commented Jan 18, 2022

dimaanj commented Jan 20, 2022

kertal Jan 20, 2022

Choose a reason for hiding this comment

jloleysens Feb 3, 2022 • edited Loading

Choose a reason for hiding this comment

jloleysens Feb 3, 2022 • edited Loading

Choose a reason for hiding this comment

kertal Feb 23, 2022

Choose a reason for hiding this comment

dimaanj commented Jan 24, 2022

dimaanj commented Jan 26, 2022

dimaanj commented Jan 27, 2022

dimaanj commented Jan 31, 2022

tsullivan left a comment

Choose a reason for hiding this comment

dimaanj commented Mar 16, 2022

kibana-ci commented Mar 16, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

History

kertal left a comment

Choose a reason for hiding this comment

kibanamachine commented Mar 16, 2022

💔 Some backports could not be created

Manual backport

Questions ?

kibanamachine commented Jun 6, 2022

dimaanj commented Jan 18, 2022 •

edited

Loading

jloleysens Feb 3, 2022 •

edited

Loading

jloleysens Feb 3, 2022 •

edited

Loading