top_metrics needs a test for flattened fields

[nik9000]

I believe top_metrics can fetch flattened fields but I never wrote a test for it because I didn't realize anyone would be interested in that. Folks seem to be. We should probably have a test to make sure it continues to work.

[elasticmachine]

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

[fighting-dreamer]

Newbie to open-source coding. Like to pick this Issue.

[nik9000]

Go ahead! I imagine we'd add the test to x-pack/plugin/src/test/resources/rest-api-spec/test/analytics/top_metrics.yml and I'd run the test with something like ./gradlew :x-pack:plugin:check -Dtests.method='*top_metrics*.

[fighting-dreamer]

trying to understand how test work.
I understand that the test/queries are to be written in the above file(x-pack/plugin/src/test/resources/rest-api-spec/test/analytics/top_metrics.yml).

[nik9000]

Sorry, yeah. Those yml files are the integration tests we share with our client libraries. In this case I'm looking for a test that creates a flattened field, indexes some documents, and lists the flattened field as a metric in a top_metrics agg. Then asserts that it got the values it expects back.

[fighting-dreamer]

Have figured out the test case, was quite busy at work this last month. hoping to raise the PR soon.

[GrayFox1]

I am interested to pick this Issue, are you still working on it? @fighting-dreamer

[nik9000]

@GrayFox1 I imagine @fighting-dreamer is not going to finish this one so if you'd like to have a look go for it.

Since we've had a whole bunch of similar comments to yours in the past few days I'll mention that we don't want contributions made as part of a class. I'm not sure if that applies to you. Folks contributing as part of a class tend to open PRs but not take the time to go through the actual code review portion of the process. And when a whole class piles on trying to contribute to the project it puts a bunch of load on the maintainers. Sorry for the big paragraph. Its become a thing.... Anyway, if you aren't from a class then welcome!

Honestly the rule isn't hard and fast - if you are from a class but swear to go through with the code review, even if we finish it after your class then I'm willing to work with you. Sorry for all this.

Go ahead! I imagine we'd add the test to x-pack/plugin/src/test/resources/rest-api-spec/test/analytics/top_metrics.yml and I'd run the test with something like ./gradlew :x-pack:plugin:check -Dtests.method='*top_metrics*.

I think these are still the correct instructions! As of today you'll need JAVA_HOME to be set to a Java 16 installation to build Elasticsearch.

[nilanshu96]

Hi @nik9000 ,
I have created a very simple test case for a flattened type field called "with flattened". You can check it here
top_metrics.yml

The test can be run using the following command:

./gradlew ':x-pack:plugin:yamlRestTest' --tests "org.elasticsearch.xpack.test.rest.XPackRestIT" -Dtests.method="test {p0=analytics/top_metrics/with flattened}"

I've confirmed that the test is passing. Currently the test is very simple. Please do tell me if there are any improvements that can be made else I'll raise a PR if this is sufficient.

[nilanshu96]

@nik9000 Can you please provide an update on this issue or is it already resolved? If not can you check my solution in previous comment?

[imotov]

Could you open a PR for it?

[nilanshu96]

Hi @imotov I've added the PR, The test in question can be found here. Currently I've created a very simple test to see if flattened fields can be tested. Can you please check it and suggest any improvements?

[nilanshu96]

Hi @imotov @nik9000 , can you please provide an update regarding this issue?

[nik9000]

Sorry about leaving you hanging again! I just had to wait for the tests to pass and then merge it. Thanks for your contribution! I promise to be better about reviewing in the future.

…

On Mon, Oct 18, 2021, 03:06 Nilanshu V Rajmane ***@***.***> wrote: Hi @imotov <https://github.com/imotov> @nik9000 <https://github.com/nik9000> , can you please provide an update regarding this issue? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#70108 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABUXIWD2LDCIMP6UHFHTJTUHPBOHANCNFSM4YZ6BQ4Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[nilanshu96]

@nik9000 don't worry, it wasn't a big deal. Also I'm happy to contribute. I got to learn a lot from this task.

[nik9000]

Closed by #78960.