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

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

Merged
merged 21 commits into from
Mar 16, 2022
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
6f282a7
[Discover] fix relative time filter for csv export from discover main…
dimaanj Jan 18, 2022
b1c777d
[Discover] fix array assignment
dimaanj Jan 19, 2022
403132c
[Discover] fix functional test
dimaanj Jan 20, 2022
b5c36d9
Merge branch 'main' into fix-csv-for-relative-timefilter
kibanamachine Jan 20, 2022
cb16297
Merge branch 'main' into fix-csv-for-relative-timefilter
kibanamachine Jan 24, 2022
5666be9
Merge branch 'main' of https://github.com/elastic/kibana into fix-csv…
dimaanj Jan 25, 2022
22c12c4
[Discover] add test coverage for the issue
dimaanj Jan 25, 2022
1199e7d
Merge branch 'main' into fix-csv-for-relative-timefilter
kibanamachine Jan 26, 2022
c914287
[Discover] add debug points for functional test
dimaanj Jan 26, 2022
0565dd7
[Discover] try to get clipboard permissions
dimaanj Jan 26, 2022
762d51c
[Discover] fix functional test
dimaanj Jan 27, 2022
b2c7fbb
Merge branch 'main' into fix-csv-for-relative-timefilter
kibanamachine Jan 27, 2022
23f21ef
Merge branch 'main' into fix-csv-for-relative-timefilter
kibanamachine Jan 31, 2022
26b0493
Merge branch 'main' into fix-csv-for-relative-timefilter
kibanamachine Feb 14, 2022
25a87bb
Merge branch 'main' into fix-csv-for-relative-timefilter
kibanamachine Feb 22, 2022
2831c1c
Merge branch 'main' into fix-csv-for-relative-timefilter
kibanamachine Mar 9, 2022
2040d86
Merge branch 'main' into fix-csv-for-relative-timefilter
kibanamachine Mar 14, 2022
17927ed
[Discover] apply suggestion
dimaanj Mar 14, 2022
d1a345a
[Discover] apply suggestion
dimaanj Mar 15, 2022
135fb18
[Discover] apply naming suggestion
dimaanj Mar 16, 2022
4198cb8
Merge branch 'main' into fix-csv-for-relative-timefilter
kibanamachine Mar 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/plugins/discover/public/application/main/services/discover_state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ export function setState(stateContainer: ReduxLikeStateContainer<AppState>, newS
/**
* Helper function to compare 2 different filter states
*/
export function isEqualFilters(filtersA: Filter[], filtersB: Filter[]) {
export function isEqualFilters(filtersA?: Filter[] | Filter, filtersB?: Filter[] | Filter) {
if (!filtersA && !filtersB) {
return true;
} else if (!filtersA || !filtersB) {
Expand Down
16 changes: 13 additions & 3 deletions src/plugins/discover/public/utils/get_sharing_data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import type { Filter, ISearchSource, SerializedSearchSourceFields } from 'src/pl
import { DOC_HIDE_TIME_COLUMN_SETTING, SORT_DEFAULT_ORDER_SETTING } from '../../common';
import type { SavedSearch, SortOrder } from '../services/saved_searches';
import { getSortForSearchSource } from '../components/doc_table';
import { AppState } from '../application/main/services/discover_state';
import { AppState, isEqualFilters } from '../application/main/services/discover_state';

/**
* Preparing data to share the current state as link or CSV/Report
Expand All @@ -26,7 +26,7 @@ export async function getSharingData(
const { uiSettings: config, data } = services;
const searchSource = currentSearchSource.createCopy();
const index = searchSource.getField('index')!;
const existingFilter = searchSource.getField('filter');
let existingFilter = searchSource.getField('filter') as Filter[] | Filter | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

@ppisljar we could simplify consumer code by always returning Filter[] from getField('filter') instead of Filter[] | Filter | (() => Filter[] | Filter | undefined). WDYT?

It may have to be Filter[] | (() => Filter[]). Still, currently seems like a lot of things to check 😅


searchSource.setField(
'sort',
Expand Down Expand Up @@ -56,10 +56,20 @@ export async function getSharingData(

return {
getSearchSource: (absoluteTime?: boolean): SerializedSearchSourceFields => {
const absoluteFilter = data.query.timefilter.timefilter.createFilter(index);
kertal marked this conversation as resolved.
Show resolved Hide resolved
const timeFilter = absoluteTime
? data.query.timefilter.timefilter.createFilter(index)
? absoluteFilter
Copy link
Member

Choose a reason for hiding this comment

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

Ok, I think I can suggest a way to omit any potential race condition:

So I'd suggest moving retrieving absolute and relative time filter to L64

const absoluteTimeFilter = data.query.timefilter.timefilter.createFilter(index);
const relativeTimeFilter = data.query.timefilter.timefilter.createRelativeFilter(index);

And then depending on absoluteTime use it like this on L70

const timeFilter = absoluteTime
        ? absoluteTimeFilter
        : relativeTimeFilter;

this would prevent the race condition when auto refresh is on and refreshing before getSearchSource is called .. because then absolute time filter would return a different result when called by getSearchSource

: 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;
}

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@jloleysens jloleysens Feb 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@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:

  1. Share data is requested for immediate use (e.g. CSV of exactly now)
  2. 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?

Copy link
Contributor

@jloleysens jloleysens Feb 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

if (existingFilter && timeFilter) {
searchSource.setField(
'filter',
Expand Down