Add profiling plugin #91640

danielmitterdorfer · 2022-11-17T07:00:55Z

With this commit we add a new X-Pack plugin that retrieves data for Universal Profiler. This functionality is currently implemented in Kibana in the function getExecutablesAndStackTraces which we replicate here for efficiency purposes. The plugin is off by default and can be enabled by setting xpack.profiling.enabled to true.

With this commit we add a new plugin that retrieves data for Universal Profiler. This functionality is currently implemented in Kibana in the function `getExecutablesAndStackTraces` which we replicate here for efficiency purposes.

With this commit we keep the transport action enabled. This is to ensure operator privileges can be checked in all cases (see OperatorPrivilegesIT.testEveryActionIsEitherOperatorOnlyOrNonOperator).

elasticsearchmachine · 2022-11-23T08:14:24Z

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

elasticsearchmachine · 2022-11-23T08:27:23Z

Hi @danielmitterdorfer, I've created a changelog YAML for you.

danielmitterdorfer · 2022-11-23T09:38:13Z

The test failure on elasticsearch-ci/part-1 is unrelated to this PR and also reproduces on main. I've raised #91837 and pushed #91838 to mute the test.

javanna

I left a couple of comments and questions, nothing major. This looks pretty solid and well tested to me, thanks @danielmitterdorfer .

I would ask @nik9000 to double check the aggs bits in TransportProfilingAction and then we are good?

...gin/profiler/src/main/java/org/elasticsearch/xpack/profiler/TransportGetProfilingAction.java

x-pack/plugin/profiler/src/main/java/org/elasticsearch/xpack/profiler/utils/MapExtractor.java

...plugin/profiler/src/test/java/org/elasticsearch/xpack/profiler/GetProfilingRequestTests.java

...lugin/profiler/src/test/java/org/elasticsearch/xpack/profiler/GetProfilingResponseTests.java

x-pack/plugin/profiler/src/test/java/org/elasticsearch/xpack/profiler/StackFrameTests.java

x-pack/plugin/profiler/src/main/java/org/elasticsearch/xpack/profiler/StackFrame.java

x-pack/plugin/profiler/src/main/java/org/elasticsearch/xpack/profiler/StackTrace.java

...gin/profiler/src/main/java/org/elasticsearch/xpack/profiler/TransportGetProfilingAction.java

danielmitterdorfer · 2022-11-28T09:44:15Z

Thanks for the review @javanna! I addressed your feedback in c21262b. I'm uncertain how to use AbstractXContentSerializingTestCase in my setup (see my comment above). Also, I clarified that we currently intend that this plugin does not leverage CCS but I'm uncertain what is required to support it (see my other comment). Could you please take another look?

javanna

I left a couple more questions/suggestions, LGTM otherwise

x-pack/plugin/profiler/src/main/java/org/elasticsearch/xpack/profiler/GetProfilingRequest.java

javanna · 2022-11-28T19:28:21Z

...k/plugin/profiler/src/main/java/org/elasticsearch/xpack/profiler/RestGetProfilingAction.java

+
+        return channel -> {
+            RestStatusToXContentListener<GetProfilingResponse> listener = new RestStatusToXContentListener<>(channel);
+            RestCancellableNodeClient cancelClient = new RestCancellableNodeClient(client, request.getHttpChannel());


It could be good to test the automatic cancellation (on channel closure), especially that all children tasks are cancelled too. We have a similar test for the search API: SearchRestCancellationIT .

Thanks for the pointer! I'll look into that.

I've added a test now in b445b6d.

...gin/profiler/src/main/java/org/elasticsearch/xpack/profiler/TransportGetProfilingAction.java

javanna · 2022-11-30T13:39:28Z

...iler/src/internalClusterTest/java/org/elasticsearch/xpack/profiler/GetProfilingActionIT.java

+            TaskManager taskManager = internalCluster().getInstance(TransportService.class, nodeName).getTaskManager();
+            Task task = taskManager.getTask(taskId.getId());
+            assertThat(task, instanceOf(CancellableTask.class));
+            assertTrue(((CancellableTask) task).isCancelled());


Should we also check that all children tasks are cancelled? My understanding is that we are now checking that the top-level profiling action is cancelled. Ideally, we'd make sure that the children search tasks either don't start before the cancellation, or get cancelled, or complete before the cancellation.

We could check here that there are no tasks with this specific parent task id, on the other hand that would be the case even if we did not set the parent task id properly. Should we instead try and collect those children tasks before the cancellation and double check that they do get cancelled afterwards?

I am not entirely sure though where things get blocked with the script below, maybe always at the first inner search request? ideally it would block at any stage but that sounds like it would complicate this test quite a bit?

Good catch! I've implemented this now in 55c158a.

looks great!

Thanks! I'll merge it then when CI is green. :)

* upstream/main: (209 commits) Remove unused methods and classes from HLRC (elastic#92030) Clean up on exception while chunking XContent (elastic#92024) Add profiling plugin (elastic#91640) Remove unused methods and classes from HLRC (elastic#92012) Remove IndexerState from HLRC (elastic#92023) Ensure cached time elapses in ClusterServiceIT (elastic#91986) Chunked encoding for RestGetIndicesAction (elastic#92016) Simplify shardsWithState (elastic#91991) [DOCS] Updates ML decider docs by mentioning CPU as scaling criterion (elastic#92018) Add chunking to ClusterState.Custom impls (elastic#91963) Speedup time_series agg by caching current tsid ordinal, parent bucket ordinal and buck ordinal (elastic#91784) Drop the ingest listener call count tracking (elastic#92003) [DOCS] fixes issue number 91889 - missing [discrete] header (elastic#91976) Fix PersistentTasksClusterServiceTests (elastic#92002) [docs] Update search-settings documentation to reflect the fact that the indices.query.bool.max_clause_count setting has been deprecated (elastic#91811) Clarify writability in Netty4HttpPipeliningHandler (elastic#91982) Load stable plugins as synthetic modules (elastic#91869) Handle any exception thrown while generating source for an IngestDocument (elastic#91981) fixing Apache HttpHost url on java-rest doc (elastic#91945) Implement repair functionality for aliases colliding with indices bug (elastic#91887) ...

With this commit we use `profiling-events-all` to query profiling events if the appropriate index for a sample count is not present yet. This can happen on cluster startup when only a few events have been accumulated because additional indices are only created when there are enough events. This aligns behavior of the Elasticsearch plugin with the Kibana plugin. Relates elastic#91640

With this commit we use `profiling-events-all` to query profiling events if the appropriate index for a sample count is not present yet. This can happen on cluster startup when only a few events have been accumulated because additional indices are only created when there are enough events. This aligns behavior of the Elasticsearch plugin with the Kibana plugin. We also refactor the profiling-related test cases so we can simulate situations after cluster startup when not all profiling-related indices have been created. As testing also cancellation would require additional complexity and decreases test maintainability (need to adjust `slack` based on the scenario which is a very low-level change), we also separate tests for the regular case and cancellation. Relates #91640

Originally the profiling plugin required to set `search.max_buckets` to a higher value than the current default. Had we enabled the profiling plugin by default, we would have changed the default value of that setting for all clusters implicitly. Due to a different bootstrapping process of the profiling solution, we do not require to change any settings anymore in the plugin. With this commit we therefore enable the plugin by default. Relates elastic#91640

Originally the profiling plugin required to set `search.max_buckets` to a higher value than the current default. Had we enabled the profiling plugin by default, we would have changed the default value of that setting for all clusters implicitly. Due to a different bootstrapping process of the profiling solution, we do not require to change any settings anymore in the plugin. With this commit we therefore enable the plugin by default. Relates #91640

Add profiling plugin

178f6da

With this commit we add a new plugin that retrieves data for Universal Profiler. This functionality is currently implemented in Kibana in the function `getExecutablesAndStackTraces` which we replicate here for efficiency purposes.

danielmitterdorfer self-assigned this Nov 17, 2022

elasticsearchmachine added the v8.7.0 label Nov 17, 2022

danielmitterdorfer added 7 commits November 17, 2022 08:34

Profiling API is a non-operator API

168eb31

More tests

1f6ac55

Rename profiler package

a3989ad

Add IT

cdc8331

Formatting fixes

6b0ed41

More tests

59a239f

Add flag xpack.profiling.enabled

5c37833

danielmitterdorfer marked this pull request as ready for review November 22, 2022 13:56

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Nov 22, 2022

Don't disable transport action

df47f95

With this commit we keep the transport action enabled. This is to ensure operator privileges can be checked in all cases (see OperatorPrivilegesIT.testEveryActionIsEitherOperatorOnlyOrNonOperator).

danielmitterdorfer added >enhancement Team:Search Meta label for search team and removed needs:triage Requires assignment of a team area label labels Nov 23, 2022

elasticsearchmachine added needs:triage Requires assignment of a team area label and removed Team:Search Meta label for search team labels Nov 23, 2022

danielmitterdorfer added the :Search/Search Search-related issues that do not fall into other categories label Nov 23, 2022

elasticsearchmachine added Team:Search Meta label for search team and removed needs:triage Requires assignment of a team area label labels Nov 23, 2022

Update docs/changelog/91640.yaml

883cf46

Merge remote-tracking branch 'origin/main' into profiler-plugin

907d5a5

Merge remote-tracking branch 'origin/main' into profiler-plugin

f6ae666

nik9000 requested review from nik9000 and dnhatn November 23, 2022 16:11

javanna removed the request for review from dnhatn November 24, 2022 13:07

javanna self-requested a review November 24, 2022 13:07

javanna reviewed Nov 24, 2022

View reviewed changes

Address review feedback

c21262b

javanna approved these changes Nov 29, 2022

View reviewed changes

danielmitterdorfer added 5 commits November 29, 2022 16:24

Implement GetProfilingResponseTests as AbstractWireSerializingTestCase

84f1acf

Remove obsolete trace context

b86f885

Use applyContentParser for request body parsing

62b9fc8

Merge remote-tracking branch 'origin/main' into profiler-plugin

a9caddf

Add cancellation test

b445b6d

javanna reviewed Nov 30, 2022

View reviewed changes

Verify task hierarchy is cancelled

55c158a

danielmitterdorfer merged commit 2b472c9 into elastic:main Nov 30, 2022

danielmitterdorfer deleted the profiler-plugin branch November 30, 2022 16:56

arteam mentioned this pull request Dec 1, 2022

[CI] GetProfilingActionIT.testAutomaticCancellation #92039

Closed

danielmitterdorfer mentioned this pull request Dec 5, 2022

Use all profiling events on startup #92087

Merged

danielmitterdorfer mentioned this pull request Jan 10, 2023

Enable profiling plugin by default #92787

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add profiling plugin #91640

Add profiling plugin #91640

danielmitterdorfer commented Nov 17, 2022 •

edited

Loading

elasticsearchmachine commented Nov 23, 2022

elasticsearchmachine commented Nov 23, 2022

danielmitterdorfer commented Nov 23, 2022 •

edited

Loading

javanna left a comment

danielmitterdorfer commented Nov 28, 2022 •

edited

Loading

javanna left a comment

javanna Nov 28, 2022

danielmitterdorfer Nov 29, 2022

danielmitterdorfer Nov 30, 2022

javanna Nov 30, 2022 •

edited

Loading

javanna Nov 30, 2022

danielmitterdorfer Nov 30, 2022

javanna Nov 30, 2022

danielmitterdorfer Nov 30, 2022

Add profiling plugin #91640

Add profiling plugin #91640

Conversation

danielmitterdorfer commented Nov 17, 2022 • edited Loading

elasticsearchmachine commented Nov 23, 2022

elasticsearchmachine commented Nov 23, 2022

danielmitterdorfer commented Nov 23, 2022 • edited Loading

javanna left a comment

Choose a reason for hiding this comment

danielmitterdorfer commented Nov 28, 2022 • edited Loading

javanna left a comment

Choose a reason for hiding this comment

javanna Nov 28, 2022

Choose a reason for hiding this comment

danielmitterdorfer Nov 29, 2022

Choose a reason for hiding this comment

danielmitterdorfer Nov 30, 2022

Choose a reason for hiding this comment

javanna Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

javanna Nov 30, 2022

Choose a reason for hiding this comment

danielmitterdorfer Nov 30, 2022

Choose a reason for hiding this comment

javanna Nov 30, 2022

Choose a reason for hiding this comment

danielmitterdorfer Nov 30, 2022

Choose a reason for hiding this comment

danielmitterdorfer commented Nov 17, 2022 •

edited

Loading

danielmitterdorfer commented Nov 23, 2022 •

edited

Loading

danielmitterdorfer commented Nov 28, 2022 •

edited

Loading

javanna Nov 30, 2022 •

edited

Loading