Include search idle info to shard stats #95740

salvatore-campagna · 2023-05-02T14:13:39Z

Extend the Index Stats API to include information about shard idleness.
To be more precise we include two pieces of info:

a boolean indicating if a shard is idle at the moment the API call takes place
a long value indicating the time in milliseconds the shard has been search idle since the last time it went into search idle state

Resolves #95727

elasticsearchmachine · 2023-05-02T14:17:46Z

Pinging @elastic/es-analytics-geo (Team:Analytics)

elasticsearchmachine · 2023-05-02T14:17:46Z

Pinging @elastic/es-search (Team:Search)

elasticsearchmachine · 2023-05-02T14:17:46Z

Hi @salvatore-campagna, I've created a changelog YAML for you.

elasticsearchmachine · 2023-05-02T14:25:49Z

Hi @salvatore-campagna, I've updated the changelog YAML for you.

The test checks just that the setting is applied, there is no waiting for the shard to go into search idle state.

salvatore-campagna · 2023-05-02T17:41:41Z

@elasticsearchmachine test this please

martijnvg

This looks good. I left a comment about where to place these new stats and testing.

martijnvg · 2023-05-03T12:39:32Z

server/src/internalClusterTest/java/org/elasticsearch/index/shard/SearchIdleIT.java

@@ -182,4 +200,27 @@ private void ensureNoPendingScheduledRefresh(ThreadPool threadPool) {
        barrier.arriveAndAwaitAdvance();
    }

+    public void testSearchIdleStats() throws InterruptedException {


Maybe also add a yaml test to rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.stats dir?
Just to verify that the new fields are included in the stats response

I was thinking about that even if the stats api sued at shard level returns a lot of data that I can't actually check with match. Also for search idle I can't check the values in the test...just that the fields exist.

.just that the fields exist.

and that is what I think we should test for. We can't assert what actual values are there, just the existence of the new fields.

martijnvg · 2023-05-03T12:40:49Z

server/src/internalClusterTest/java/org/elasticsearch/index/shard/SearchIdleIT.java

+            .prepareCreate(indexName)
+            .setSettings(
+                Settings.builder()
+                    .put(IndexSettings.INDEX_SEARCH_IDLE_AFTER.getKey(), searchIdleAfter + "s")


Maybe reduce this idle after to something like 100ms here? Otherwise it increases the time to run this test significantly.

martijnvg · 2023-05-03T12:48:32Z

server/src/main/java/org/elasticsearch/action/admin/indices/stats/ShardStats.java

@@ -211,6 +259,9 @@ static final class Fields {
        static final String PRIMARY = "primary";
        static final String NODE = "node";
        static final String RELOCATING_NODE = "relocating_node";
+        static final String SEARCH_IDLE = "search_idle";
+        static final String IS_SEARCH_IDLE = "is_idle";


I'm not really happy with search_idle object. Thinking more about this, maybe we should add these two fields to SearchStats?

martijnvg · 2023-05-10T08:59:18Z

server/src/internalClusterTest/java/org/elasticsearch/search/stats/SearchIdleStatsTests.java

+
+import java.util.concurrent.TimeUnit;
+
+public class SearchIdleStatsTests extends ESSingleNodeTestCase {


I think this either need to have a IT suffix or be moved to server/src/test/java?

martijnvg · 2023-05-10T09:39:03Z

server/src/main/java/org/elasticsearch/index/search/stats/SearchStats.java

@@ -316,6 +338,8 @@ public void add(SearchStats searchStats) {
                groupStats.get(entry.getKey()).add(entry.getValue());
            }
        }
+        isSearchIdle = searchStats.isSearchIdle;


If shards stats are collected in index or data stream level, this could hide shards that are search idle.
Maybe we should report sum or max idle time? Or maybe we should move this information back to ShardStats?

This reverts commit a72ecbd.

This reverts commit 51af680.

This reverts commit 632193a.

This reverts commit b7986e5.

martijnvg

Left one comment, LGTM otherwise.

martijnvg · 2023-05-11T07:39:32Z

server/src/main/java/org/elasticsearch/action/admin/indices/stats/ShardStats.java

@@ -211,6 +257,8 @@ static final class Fields {
        static final String PRIMARY = "primary";
        static final String NODE = "node";
        static final String RELOCATING_NODE = "relocating_node";
+        static final String IS_SEARCH_IDLE = "is_search_idle";


maybe just search_idle instead of is_search_idle? I think boolean fields don't usually get the is_ prefix. (see for example primary field)

This reverts commit f523caf.

This reverts commit f523caf. #95740 introduced a transport protocol change but using the old stack version transport versions. This breaks inter-commit backward compatibility. This pull request reverts this for now until we can go back and reintroduce the change using a new transport version.

mark-vieira · 2023-05-17T18:04:33Z

This PR got merged after the update to transport versions and mistakenly introduced a usage of an existing transport version. In this case the 8_9_0. We should have introduced a new transport version here. I've reverted this commit for now in #96204 since it breaks backwards compatibility.

Extend the Index Stats API to include information about shard idleness. To be more precise we include two pieces of info: * a boolean indicating if a shard is idle at the moment the API call takes place * a long value indicating the time in milliseconds the shard has been search idle since the last time it went into search idle state

Include search idle info to shard stats (#95740) Extend the Index Stats API to include information about shard idleness. To be more precise we include two pieces of info: * a boolean indicating if a shard is idle at the moment the API call takes place * a long value indicating the time in milliseconds the shard has been search idle since the last time it went into search idle state

feature: include search idle info to shard stats

1b9b293

elasticsearchmachine added needs:triage Requires assignment of a team area label v8.9.0 labels May 2, 2023

fix: remove unnecessary variables

ad1e040

salvatore-campagna self-assigned this May 2, 2023

Update docs/changelog/95740.yaml

dbcefda

salvatore-campagna and others added 5 commits May 2, 2023 16:25

Update docs/changelog/95740.yaml

bba65f4

fix: changelog area does not support array of values

b66bb7f

fix: remove check on idle time

c04195f

The test checks just that the setting is applied, there is no waiting for the shard to go into search idle state.

fix: udpate constructor invocation

44380fd

fix: udpate constructor invocation

db879e2

salvatore-campagna requested a review from martijnvg May 2, 2023 17:40

salvatore-campagna added the test-full-bwc Trigger full BWC version matrix tests label May 2, 2023

Merge branch 'main' into feature/95727-indices-stats-search-idle-time

562575f

martijnvg reviewed May 3, 2023

View reviewed changes

salvatore-campagna added 4 commits May 4, 2023 12:01

fix: move idle info to SearchStats

b7986e5

test: include a yaml test

83d7776

fix: remove check on search idle

632193a

fix: update yaml summary

51af680

salvatore-campagna added 2 commits May 4, 2023 14:27

fix: add missing skip

4d43908

fix: apply settings to the test index only

a72ecbd

martijnvg reviewed May 10, 2023

View reviewed changes

salvatore-campagna added 5 commits May 10, 2023 12:42

Revert "fix: apply settings to the test index only"

8015723

This reverts commit a72ecbd.

Revert "fix: update yaml summary"

be7810d

This reverts commit 51af680.

Revert "fix: remove check on search idle"

5e27ca6

This reverts commit 632193a.

Revert "fix: move idle info to SearchStats"

1caac8c

This reverts commit b7986e5.

fix: move search idle stats back into ShardStats

75d025c

salvatore-campagna requested a review from martijnvg May 10, 2023 21:57

martijnvg approved these changes May 11, 2023

View reviewed changes

fix: remove 'is' prefix

99a77ae

salvatore-campagna merged commit f523caf into elastic:main May 16, 2023

mark-vieira added a commit to mark-vieira/elasticsearch that referenced this pull request May 17, 2023

Revert "Include search idle info to shard stats (elastic#95740)"

caa4205

This reverts commit f523caf.

mark-vieira mentioned this pull request May 17, 2023

Revert "Include search idle info to shard stats (#95740)" #96204

Merged

salvatore-campagna mentioned this pull request May 18, 2023

Search idle info transport version bump #96213

Merged

salvatore-campagna mentioned this pull request May 22, 2023

Remove duplicated changelog #96241

Merged

DaveCTurner mentioned this pull request Jul 15, 2024

Expose search-idle shard state in shard stats API #110688

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include search idle info to shard stats #95740

Include search idle info to shard stats #95740

salvatore-campagna commented May 2, 2023 •

edited

Loading

elasticsearchmachine commented May 2, 2023

elasticsearchmachine commented May 2, 2023

elasticsearchmachine commented May 2, 2023

elasticsearchmachine commented May 2, 2023

salvatore-campagna commented May 2, 2023

martijnvg left a comment

martijnvg May 3, 2023

salvatore-campagna May 3, 2023

martijnvg May 4, 2023

martijnvg May 3, 2023

martijnvg May 3, 2023

martijnvg May 10, 2023

martijnvg May 10, 2023

martijnvg left a comment

martijnvg May 11, 2023

mark-vieira commented May 17, 2023


		import java.util.concurrent.TimeUnit;

		public class SearchIdleStatsTests extends ESSingleNodeTestCase {

Include search idle info to shard stats #95740

Include search idle info to shard stats #95740

Conversation

salvatore-campagna commented May 2, 2023 • edited Loading

elasticsearchmachine commented May 2, 2023

elasticsearchmachine commented May 2, 2023

elasticsearchmachine commented May 2, 2023

elasticsearchmachine commented May 2, 2023

salvatore-campagna commented May 2, 2023

martijnvg left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-vieira commented May 17, 2023

salvatore-campagna commented May 2, 2023 •

edited

Loading