Centralize telemetry listener wrapping in TransportSearchAction #134625

javanna · 2025-09-12T09:52:59Z

TransportSearchAction collects telemetry around CCS features used as well as took time of a search request. It does that via a action listener wrapping. The open PIT API uses TransportSearchAction for its execution, yet it does not wrap the listener in the same way, as search telemetry should not be affected by it. As a result, we have a TelemetryListener interface, with some instanceof checks in the code and a single implementation of such interface that is the action listener wrapper itself, which allows to record the telemetry on response.

This can be simplified by moving the listener wrapping to the TransportSearchAction code, and specialize a little the open PIT path so that telemetry can still be disabled for it. Along with the simplifications, this change allows us to delay the wrapping of the listener, which will allow us to grab the resolved indices in #134232 .

javanna · 2025-09-12T15:20:08Z

server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java

-    private class SearchResponseActionListener extends DelegatingActionListener<SearchResponse, SearchResponse>
-        implements
-            TelemetryListener {
+    private static class SearchTelemetryListener extends DelegatingActionListener<SearchResponse, SearchResponse> {


I made this static, it feels much better to be able to reason about its dependencies and not have it be tied to TransportSearchAction's lifecycle.

javanna · 2025-09-12T15:20:58Z

server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java


-    private interface TelemetryListener {


Given I moved the wrapping inside of executeSearch, this interface is no longer required. It was there for open PIT only as it provides a different type of listener.

javanna · 2025-09-12T15:21:52Z

server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java

+        SearchRequest original,
+        ActionListener<SearchResponse> originalListener,
+        Function<ActionListener<SearchResponse>, SearchPhaseProvider> searchPhaseProvider,
+        boolean collectSearchTelemetry


This isn't perfect, but it gets the job done. Ideally, open PIT would have its own transport action, but I am not even sure that's a change worth making given its complexity.

Before this change this flag was not needed because the listener had all the telemetry logic encapsulated in it. The downside was that we needed to do some instanceof checks on it. Another issue I found with that is that it does not allow to store resolved indices (without carrying a reference to the search request in the listener).

…open_pit

javanna · 2025-09-12T15:34:31Z

server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java

@@ -2069,7 +2072,7 @@ public void onResponse(SearchResponse searchResponse) {
                }
                searchResponseMetrics.incrementResponseCount(responseCountTotalStatus);

-                if (collectTelemetry()) {
+                if (collectCCSTelemetry()) {


I renamed this to avoid confusion, as this flag controls only whether we record CCS specific telemetry.

elasticsearchmachine · 2025-09-12T15:58:33Z

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

server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java

smalyshev · 2025-09-12T19:12:20Z

server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java

            if (ccsCheckCompatibility) {
                checkCCSVersionCompatibility(rewritten);
            }

+            final CCSUsage.Builder ccsUsageBuilder = new CCSUsage.Builder();


It looks like we always create and update ccsUsageBuilder but only use it if collectSearchTelemetry is true?

yes I was a bit lazy here, I will iterate over this once more.

This was not so different from how things already were: a CCS usage builder always got created despite we may not be collecting CCS telemetry. I did not like that my change made things even harder to read. I tied the creation of the listener to the telemetry collection now, so that we only create the usage builder when necessary, and the code should be easier to follow.

smalyshev · 2025-09-12T19:16:10Z

server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java

-                // We set the keep alive to -1 to indicate that we don't need the pit id in the response.
-                // This is needed since we delete the pit prior to sending the response so the id doesn't exist anymore.
-                source.pointInTimeBuilder(new PointInTimeBuilder(resp.getPointInTimeId()).setKeepAlive(TimeValue.MINUS_ONE));
-                var pitListener = new SearchResponseActionListener(delegate) {


One thing I notice here that SearchResponseActionListener also collects SearchResponseMetrics. But if we convert this to just ActionListener this doesn't happen anymore? Is that intended?

that's a good observation. Yet onResponse is entirely rewritten here, hence SearchReponseActionListener in this case does not do anything anyways. Using a plain ActionListener does not cause any change in behaviour. I added a test around this though that was missing: this is triggered e.g. when a compound retriever is used, which opens a PIT under the hood and performs a search against it (executed as an async action).

I also moved up the shouldOpenPIT conditional branch, as it can quickly shortcut execution as opposed to creating the rewriteListener for nothing when a PIT needs to be opened. I think this improved readability, let me know.

smalyshev · 2025-09-12T19:17:28Z

The approach looks fine to me but have some question about particular details.

javanna · 2025-09-15T09:43:26Z

server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java

+                })
+            );
+            return;
+        }


the only change in this block is no longer using SearchResponseActionListener, which was not necessary (beyond my changes - added a test to verify that). This block can shortcut execution hence I moved it up.

andreidan

Thanks for working on this Luca.

The context comments helped the review ! 🚀

andreidan · 2025-09-15T10:58:27Z

...er/src/test/java/org/elasticsearch/search/TelemetryMetrics/SearchTookTimeTelemetryTests.java

@@ -85,6 +90,29 @@ public void testSimpleQuery() {
        assertEquals(searchResponse.getTook().millis(), measurements.getFirst().getLong());
    }

+    public void testCompoundRetriever() {


very nice !

…open_pit

Delay SearchResponseActionListener wrapping to a single location

c07deb2

elasticsearchmachine added the v9.2.0 label Sep 12, 2025

iter

a44bb23

javanna commented Sep 12, 2025

View reviewed changes

javanna changed the title ~~Delay SearchResponseActionListener wrapping to a single location~~ Centralize telemetry listener wrapping in TransportSearchAction Sep 12, 2025

javanna marked this pull request as ready for review September 12, 2025 15:31

Merge branch 'main' into enhancement/search_response_action_listener_…

e64a7b1

…open_pit

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Sep 12, 2025

javanna mentioned this pull request Sep 12, 2025

Add relevant attributes to search took time APM metrics #134232

Merged

javanna commented Sep 12, 2025

View reviewed changes

javanna added :Search Foundations/Search Catch all for Search Foundations >refactoring and removed needs:triage Requires assignment of a team area label labels Sep 12, 2025

javanna requested a review from smalyshev September 12, 2025 15:58

elasticsearchmachine added the Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch label Sep 12, 2025

smalyshev reviewed Sep 12, 2025

View reviewed changes

javanna added 2 commits September 15, 2025 11:36

iter

0b9936f

iter

176d7bf

javanna commented Sep 15, 2025

View reviewed changes

tteofili approved these changes Sep 15, 2025

View reviewed changes

andreidan approved these changes Sep 15, 2025

View reviewed changes

Merge branch 'main' into enhancement/search_response_action_listener_…

9b9dad8

…open_pit

javanna merged commit 1f4abac into elastic:main Sep 15, 2025
35 checks passed

javanna deleted the enhancement/search_response_action_listener_open_pit branch September 15, 2025 13:12

Centralize telemetry listener wrapping in TransportSearchAction #134625

Centralize telemetry listener wrapping in TransportSearchAction #134625

Conversation

javanna commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Sep 12, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smalyshev commented Sep 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreidan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

javanna commented Sep 12, 2025 •

edited

Loading