Skip to content

Introduced monitoring mode for query string query max length.#19539

Merged
jainankitk merged 2 commits intoopensearch-project:mainfrom
lukasz-soszynski-eliatra:query-string-query-length-monitor-mode
Dec 9, 2025
Merged

Introduced monitoring mode for query string query max length.#19539
jainankitk merged 2 commits intoopensearch-project:mainfrom
lukasz-soszynski-eliatra:query-string-query-length-monitor-mode

Conversation

@lukasz-soszynski-eliatra
Copy link
Copy Markdown
Contributor

Description

The PR introduces "query string query" monitoring mode. The mode is by default disabled. If the system administrator enables the mode (using the configuration property search.query.max_query_string_length_monitor_only), then the OS executes queries that exceed the max length limit and logs an additional warning message.

The PR is related to the discussion

Related Issues

#19491 (comment)

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Lukasz Soszynski <lukasz.soszynski@eliatra.com>
@lukasz-soszynski-eliatra lukasz-soszynski-eliatra force-pushed the query-string-query-length-monitor-mode branch from ac1cdd7 to 377eca4 Compare October 6, 2025 14:31
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Oct 6, 2025

❌ Gradle check result for 377eca4: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@kkhatua kkhatua requested a review from jainankitk October 6, 2025 16:35
@kkhatua
Copy link
Copy Markdown
Member

kkhatua commented Oct 6, 2025

Tagging @cwperks , @jainankitk for a review.

@cwperks
Copy link
Copy Markdown
Member

cwperks commented Oct 7, 2025

@kkhatua I'm not really in favor of one-off settings like this. The max-length setting introduced in #19491 is already changeable dynamically.

What do you think about making any responses that exceed the configured max length to be actionable? i.e. the response should not just be that the request fails, but state the reason for failure and how to adjust the max length setting.

This is from one of the tests:

assertThat(e.getDetailedMessage(), containsString("Query string length exceeds max allowed length 10"));

Maybe we can specify the setting name in that message?

@kumargu
Copy link
Copy Markdown
Contributor

kumargu commented Oct 8, 2025

@kkhatua I'm not really in favor of one-off settings like this. The max-length setting introduced in #19491 is already changeable dynamically.

What do you think about making any responses that exceed the configured max length to be actionable? i.e. the response should not just be that the request fails, but state the reason for failure and how to adjust the max length setting.

This is from one of the tests:

assertThat(e.getDetailedMessage(), containsString("Query string length exceeds max allowed length 10"));

Maybe we can specify the setting name in that message?

yeah having a failure reason would be useful.

@kkhatua
Copy link
Copy Markdown
Member

kkhatua commented Oct 9, 2025

Agree on providing a failure reason with the settings to modify, similar to other settings.
However, the monitoring mode ensures this is an optional breaking change.

Tagging @jainankitk to review. Would be good to have this for 3.3 release, but it's not a must.

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@cwperks
Copy link
Copy Markdown
Member

cwperks commented Oct 9, 2025

Fixed the conflicts due to the CHANGELOG being cleared. The content of this change looks good to me. I was comparing this more to XContentConstraint where there is a max enforced in the Jackson library itself. Similar to Lucene BoolQuery with 1024 max clause count:

@kkhatua
Copy link
Copy Markdown
Member

kkhatua commented Nov 6, 2025

@jainankitk given that we're only supporting it as a boolean and not an IGNORE mode, I dont think this needs a revision as an enum.
If rest looks fine, can we go ahead with final review and approval?

@opensearch-trigger-bot
Copy link
Copy Markdown
Contributor

This PR is stalled because it has been open for 30 days with no activity.

@opensearch-trigger-bot opensearch-trigger-bot bot added the stalled Issues that have stalled label Dec 7, 2025
@jainankitk jainankitk added backport 3.4 Backport to 3.4 branch and removed stalled Issues that have stalled labels Dec 8, 2025
@jainankitk jainankitk closed this Dec 8, 2025
@jainankitk jainankitk reopened this Dec 8, 2025
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Dec 8, 2025

❌ Gradle check result for 25b7541: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Dec 9, 2025

❌ Gradle check result for 25b7541: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@jainankitk
Copy link
Copy Markdown
Contributor

Failed due to flaky test:

[Test Result](https://build.ci.opensearch.org/job/gradle-check/68449/testReport/) (1 failure / +1)

    [org.opensearch.remotestore.RestoreShallowSnapshotV2IT.testRestoreShallowSnapshotIndexAfterSnapshot {p0={"opensearch.experimental.feature.writable_warm_index.enabled":"true"}}](https://build.ci.opensearch.org/job/gradle-check/68449/testReport/junit/org.opensearch.remotestore/RestoreShallowSnapshotV2IT/testRestoreShallowSnapshotIndexAfterSnapshot__p0___opensearch_experimental_feature_writable_warm_index_enabled___true___/)

Retrying gradle check

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Dec 9, 2025

✅ Gradle check result for 25b7541: SUCCESS

@jainankitk jainankitk merged commit 325cf30 into opensearch-project:main Dec 9, 2025
86 of 92 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Dec 9, 2025
Signed-off-by: Lukasz Soszynski <lukasz.soszynski@eliatra.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Co-authored-by: Craig Perkins <cwperx@amazon.com>
(cherry picked from commit 325cf30)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
jainankitk pushed a commit that referenced this pull request Dec 9, 2025
#20195)

(cherry picked from commit 325cf30)

Signed-off-by: Lukasz Soszynski <lukasz.soszynski@eliatra.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Craig Perkins <cwperx@amazon.com>
fdesu pushed a commit to fdesu/OpenSearch that referenced this pull request Dec 13, 2025
…arch-project#19539)

Signed-off-by: Lukasz Soszynski <lukasz.soszynski@eliatra.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Co-authored-by: Craig Perkins <cwperx@amazon.com>
fdesu pushed a commit to fdesu/OpenSearch that referenced this pull request Dec 13, 2025
…arch-project#19539)

Signed-off-by: Lukasz Soszynski <lukasz.soszynski@eliatra.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Co-authored-by: Craig Perkins <cwperx@amazon.com>
liuguoqingfz pushed a commit to liuguoqingfz/OpenSearch that referenced this pull request Dec 15, 2025
…arch-project#19539)

Signed-off-by: Lukasz Soszynski <lukasz.soszynski@eliatra.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Co-authored-by: Craig Perkins <cwperx@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport 3.4 Backport to 3.4 branch v3.4.0 Issues and PRs related to version 3.4.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants