Profile API should show node details as well as shard details #96396

quux00 · 2023-05-27T15:24:27Z

SearchProfileResults has additional fields for XContent output: node_id, cluster, index, shard_id. It parses the existing composite ID using the new parseProfileShardId method, which reverses the SeachShardTarget.toString method.

No new information is added here, merely the splitting out of the four pieces of information in the profile shards "composite" id that is created by the SeachShardTarget.toString method.

Profile/shards output now has the form:

  "profile": {
    "shards": [
      {
        "id": "[2m7SW9oIRrirdrwirM1mwQ][blogs][0]",
        "node_id": "2m7SW9oIRrirdrwirM1mwQ",
        "shard_id": "0",
        "index": "blogs",
        "cluster": "(local)",
        "searches": [ ... ]
        ...
      },
      {
        "id": "[UngEVXTBQL-7w5j_tftGAQ][remote1:blogs][2]",
        "node_id": "UngEVXTBQL-7w5j_tftGAQ",
        "shard_id": "2",
        "index": "blogs",
        "cluster": "remote1",
        "searches": [ ... ]
        ...

where the latter is on a remote cluster and you can see that as the prefix on the index name.

Partially addresses #25896

elasticsearchmachine · 2023-06-09T21:33:24Z

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

elasticsearchmachine · 2023-06-09T21:33:24Z

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

benwtrent · 2023-06-12T13:57:16Z

server/src/main/java/org/elasticsearch/search/profile/SearchProfileResults.java

@@ -79,9 +90,24 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
        for (String key : sortedKeys) {
            builder.startObject();
            builder.field(ID_FIELD, key);
-            shardResults.get(key).toXContent(builder, params);
+
+            ShardProfileId shardProfileId = parseProfileShardId(key);


I think this is good for backwards compatibility, but what do you think of actually serializing those objects back instead of encoding them in the id and parsing them out again?

It seems brittle to me to store important info in a plain text string that is assumed to be formatted in a specific way.

@quux00 the brittleness could be acceptable if test coverage was adequate.

I know there can be thousands SearchProfileResults and maybe we don't want to add more fields to it for the sake of heap usage.

What do you think @javanna ?

Hi Ben,

I think this is good for backwards compatibility, but what do you think of actually serializing those objects back instead of encoding them in the id and parsing them out again? It seems brittle to me to store important info in a plain text string that is assumed to be formatted in a specific way.

In terms of brittleness, I marked this PR as partially addressing the original issue: #25896, where the request was basically to split the composite ID and maybe add some additional information. They recommended not changing the ID itself because that would be a backwards incompatible change.

So the brittleness is already baked in when that composite ID was created and now we need to support that composite ID in order to not make breaking changes.

In my analysis comment on that issue, I analyzed all the formats of that ID and how it's generated, indicating that it would be fairly straightforward to parse that out. Adding new info is problematic though because that composite ID is used as a Map key (https://github.com/elastic/elasticsearch/pull/96396/files#diff-582e9063bae3a8d070700f30a160c2647fd69bff489591ce5a16821bac63af6aR52) and that Map is passed around quite a bit through multiple layers before it gets to the place where it is rendered as XContent. IIRC (it's been two weeks since I looked at this), the likely place to serialize the data is in SearchShardTarget, but that doesn't make out to the place the XContent is rendered (only the composite ID map key).

I think we'd need to refactor a fair bit of code to avoid using that ID as the map key. So I decided to try to go the simple path first - just parsing out the existing info from that map key.

We could certainly try the bigger refactoring if we think it's worth it and want additional things not in the composite id, like cluster_uuid.

the brittleness could be acceptable if test coverage was adequate.

In terms of better test coverage, the approach I took was bottom up from the possible formats that are generated by SearchShardTarget.toString(), which is the composite ID map key.

I can enhance the test I added to use the SearchShardTarget.toString() method to generate the input.

I'm open to suggestions on how else to test it. Since the node_id and possibly other fields would be randomized in yamlRestTests. The test would need to parse out the composite on the fly and I'm not sure how I'd do that in a yamlRestTest. Do you have ideas on an additional good way to test this?

benwtrent

Thanks for the clarifications @quux00

Could you add some tests on the output and add an assert to the parser to catch during tests?

Here is where you should put the integration tests:
rest-api-spec/test/search/370_profile.yml

These yaml tests are ran against various Elasticsearch versions. So you will have to restrict the version via

  - skip:
      version: " - 8.7.99"
      reason: introduced in 8.8.0

Or whatever version is appropriate.

server/src/main/java/org/elasticsearch/search/profile/SearchProfileResults.java

elasticsearchmachine · 2023-06-22T09:43:42Z

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

benwtrent

two minor things and I think this is good to go.

benwtrent · 2023-06-22T11:48:01Z

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search/370_profile.yml

+---
+composite_id_parsed:
+  - skip:
+      version: " - 8.8.99"


With 8.9.0 being cut, you will need to update this as it will get released in 8.10 now.

Fixed. Thanks for the reminder.

I suppose you haven't pushed your local commit yet? Once you do, I will give it another review

Pushed it now. It must have failed when I tried it earlier and I missed it.

server/src/main/java/org/elasticsearch/search/profile/SearchProfileResults.java

benwtrent

SearchProfileResults has additional fields for XContent output: node_id, cluster, index, shard_id. It parses the existing composite ID using the new parseProfileShardId method, which reverses the SeachShardTarget.toString method. No new information is added here, merely the splitting out of the four pieces of information in the profile shards "composite" id that is created by the SeachShardTarget.toString method. Profile/shards output now has the form: "profile": { "shards": [ { "id": "[2m7SW9oIRrirdrwirM1mwQ][blogs][0]", "node_id": "2m7SW9oIRrirdrwirM1mwQ", "shard_id": "0", "index": "blogs", "cluster": "(local)", "searches": [ ... ] ... }, { "id": "[UngEVXTBQL-7w5j_tftGAQ][remote1:blogs][2]", "node_id": "UngEVXTBQL-7w5j_tftGAQ", "shard_id": "2", "index": "blogs", "cluster": "remote1", "searches": [ ... ] ... where the latter is on a remote cluster and you can see that as the prefix on the index name. Partially addresses elastic#25896

…sing

Added asserts to the SearchProfileResults.parseCompositeProfileShardId method to assert against invalid inputs Added yamlRestTest for the new fields in the profile response. Updated the existing unit test to use more randomization of inputs and to use the SearchShardTarget.toString method to generate the compositeId (which is what happens in production)

…dTarget.toString to set the key for the SearchProfileShardResult map

…hardId regex to private static final var

…atch on known value

quux00 added the WIP label May 27, 2023

elasticsearchmachine added the v8.9.0 label May 27, 2023

quux00 force-pushed the profile/show-node-details branch 2 times, most recently from 0e8036e to 1efbe87 Compare June 9, 2023 21:21

quux00 changed the title ~~Draft: Profile API should show node details as well as shard details~~ Profile API should show node details as well as shard details Jun 9, 2023

quux00 added :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team >non-issue >enhancement and removed WIP >non-issue labels Jun 9, 2023

quux00 marked this pull request as ready for review June 9, 2023 21:33

quux00 requested review from jdconrad and javanna June 9, 2023 21:33

quux00 force-pushed the profile/show-node-details branch from 5dc8388 to fcdd4be Compare June 12, 2023 13:39

benwtrent reviewed Jun 12, 2023

View reviewed changes

benwtrent reviewed Jun 15, 2023

View reviewed changes

server/src/main/java/org/elasticsearch/search/profile/SearchProfileResults.java Show resolved Hide resolved

quux00 force-pushed the profile/show-node-details branch from fcdd4be to 7440b1d Compare June 16, 2023 19:58

pugnascotia added v8.10.0 and removed v8.9.0 labels Jun 22, 2023

benwtrent reviewed Jun 22, 2023

View reviewed changes

quux00 force-pushed the profile/show-node-details branch from e52024f to 86f29d1 Compare June 23, 2023 15:25

benwtrent approved these changes Jun 23, 2023

View reviewed changes

quux00 added 4 commits June 23, 2023 19:55

Properly updated profile.asciidoc with all embedded test snippets pas…

9885849

…sing

code comment cleanup

fe9b1f7

Fixed javadoc for record ShardProfileId

ab51c50

quux00 added 4 commits June 23, 2023 19:55

SearchProfileResultsTests.createTestItem now properly uses SearchShar…

6548ac1

…dTarget.toString to set the key for the SearchProfileShardResult map

PR feedback - updated yaml test skip to 8.9.99 and changed compositeS…

0489321

…hardId regex to private static final var

when running when with remote cluster the cluster changes, so can't m…

eec0fc8

…atch on known value

quux00 force-pushed the profile/show-node-details branch from 86f29d1 to eec0fc8 Compare June 23, 2023 23:59

quux00 merged commit afbf1f5 into elastic:main Jun 24, 2023

quux00 mentioned this pull request Jul 19, 2023

Add specific cluster error info, shard info and additional metadata for CCS when minimizing roundtrips #97731

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Profile API should show node details as well as shard details #96396

Profile API should show node details as well as shard details #96396

quux00 commented May 27, 2023 •

edited

Loading

elasticsearchmachine commented Jun 9, 2023

elasticsearchmachine commented Jun 9, 2023

benwtrent Jun 12, 2023

benwtrent Jun 14, 2023

quux00 Jun 14, 2023

benwtrent left a comment

elasticsearchmachine commented Jun 22, 2023

benwtrent left a comment

benwtrent Jun 22, 2023

quux00 Jun 23, 2023

benwtrent Jun 23, 2023

quux00 Jun 23, 2023

benwtrent left a comment

Profile API should show node details as well as shard details #96396

Profile API should show node details as well as shard details #96396

Conversation

quux00 commented May 27, 2023 • edited Loading

elasticsearchmachine commented Jun 9, 2023

elasticsearchmachine commented Jun 9, 2023

benwtrent Jun 12, 2023

Choose a reason for hiding this comment

benwtrent Jun 14, 2023

Choose a reason for hiding this comment

quux00 Jun 14, 2023

Choose a reason for hiding this comment

benwtrent left a comment

Choose a reason for hiding this comment

elasticsearchmachine commented Jun 22, 2023

benwtrent left a comment

Choose a reason for hiding this comment

benwtrent Jun 22, 2023

Choose a reason for hiding this comment

quux00 Jun 23, 2023

Choose a reason for hiding this comment

benwtrent Jun 23, 2023

Choose a reason for hiding this comment

quux00 Jun 23, 2023

Choose a reason for hiding this comment

benwtrent left a comment

Choose a reason for hiding this comment

quux00 commented May 27, 2023 •

edited

Loading