Add parsing from xContent to SearchProfileShardResults and nested classes #22649

cbuescher · 2017-01-17T09:12:26Z

In preparation for being able to parse SearchResponse from its rest representation
for the java rest client, this adds fromXContent to SearchProfileShardResults and its
nested classes.

…nd CollectorResult

javanna

left a few minors, thanks for doing this

javanna · 2017-01-17T10:37:26Z

core/src/main/java/org/elasticsearch/search/profile/ProfileResult.java

What causes this if? differences between the different content types that we support?

Maybe we should rather do this while parsing instead of using parser.map() here?

And use java.lang.Number?

javanna · 2017-01-17T10:43:29Z

core/src/main/java/org/elasticsearch/search/profile/query/CollectorResult.java

nit: can you replace this with the same comment that we had above? I find it more effective and clear

javanna · 2017-01-17T10:45:21Z

core/src/test/java/org/elasticsearch/search/profile/ProfileResultTests.java

javanna · 2017-01-17T10:46:28Z

core/src/test/java/org/elasticsearch/search/profile/ProfileResultTests.java

why not using XContentHelper#toXContent here too?

javanna · 2017-01-17T11:09:54Z

core/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java

nice, this method could be potentially called in all our tests so that we randomize the human readable flag, just to make sure we don't miss any of those additional fields. I think we haven't and this is the first time we encounter this but tests are the only way to actually verify. Maybe we can do this as a followup?

++ good point

can you look into this when you have time please?

tlrx

Thanks @cbuescher, I'm sure that it hasn't been fun to do. I left some comments here and there, nothing big.

tlrx · 2017-01-17T12:15:45Z

core/src/main/java/org/elasticsearch/search/profile/ProfileResult.java

And use java.lang.Number?

tlrx · 2017-01-17T12:17:01Z

core/src/main/java/org/elasticsearch/search/profile/ProfileResult.java

+                }
+            }
+        }
+        Map<String, Long> timings = new HashMap<>(parsedTimings.size());


Can we check that parsedTimings is not null before doing this?

This part has been removed while addressing another comment.

tlrx · 2017-01-17T12:18:18Z

core/src/main/java/org/elasticsearch/search/profile/ProfileResult.java

+     *            A map of breakdown timing for the node
+     * @return The total time at this node, inclusive of children
+     */
+    private static long getNodeTime(Map<String, Long> timings) {


What if timings is null? It doesn't seem to be required in the ctor

Good point, I moved this code from somewhere where it was clear that the map is always there. I will add a check to the ctor.

Thanks! Could we also rename this to getTotalTime or something?

The reason I left the name was to make it clearer where the code was before in this PR (its from AbstractInternalProfileTree). I will rename it to getTotalTime. Also I think the comment is not up to date anymore (it doesn't sum any children) so I will remove that part)

tlrx · 2017-01-17T12:20:33Z

core/src/main/java/org/elasticsearch/search/profile/SearchProfileShardResults.java

+        ensureExpectedToken(XContentParser.Token.START_OBJECT, token, parser::getTokenLocation);
+        List<QueryProfileShardResult> queryProfileResults = new ArrayList<>();
+        AggregationProfileShardResult aggProfileShardResult = null;
+        String name = null;


Could this be renamed to id to avoid confusion?

tlrx · 2017-01-17T12:22:11Z

core/src/main/java/org/elasticsearch/search/profile/SearchProfileShardResults.java

+                } else {
+                    throwUnknownField(currentFieldName, parser.getTokenLocation());
+                }
+            }


Maybe add another else block with unexpected token type?

tlrx · 2017-01-17T12:23:46Z

core/src/main/java/org/elasticsearch/search/profile/query/CollectorResult.java

+                } else {
+                    throwUnknownField(currentFieldName, parser.getTokenLocation());
+                }
+            }


Same here, shouldn't we throw an exception if the token type is not expected? Or maybe I'm missing something.

tlrx · 2017-01-17T12:24:20Z

core/src/main/java/org/elasticsearch/search/profile/query/QueryProfileShardResult.java

+                } else {
+                    throwUnknownField(currentFieldName, parser.getTokenLocation());
+                }
+            }


Maybe also here

tlrx · 2017-01-17T12:26:41Z

core/src/test/java/org/elasticsearch/search/profile/SearchProfileShardResultsTests.java

+        XContentType xcontentType = randomFrom(XContentType.values());
+        XContentBuilder builder = XContentFactory.contentBuilder(xcontentType);
+        builder.startObject();
+        builder = shardResult.toXContent(builder, ToXContent.EMPTY_PARAMS);


Can't we use XContentHelper.toXContent() here?

cbuescher · 2017-01-17T21:46:09Z

@tlrx @javanna thanks for the review, I pushed a commit that hopefully addresses all your comments.

javanna · 2017-01-18T12:47:39Z

core/src/main/java/org/elasticsearch/search/profile/SearchProfileShardResults.java

        XContentParser.Token token = parser.currentToken();
        ensureExpectedToken(XContentParser.Token.START_OBJECT, token, parser::getTokenLocation);
-        Map<String, ProfileShardResult> searchProfileResults = new HashMap<>();
+        Map<String, ProfileShardResult> searchProfileResults = new LinkedHashMap<>();


why do we need ordering here?

This way we preseve the order of the shard keys we encountered during xContent parsing. Without preserving this order, I encountered different xContent rendering outputs for the "shards" section (which is an array) in the roundtrip tests, so the order matters (at least the way assertToXContentEquivalent works so far).

I pushed another commit that fixes the order in which the elements in the "shards" array are written out.

tlrx

Thanks for the hard work! I made many comments but they are all cosmetics or minor. This is getting very close

tlrx · 2017-01-18T15:53:35Z

core/src/main/java/org/elasticsearch/search/profile/ProfileResult.java

+     *            A map of breakdown timing for the node
+     * @return The total time at this node, inclusive of children
+     */
+    private static long getNodeTime(Map<String, Long> timings) {


Thanks! Could we also rename this to getTotalTime or something?

tlrx · 2017-01-18T15:58:57Z

core/src/main/java/org/elasticsearch/search/profile/ProfileResult.java

+                } else if (DESCRIPTION.match(currentFieldName)) {
+                    description = parser.text();
+                } else if (NODE_TIME.match(currentFieldName)) {
+                    // we need to consume this value, but we use the raw nanosecond value


I think the "but we use the raw nanosecond value" part of this comment is misleading. When I read it I thought we used the NODE_TIME_RAW but it doesn't seems to be the case

Yes, we sum the timings values in ProfileResult ctor now, no need for both of these values, I'll update the comments

tlrx · 2017-01-18T16:01:25Z

core/src/main/java/org/elasticsearch/search/profile/query/QueryProfileShardResult.java

+        String currentFieldName = null;
+        List<ProfileResult> queryProfileResults = new ArrayList<>();
+        long rewriteTime = 0;
+        CollectorResult profileCollector = null;


Maybe profileCollector -> collector to reflect the field's name?

tlrx · 2017-01-18T16:07:08Z

core/src/test/java/org/elasticsearch/search/profile/ProfileResultTests.java

+        try (XContentParser parser = createParser(xContentType.xContent(), originalBytes)) {
+            ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation);
+            ProfileResult parsed = ProfileResult.fromXContent(parser);
+            assertEquals(profileResult.getTime(), parsed.getTime());


Can we check the parser in the try/catch block, and the parsed ProfileResult after that?

It's basically moving those two lines:
ProfileResult parsed = ProfileResult.fromXContent(parser);
assertEquals(profileResult.getTime(), parsed.getTime());

after the try block.

tlrx · 2017-01-18T16:08:34Z

core/src/test/java/org/elasticsearch/search/profile/ProfileResultTests.java

-        timings.put("key1", 12345L);
-        timings.put("key2", 6789L);
-        ProfileResult result = new ProfileResult("someType", "some description", timings, children, 123456L);
+        children.add(new ProfileResult("child1", "desc1", timings1, Collections.emptyList()));


I suppose that you can use singletonMap("key1", 100L) directly here

tlrx · 2017-01-18T16:16:56Z

...st/java/org/elasticsearch/search/profile/aggregation/AggregationProfileShardResultTests.java

+        XContentParserUtils.ensureFieldName(parser, parser.nextToken(), AggregationProfileShardResult.AGGREGATIONS);
+        XContentParserUtils.ensureExpectedToken(parser.nextToken(), XContentParser.Token.START_ARRAY, parser::getTokenLocation);
+        AggregationProfileShardResult parsed = AggregationProfileShardResult.fromXContent(parser);
+        assertToXContentEquivalent(builder.bytes(), toXContent(parsed, xcontentType), xcontentType);


Can we move this down, after the Token checks?

tlrx · 2017-01-18T16:17:23Z

...st/java/org/elasticsearch/search/profile/aggregation/AggregationProfileShardResultTests.java

+        ProfileResult profileResult = new ProfileResult("someType", "someDescription", timings, Collections.emptyList());
+        profileResults.add(profileResult);
+        AggregationProfileShardResult aggProfileResults = new AggregationProfileShardResult(profileResults);
+        XContentBuilder builder = JsonXContent.contentBuilder();


Same here, try with resource block

tlrx · 2017-01-18T16:19:09Z

core/src/test/java/org/elasticsearch/search/profile/query/CollectorResultTests.java

+        CollectorResult collectorResult = createTestItem(1);
+        XContentType xcontentType = randomFrom(XContentType.values());
+        XContentBuilder builder = XContentFactory.contentBuilder(xcontentType);
+        boolean humanReadable = randomBoolean();


No need to use an intermediate variable for this

I need it later when comparing to the xContent rendering of the parsed object.

I meant builder.humanReadable(randomBoolean()); here and use builder.humanReadable() later

Ah, that doesn't work after my last changes because I got rid of the builder.

tlrx · 2017-01-18T16:19:44Z

core/src/test/java/org/elasticsearch/search/profile/query/QueryProfileShardResultTests.java

+            queryProfileResults.add(ProfileResultTests.createTestItem(1));
+        }
+        CollectorResult profileCollector = CollectorResultTests.createTestItem(2);
+        long rewriteTime = randomNonNegativeLong();


Should we use the %100_000 strategy here too?

Yes, good point.

tlrx · 2017-01-18T16:19:57Z

core/src/test/java/org/elasticsearch/search/profile/query/QueryProfileShardResultTests.java

+    public void testFromXContent() throws IOException {
+        QueryProfileShardResult profileResult = createTestItem();
+        XContentType xcontentType = randomFrom(XContentType.values());
+        XContentBuilder builder = XContentFactory.contentBuilder(xcontentType);


tlrx · 2017-01-18T16:28:05Z

core/src/main/java/org/elasticsearch/search/profile/SearchProfileShardResults.java

-
-        for (Map.Entry<String, ProfileShardResult> entry : shardResults.entrySet()) {
+        builder.startObject(PROFILE_FIELD).startArray(SHARDS_FIELD);
+        TreeSet<String> sortedKeys = new TreeSet<>(shardResults.keySet());


Are we forced to sort all shard results every time just to be able to check them in tests? Can't we check / sort again in the test instead?

I talked to @javanna about this, the idea was to also make the rendered response reproducibly the same for the same source object. Happy to change this back any time, but the overhead this poses is negligible I think, also its just performed once per response.

it is not just for tests. Unfortunately this object uses a map to store information that goes into a json array, which is by definition ordered. Either we change the format of the response or we guarantee ordering then. I think this is a good way to guarantee ordering and make the response more human readable too.

maybe add a comment to explain this @cbuescher ?

cbuescher · 2017-01-18T17:43:55Z

@tlrx thanks for reviewing, I added a commit that addresses most of your comments and left some answer where I'd like to keep things (or discuss further)

javanna

LGTM

tlrx

LGTM

…sses (#22649) In preparation for being able to parse SearchResponse from its rest representation for the java rest client, this adds fromXContent to SearchProfileShardResults and its nested classes.

cbuescher · 2017-01-19T17:18:33Z

Ported to 5.x with c8e027f

@Header

* master: (117 commits) Add missing serialization BWC for disk usage estimates Expose disk usage estimates in nodes stats S3 repository: Deprecate specifying credentials through env vars, sys props, and remove profile files (elastic#22567) Fix Eclipse project generation Fix deprecation logging for lenient booleans Remove @Header we no longer need Make lexer abstract [Docs] Remove outdated info about enabling/disabling doc_values (elastic#22694) Move lexer hacks to EnhancedPainlessLexer Fix incorrect args order passed to createAggregator Improve painless's javadocs Add TestWithDependenciesPlugin to build (elastic#22646) Add parsing from xContent to SearchProfileShardResults and nested classes (elastic#22649) Add unit tests for FiltersAggregator (elastic#22678) Don't register search response listener in transport clients unmute FieldStatsIntegrationIT.testGeoPointNotIndexed, fix already pushed Mute FieldStatsIntegrationIT.testGeoPointNotIndexed, for now Painless: Add augmentation to string for base 64 (elastic#22665) Fix NPE on FieldStats with mixed cluster on version pre/post 5.2 (elastic#22688) Add parsing methods for UpdateResponse (elastic#22586) ...

As a follow up to elastic#22649, this changes the resent tests for parsing parts of search responses to randomly set the humanReadable() flag of the XContentBuilder that is used to render the responses. This should help to test that we can parse back thoses classes if the user specifies `?human=true` in the request url.

As a follow up to #22649, this changes the resent tests for parsing parts of search responses to randomly set the humanReadable() flag of the XContentBuilder that is used to render the responses. This should help to test that we can parse back thoses classes if the user specifies `?human=true` in the request url.

cbuescher added 5 commits January 16, 2017 17:18

Adding parsing to ProfileResult

be41a0e

Adding parsing to AggregationProfileShardResult

7463175

Add parsing to QueryProfileShardResult

f0e1590

Add parsing SearchProfileShardResults

90d9fea

Small test changes concerning parsing time strings in ProfileResult a…

413d2d0

…nd CollectorResult

cbuescher added :Search/Search Search-related issues that do not fall into other categories >enhancement review v6.0.0-alpha1 labels Jan 17, 2017

cbuescher requested review from javanna and tlrx January 17, 2017 09:12

Adapt to introduction of the time_in_nanos field

7cf041a

cbuescher force-pushed the addParsing-ProfileResult branch from 663aa6f to 7cf041a Compare January 17, 2017 11:20

javanna requested changes Jan 17, 2017

View reviewed changes

tlrx requested changes Jan 17, 2017

View reviewed changes

cbuescher force-pushed the addParsing-ProfileResult branch from 92d0e1e to fabf41b Compare January 17, 2017 21:09

Addressing review comments

4029cda

cbuescher force-pushed the addParsing-ProfileResult branch from fabf41b to 4029cda Compare January 17, 2017 21:16

javanna reviewed Jan 18, 2017

View reviewed changes

Fixed order of ids in response shard section

11cd520

tlrx requested changes Jan 18, 2017

View reviewed changes

tlrx reviewed Jan 18, 2017

View reviewed changes

Addressing more review comments

e81c546

javanna approved these changes Jan 19, 2017

View reviewed changes

adding comments

746dee3

tlrx approved these changes Jan 19, 2017

View reviewed changes

minor addition

1be3216

cbuescher force-pushed the addParsing-ProfileResult branch from 0ef1ad1 to 1be3216 Compare January 19, 2017 14:01

cbuescher merged commit e035540 into elastic:master Jan 19, 2017

cbuescher added the v5.3.0 label Jan 19, 2017

cbuescher mentioned this pull request Jan 20, 2017

Include human readable responses in response parsing tests #22717

Merged

cbuescher deleted the addParsing-ProfileResult branch November 27, 2025 10:28

Add parsing from xContent to SearchProfileShardResults and nested classes #22649

Add parsing from xContent to SearchProfileShardResults and nested classes #22649

Uh oh!

Conversation

cbuescher commented Jan 17, 2017

Uh oh!

javanna left a comment

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!

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!

tlrx left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

cbuescher commented Jan 17, 2017

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!

tlrx left a comment

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!

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

cbuescher Jan 18, 2017 •

edited

Loading