Add stats api. #308

adnapibar · 2020-05-29T08:44:52Z

Issue #302

The /_ltr/stats api provides basic information on the plugin usage and performance, including,

overall status of the plugin health.
statistics on the feature stores.
statistics on the internal caches used by the plugin.

nathancday · 2020-05-29T14:02:26Z

Thanks for building this out and contributing it! I'm checking it out locally now and I'm sure this will be useful to a lot of plugin users.

@nomoa would you review this PR when you have a chance?

nathancday · 2020-05-29T14:05:01Z

Also @ebernhardson if you want to take a look, I'm interested if this would work to resolve the issue you raised about cache churn in #140

nathancday · 2020-06-03T14:02:54Z

Hey @adnapibar could you do a quick brain dump to about how you designed the stats api to work and add a new section for its usage to docs/advanced-functionality.rst?

nomoa

Looks great, thanks!

I haven't fully reviewed the patch yet but I wanted to send a first round of comments.

The main concern I have is the blocking call in StoreUtils on the Client to get the counts of the various elements stored. I think this is problematic to have a blocking call there and we should flip the logic to use the async APIs like everything else. But overall what StoreUtils is extracting should probably be clarified a bit more.

One thing that would be very nice is implementing some tests in src/test/resources/rest-api-spec/.

Thanks!

src/main/java/com/o19s/es/ltr/LtrQueryParserPlugin.java

src/main/java/com/o19s/es/ltr/action/LTRStatsAction.java

src/main/java/com/o19s/es/ltr/utils/StoreUtils.java

src/main/java/com/o19s/es/ltr/stats/LTRStats.java

src/main/java/com/o19s/es/ltr/rest/RestLTRStats.java

src/main/java/com/o19s/es/ltr/utils/StoreUtils.java

adnapibar · 2020-07-13T02:53:05Z

Looks great, thanks!

I haven't fully reviewed the patch yet but I wanted to send a first round of comments.

The main concern I have is the blocking call in StoreUtils on the Client to get the counts of the various elements stored. I think this is problematic to have a blocking call there and we should flip the logic to use the async APIs like everything else. But overall what StoreUtils is extracting should probably be clarified a bit more.

One thing that would be very nice is implementing some tests in src/test/resources/rest-api-spec/.

Thanks!

Thanks @nomoa for your review. I couldn't get to resolve the comments earlier, sorry about that. I have resolved most of your earlier comments. I agree the StoreUtils was not abstracting much, so I have removed that and moved the code to the suppliers. The code to get the counts is now replaced with an aggregate query (not sure if that resolves the blocking concern that you have, any pointers on that would be great!). Also, added some tests in the rest-api-specs.

Thanks!

nomoa

@adnapibar thanks and sorry for the late response!

I left some minor comments (mostly cleanups) but overall this looks great.

This is a big PR so another pair of eyes might be welcome to make sure that I did not miss anything important.

docs/advanced-functionality.rst

src/main/java/com/o19s/es/ltr/action/TransportLTRStatsAction.java

src/main/java/com/o19s/es/ltr/stats/LTRStat.java

src/main/java/com/o19s/es/ltr/stats/suppliers/StoreStatsSupplier.java

nomoa · 2020-08-06T14:36:44Z

Forgot to mention the blocking calls:
I think it's OK to leave them, it is generally not great to call get() on client requests, reason is that you hold 2 threads from the threadpool for the operation: one waiting and another one doing the job.

Just for reference the way to fix this is to switch to async calls using the execute method with an ActionListener but this seems hard in the current state of the PR since it relies on Supplier::get() to gather its metrics.

nathancday · 2020-08-06T15:33:44Z

Thanks again @adnapibar for all of the new functionality and @nomoa for all of the review. I just requested a second set of @worleydl

softwaredoug

Thanks @adnapibar! Aside from various stylistic questions, I think the two most important fixes we would want to see

The bug @nomoa pointed out on the feature store aggregation (and an accompanying test ideally)
We should rename stats => _stats because I just know someone will want to create a feature store named stats and we use _ for reserved names

See other various stylistic changes of various degrees of importance...

softwaredoug · 2020-08-06T17:21:50Z

src/main/java/com/o19s/es/ltr/action/LTRStatsAction.java

+        }
+    }
+
+    public static class LTRStatsNodesRequest extends BaseNodesRequest<LTRStatsNodesRequest> {


Nit - Not sure there's any way we could better differentiate this is a Nodes and the other request/response are singular Node

I kept it as per existing conventions in the repo. One alternative convention might be to use LTRStatsNodeRequest for single node and LTRStatsRequest for nodes. (example)

src/main/java/com/o19s/es/ltr/stats/LTRStat.java

src/main/java/com/o19s/es/ltr/stats/StatName.java

src/main/java/com/o19s/es/ltr/stats/suppliers/StoreStatsSupplier.java

src/main/java/com/o19s/es/ltr/stats/suppliers/PluginHealthStatusSupplier.java

src/main/java/com/o19s/es/ltr/stats/suppliers/StoreStatsSupplier.java

src/test/resources/rest-api-spec/test/fstore/90_get_stats.yml

adnapibar · 2020-08-10T19:57:52Z

Thanks @nomoa and @softwaredoug for taking time to review my PR. I have addressed your comments and made the requested changes. Please let me know if there is anything else that I have missed or should add.

src/main/java/com/o19s/es/ltr/stats/suppliers/StoreStatsSupplier.java

nathancday · 2020-08-17T14:44:25Z

@softwaredoug and @nomoa green-light for merge?

nomoa · 2020-08-24T12:15:53Z

@nathancday LGTM

softwaredoug

+1 from me, thanks for all the hard work

The api provides basic information on the plugin usage and performance, including, - overall status of the plugin health. - statistics on the feature stores. - statistics on the internal caches used by the plugin.

- Refactor to remove the LTRStats dependency from RestLTRStats - Remove client and clusterService instance variables from LtrQueryParserPlugin. - Minor Refactoring

- move the operations inside the stat suppliers.

adnapibar · 2020-08-27T23:46:12Z

@nathancday Can we merge it now?

worleydl · 2020-08-28T03:52:21Z

Merging now. Should see a release next week.

nathancday requested a review from nomoa May 29, 2020 14:02

nomoa reviewed Jun 4, 2020

View reviewed changes

adnapibar force-pushed the stats branch from 6e22177 to 1759fd4 Compare July 11, 2020 16:42

adnapibar requested a review from nomoa August 4, 2020 18:08

nomoa reviewed Aug 6, 2020

View reviewed changes

nathancday requested review from softwaredoug and worleydl August 6, 2020 14:56

softwaredoug suggested changes Aug 7, 2020

View reviewed changes

adnapibar force-pushed the stats branch from 0f23d9e to d0a3ac2 Compare August 10, 2020 19:45

nomoa reviewed Aug 11, 2020

View reviewed changes

src/main/java/com/o19s/es/ltr/stats/suppliers/StoreStatsSupplier.java Show resolved Hide resolved

softwaredoug approved these changes Aug 24, 2020

View reviewed changes

Rabi Panda and others added 8 commits August 27, 2020 16:01

Add stats api.

3f0249c

The api provides basic information on the plugin usage and performance, including, - overall status of the plugin health. - statistics on the feature stores. - statistics on the internal caches used by the plugin.

Refactor the getRequest method.

946d621

Fix case-change in IndexMetadata as part of 7.8 upgrade.

cf1591a

Resolve Review comments

fe316cd

- Refactor to remove the LTRStats dependency from RestLTRStats - Remove client and clusterService instance variables from LtrQueryParserPlugin. - Minor Refactoring

Remove StoreUtils

fcf3c48

- move the operations inside the stat suppliers.

Use aggregates with multi search requests.

f3fb7db

add rest-api spec and test.

6d0b1d3

add stats section into advanced functionality doc.

c3520bb

adnapibar force-pushed the stats branch from d0a3ac2 to 5477d14 Compare August 27, 2020 23:40

Addressed review comments.

00a4b96

adnapibar force-pushed the stats branch from 5477d14 to 00a4b96 Compare August 27, 2020 23:44

worleydl merged commit 0455051 into o19s:master Aug 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stats api. #308

Add stats api. #308

adnapibar commented May 29, 2020 •

edited

Loading

nathancday commented May 29, 2020

nathancday commented May 29, 2020

nathancday commented Jun 3, 2020

nomoa left a comment

adnapibar commented Jul 13, 2020

nomoa left a comment

nomoa commented Aug 6, 2020

nathancday commented Aug 6, 2020

softwaredoug left a comment

softwaredoug Aug 6, 2020

adnapibar Aug 11, 2020 •

edited

Loading

adnapibar commented Aug 10, 2020

nathancday commented Aug 17, 2020

nomoa commented Aug 24, 2020

softwaredoug left a comment

adnapibar commented Aug 27, 2020

worleydl commented Aug 28, 2020

Add stats api. #308

Add stats api. #308

Conversation

adnapibar commented May 29, 2020 • edited Loading

nathancday commented May 29, 2020

nathancday commented May 29, 2020

nathancday commented Jun 3, 2020

nomoa left a comment

Choose a reason for hiding this comment

adnapibar commented Jul 13, 2020

nomoa left a comment

Choose a reason for hiding this comment

nomoa commented Aug 6, 2020

nathancday commented Aug 6, 2020

softwaredoug left a comment

Choose a reason for hiding this comment

softwaredoug Aug 6, 2020

Choose a reason for hiding this comment

adnapibar Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

adnapibar commented Aug 10, 2020

nathancday commented Aug 17, 2020

nomoa commented Aug 24, 2020

softwaredoug left a comment

Choose a reason for hiding this comment

adnapibar commented Aug 27, 2020

worleydl commented Aug 28, 2020

adnapibar commented May 29, 2020 •

edited

Loading

adnapibar Aug 11, 2020 •

edited

Loading