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

[Reporting] Add max concurrent shards setting to schema #170344

Merged
merged 19 commits into from Dec 5, 2023

Conversation

rshen91
Copy link
Contributor

@rshen91 rshen91 commented Nov 1, 2023

Summary

Closes #161561
This PR exposes the max_concurrent_shards setting in the schema for customers for point in time CSV report generation.

Checklist

@rshen91 rshen91 self-assigned this Nov 1, 2023
@rshen91 rshen91 added the Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) label Nov 1, 2023
@rshen91 rshen91 marked this pull request as ready for review November 13, 2023 15:42
@rshen91 rshen91 requested a review from a team as a code owner November 13, 2023 15:42
@elasticmachine
Copy link
Contributor

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

@rshen91 rshen91 added release_note:enhancement backport:all-open Backport to all branches that could still receive a release labels Nov 13, 2023
Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

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

LGTM 👍

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.

I think the maxConcurrentShardRequests option also needs to be used somewhere else in the generate_csv file.

@@ -75,6 +79,7 @@ export class CsvGenerator {
{
requestTimeout: duration,
maxRetries: 0,
maxConcurrentShardRequests,
Copy link
Member

@tsullivan tsullivan Nov 14, 2023

Choose a reason for hiding this comment

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

This also needs to be added to the transport field of the data.search() call in the doSearch() method. Doing that will throw a type compilation error, since the field isn't known in the data service. You'll have to reach out to them and make sure the field is recognized and correctly sent in the requests

I think the recommended way to go in doSearch would be to add the maxConcurrentShardRequests onto the searchSource object with:

private async doSearch(searchSource, settings, ...) {
  searchSource.setField('maxConcurrentShardRequests', settings.maxConcurrentShardRequests);
  ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Omg thank you good catch

Copy link
Member

@tsullivan tsullivan Nov 16, 2023

Choose a reason for hiding this comment

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

I did some searching around to find how this is handled in other parts of the code, and I think max_concurrent_shard_requests must be in the body, not the options.

      const response = await this.clients.es.asCurrentUser.openPointInTime(
        {
          // Note, this throws a TypeScript error, same as "ignore_throttled"
          // Check with the ES clients team to make sure this will work
          max_concurrent_shard_requests: maxConcurrentShardRequests,
          index: indexPatternTitle,
          keep_alive: duration,
          ignore_unavailable: true,
          // @ts-expect-error ignore_throttled is not in the type definition, but it is accepted by es
          ignore_throttled: includeFrozen ? false : undefined, // "true" will cause deprecation warnings logged in ES
        },
        {
          requestTimeout: duration,
          maxRetries: 0,
        }
      );

@rshen91 rshen91 requested review from a team as code owners November 16, 2023 19:16
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.

I checked through the type definitions and found other parts of the calls where max_concurrent_shard_requests should be plugged in.

@@ -121,6 +126,7 @@ export class CsvGenerator {
params: {
body: searchBody,
},
maxConcurrentShardRequests,
};
Copy link
Member

Choose a reason for hiding this comment

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

Based on other code I could find and the way the types are structured, it looks like max_concurrent_shard_requests should be deep one level further:

    const searchParams: IEsSearchRequest = {
      params: {
        body: searchBody,
        max_concurrent_shard_requests: maxConcurrentShardRequests,
      },
    };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

awesome! changes in bd31abe

Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -6,6 +6,7 @@
*/

import React, { useMemo, useCallback, useState } from 'react';
import { FormattedMessage } from '@kbn/i18n-react';
Copy link
Member

Choose a reason for hiding this comment

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

Maybe these changes made it into this PR from rebasing commits that included a merge commit. You can take this change out of your PR with:

git fetch elastic main
git checkout elastic/main x-pack/plugins/infra/

And that will reset the code under this directory back to the source in main.

@rshen91 rshen91 enabled auto-merge (squash) December 3, 2023 23:58
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

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @rshen91

@rshen91 rshen91 merged commit 7508ee1 into elastic:main Dec 5, 2023
34 checks passed
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
7.17 Backport failed because of merge conflicts
8.11 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 170344

Questions ?

Please refer to the Backport tool documentation

@tsullivan tsullivan removed the backport:all-open Backport to all branches that could still receive a release label Dec 5, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Dec 5, 2023
tsullivan added a commit that referenced this pull request May 7, 2024
…182536) (#182742)

# Backport

This will backport the following commits from `main` to `8.14`:
- [[Reporting/CSV] Resolve max_concurrent_shard_requests issue
(#182536)](#182536)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Tim
Sullivan","email":"tsullivan@users.noreply.github.com"},"sourceCommit":{"committedDate":"2024-05-03T14:30:45Z","message":"[Reporting/CSV]
Resolve max_concurrent_shard_requests issue (#182536)\n\n##
Summary\r\n\r\nThere has been a consistent failure in a Discover-related
test set in\r\nthe kibana-ES-serverless verification job, meaning that
ES-Kibana\r\ncompatibility has drifted.\r\n\r\nError details:\r\n```\r\n
+ \"Encountered an unknown error: status_exception\r\n + \tRoot
causes:\r\n + \t\tstatus_exception: Parameter validation failed for
[/_search]: The http parameter [max_concurrent_shard_requests] (with
value [5]) is not permitted when running in serverless mode\"\r\n +
\"Encountered an error with the number of CSV rows generated fromthe
search: expected rows were indeterminable, received 0.\"\r\n at
Context.<anonymous> (reporting.ts:182:33)\r\n at
processTicksAndRejections (node:internal/process/task_queues:95:5)\r\n
at Object.apply (wrap_function.js:73:16)\r\n```\r\n\r\nThis tracked back
to a feature added for reporting awhile back, which\r\ncreated a config
schema field for the `max_concurrent_shard_requests`\r\nparameter in the
search
queries:\r\nhttps://github.com//pull/170344/files#diff-7bceb37eef3761e1161cf04f41668dd9195bfac9fea36e734a230b5ed878a974\r\n\r\nMost
of the changes in this PR are in test code. I created \"Test\"
which\r\nextend protected methods in the original classes. This was done
to\r\nremove the `@ts-expect-errors` lines of
code.","sha":"f51c5c92bcc13686119ddf215eaa083ffac8e517","branchLabelMapping":{"^v8.15.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","Feature:Reporting","release_note:skip","backport:skip","Team:SharedUX","v8.15.0"],"number":182536,"url":"#182536
Resolve max_concurrent_shard_requests issue (#182536)\n\n##
Summary\r\n\r\nThere has been a consistent failure in a Discover-related
test set in\r\nthe kibana-ES-serverless verification job, meaning that
ES-Kibana\r\ncompatibility has drifted.\r\n\r\nError details:\r\n```\r\n
+ \"Encountered an unknown error: status_exception\r\n + \tRoot
causes:\r\n + \t\tstatus_exception: Parameter validation failed for
[/_search]: The http parameter [max_concurrent_shard_requests] (with
value [5]) is not permitted when running in serverless mode\"\r\n +
\"Encountered an error with the number of CSV rows generated fromthe
search: expected rows were indeterminable, received 0.\"\r\n at
Context.<anonymous> (reporting.ts:182:33)\r\n at
processTicksAndRejections (node:internal/process/task_queues:95:5)\r\n
at Object.apply (wrap_function.js:73:16)\r\n```\r\n\r\nThis tracked back
to a feature added for reporting awhile back, which\r\ncreated a config
schema field for the `max_concurrent_shard_requests`\r\nparameter in the
search
queries:\r\nhttps://github.com//pull/170344/files#diff-7bceb37eef3761e1161cf04f41668dd9195bfac9fea36e734a230b5ed878a974\r\n\r\nMost
of the changes in this PR are in test code. I created \"Test\"
which\r\nextend protected methods in the original classes. This was done
to\r\nremove the `@ts-expect-errors` lines of
code.","sha":"f51c5c92bcc13686119ddf215eaa083ffac8e517"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.15.0","labelRegex":"^v8.15.0$","isSourceBranch":true,"state":"MERGED","url":"#182536
Resolve max_concurrent_shard_requests issue (#182536)\n\n##
Summary\r\n\r\nThere has been a consistent failure in a Discover-related
test set in\r\nthe kibana-ES-serverless verification job, meaning that
ES-Kibana\r\ncompatibility has drifted.\r\n\r\nError details:\r\n```\r\n
+ \"Encountered an unknown error: status_exception\r\n + \tRoot
causes:\r\n + \t\tstatus_exception: Parameter validation failed for
[/_search]: The http parameter [max_concurrent_shard_requests] (with
value [5]) is not permitted when running in serverless mode\"\r\n +
\"Encountered an error with the number of CSV rows generated fromthe
search: expected rows were indeterminable, received 0.\"\r\n at
Context.<anonymous> (reporting.ts:182:33)\r\n at
processTicksAndRejections (node:internal/process/task_queues:95:5)\r\n
at Object.apply (wrap_function.js:73:16)\r\n```\r\n\r\nThis tracked back
to a feature added for reporting awhile back, which\r\ncreated a config
schema field for the `max_concurrent_shard_requests`\r\nparameter in the
search
queries:\r\nhttps://github.com//pull/170344/files#diff-7bceb37eef3761e1161cf04f41668dd9195bfac9fea36e734a230b5ed878a974\r\n\r\nMost
of the changes in this PR are in test code. I created \"Test\"
which\r\nextend protected methods in the original classes. This was done
to\r\nremove the `@ts-expect-errors` lines of
code.","sha":"f51c5c92bcc13686119ddf215eaa083ffac8e517"}}]}] BACKPORT-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:enhancement Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Reporting/CSV] Add setting for max_concurrent_shard_requests in CSV export
7 participants