Fix csv generation when `search:includeFrozen` is enabled #152354

Dosant · 2023-02-28T14:54:08Z

Summary

Since it isn't possible to freeze an index in 8.x to test you have to create an index and freeze it in 7.x and then upgrade to 8.x.

Dosant · 2023-02-28T14:56:06Z

@tsullivan, is this what you had in mind here? #151546 (comment)

Dosant · 2023-02-28T16:27:58Z

x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts

@@ -66,6 +66,8 @@ export class CsvGenerator {
          index: indexPatternTitle,
          keep_alive: duration,
          ignore_unavailable: true,
+          // @ts-ignore
+          ignore_throttled: settings.includeFrozen ? false : undefined, // "true" will cause deprecation warnings logged in ES


This is strange, but in my testing I found that this option doesn't seem to do anything when used with PIT. It seems like when PIT is used the search always includes frozen indices.

This leads to the situation that when include frozen is false (default), discover won't show them, but csv will

Hm, is there not a way to ignore frozen indices when using PIT? I don't see that mentioned in the docs. Can we run this by the ES search team?

I've reached out to the ES team, to get ideas why this happens.

Unfortunately, the change in the PR doesn't really seem to do anything, and has the side-effect of adding a ts-ignore line, since ignore_throttled isn't part of the OpenPointInTimeRequest interface. I believe we can probably drop awareness of the includeFrozen setting from this code, to resolve the bug and not add the ts-ignore line.

Let's keep this here, but link to elastic/elasticsearch#94377 after the ts-ignore, so anyone reading the code can understand why it is ignored. When the ES issue is resolved, the functionality should just work as the user intends, and at that point we might be able to just remove the ts-ignore as part of minor cleanup.

I have merged elastic/elasticsearch#94377. I think you can remove this workaround.

Opened to clean up: #153359

elasticmachine · 2023-02-28T16:28:58Z

Pinging @elastic/appex-sharedux (Team:SharedUX)

tsullivan · 2023-03-01T22:14:00Z

@tsullivan, is this what you had in mind here? #151546 (comment)

I think this is the right approach for the bug. Apologies that I wrote a different proposal earlier on - but I think these changes highlight that SearchSource is not putting the ignore_throttled indices option "automatically" - rather it really is controlled in the CSV Generation module.

tsullivan

Let's try to understand if always including frozen indices with PIT is intentional

tsullivan · 2023-03-01T22:18:26Z

x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts

@@ -66,6 +66,8 @@ export class CsvGenerator {
          index: indexPatternTitle,
          keep_alive: duration,
          ignore_unavailable: true,
+          // @ts-ignore
+          ignore_throttled: settings.includeFrozen ? false : undefined, // "true" will cause deprecation warnings logged in ES


Hm, is there not a way to ignore frozen indices when using PIT? I don't see that mentioned in the docs. Can we run this by the ES search team?

tsullivan

The ignore_throttled isn't part of the OpenPointInTimeRequest interface, which indicates we probably don't need to handle it at all. But let's wait to hear what the ES team has to say.

tsullivan · 2023-03-06T21:41:49Z

x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts

@@ -66,6 +66,8 @@ export class CsvGenerator {
          index: indexPatternTitle,
          keep_alive: duration,
          ignore_unavailable: true,
+          // @ts-ignore
+          ignore_throttled: settings.includeFrozen ? false : undefined, // "true" will cause deprecation warnings logged in ES


I've reached out to the ES team, to get ideas why this happens.

Unfortunately, the change in the PR doesn't really seem to do anything, and has the side-effect of adding a ts-ignore line, since ignore_throttled isn't part of the OpenPointInTimeRequest interface. I believe we can probably drop awareness of the includeFrozen setting from this code, to resolve the bug and not add the ts-ignore line.

Dosant · 2023-03-07T11:55:23Z

@tsullivan

I believe we can probably drop awareness of the includeFrozen setting from this code, to resolve the bug and not add the ts-ignore line.

Yes, agree, this would be better if there is no intention to change the behavior on es side

The ignore_throttled isn't part of the OpenPointInTimeRequest interface, which indicates we probably don't need to handle it at all. But let's wait to hear what the ES team has to say.

it isn't part of the interface but es accepts as a valid option inside the request. So there is a validation level that allows the option, but the internal implementation doesn't seem to be using it.

tsullivan

LGTM, but first let's add a way to track the issue in ES by linking to elastic/elasticsearch#94377

tsullivan · 2023-03-07T21:58:30Z

x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts

@@ -66,6 +66,8 @@ export class CsvGenerator {
          index: indexPatternTitle,
          keep_alive: duration,
          ignore_unavailable: true,
+          // @ts-ignore
+          ignore_throttled: settings.includeFrozen ? false : undefined, // "true" will cause deprecation warnings logged in ES


Let's keep this here, but link to elastic/elasticsearch#94377 after the ts-ignore, so anyone reading the code can understand why it is ignored. When the ES issue is resolved, the functionality should just work as the user intends, and at that point we might be able to just remove the ts-ignore as part of minor cleanup.

…x-frozen-csv

Dosant · 2023-03-08T09:28:23Z

Thanks @tsullivan, I added a comment and link to the issue: #152884

kibana-ci · 2023-03-08T10:38:26Z

💚 Build Succeeded

Buildkite Build
Commit: 4b79952

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`securitySolution`	428	430	+2

Total ESLint disabled count

id	before	after	diff
`securitySolution`	505	507	+2

History

💚 Build #111073 succeeded 0638860

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

…2354) fix elastic#151546 see: elastic#151546 (comment) Since it isn't possible to freeze an index in 8.x to test you have to create an index and freeze it in 7.x and then upgrade to 8.x. (cherry picked from commit 00e0f5c)

kibanamachine · 2023-03-08T12:31:36Z

💚 All backports created successfully

Status	Branch	Result
✅	8.7

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

Questions ?

Please refer to the Backport tool documentation

) (#152901) # Backport This will backport the following commits from `main` to `8.7`: - [Fix csv generation when `search:includeFrozen` is enabled (#152354)](#152354)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Anton Dosov <[email protected]>

…2354) fix elastic#151546 see: elastic#151546 (comment) Since it isn't possible to freeze an index in 8.x to test you have to create an index and freeze it in 7.x and then upgrade to 8.x.

fix frozen csv

0638860

Dosant commented Feb 28, 2023

View reviewed changes

Dosant added (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) labels Feb 28, 2023

Dosant requested a review from tsullivan February 28, 2023 16:28

Dosant marked this pull request as ready for review February 28, 2023 16:28

Dosant requested a review from a team as a code owner February 28, 2023 16:28

Dosant mentioned this pull request Mar 1, 2023

CSV Export does not work with the search:includeFrozen: true setting #151546

Closed

tsullivan reviewed Mar 1, 2023

View reviewed changes

tsullivan reviewed Mar 6, 2023

View reviewed changes

tsullivan approved these changes Mar 7, 2023

View reviewed changes

Dosant added 2 commits March 8, 2023 10:20

Merge branch 'main' of github.com:elastic/kibana into d/2023-02-28-fi…

7c0f85b

…x-frozen-csv

add a comment

4b79952

Dosant added release_note:fix backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Mar 8, 2023

Dosant merged commit 00e0f5c into elastic:main Mar 8, 2023

kibanamachine added the v8.8.0 label Mar 8, 2023

kibanamachine mentioned this pull request Mar 8, 2023

[8.7] Fix csv generation when search:includeFrozen is enabled (#152354) #152901

Merged

kibanamachine added the v8.7.0 label Mar 8, 2023

vadimkibana assigned Dosant Mar 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix csv generation when `search:includeFrozen` is enabled #152354

Fix csv generation when `search:includeFrozen` is enabled #152354

Dosant commented Feb 28, 2023 •

edited by kibanamachine

Loading

Dosant commented Feb 28, 2023

Dosant Feb 28, 2023

tsullivan Mar 1, 2023

tsullivan Mar 6, 2023

tsullivan Mar 7, 2023

dnhatn Mar 8, 2023

Dosant Mar 21, 2023

elasticmachine commented Feb 28, 2023

tsullivan commented Mar 1, 2023

tsullivan left a comment

tsullivan Mar 1, 2023

tsullivan left a comment

tsullivan Mar 6, 2023

Dosant commented Mar 7, 2023

tsullivan left a comment

tsullivan Mar 7, 2023

Dosant commented Mar 8, 2023

kibana-ci commented Mar 8, 2023

ESLint disabled line counts

Total ESLint disabled count

kibanamachine commented Mar 8, 2023

Fix csv generation when search:includeFrozen is enabled #152354

Fix csv generation when search:includeFrozen is enabled #152354

Conversation

Dosant commented Feb 28, 2023 • edited by kibanamachine Loading

Summary

Dosant commented Feb 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Feb 28, 2023

tsullivan commented Mar 1, 2023

tsullivan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsullivan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dosant commented Mar 7, 2023

tsullivan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dosant commented Mar 8, 2023

kibana-ci commented Mar 8, 2023

💚 Build Succeeded

Metrics [docs]

ESLint disabled line counts

Total ESLint disabled count

History

kibanamachine commented Mar 8, 2023

💚 All backports created successfully

Questions ?

Fix csv generation when `search:includeFrozen` is enabled #152354

Fix csv generation when `search:includeFrozen` is enabled #152354

Dosant commented Feb 28, 2023 •

edited by kibanamachine

Loading