Stats api POC by stacey-gammon · Pull Request #11828 · elastic/kibana

stacey-gammon · 2017-05-16T20:10:18Z

POC

Relies on the saved object api: #11632

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

…ibana into tylersmalley-saved-objects-api

pickypg · 2017-05-16T21:27:23Z

        status: kbnServer.status.toJSON(),
-        metrics: kbnServer.metrics
+        metrics: kbnServer.metrics,
+        stats,


This is exactly what I had in mind. Part of me wonders if we should create a new _kibana/stats endpoint in Kibana, analogous to _nodes/stats for a specific Elasticsearch node, and start introducing this type of functionality.

At that point stats should could be either renamed or merged entirely with the top-level object. Then we could also cleanup the current API in that new API in terms of what it returns as it's a mix of "info" and "stats" even though it purports to be "status" only.

So what do you think is the best way forward? Continuing to put it on the status api and migrate as a second step, or start with a new api for these stats?

If the latter, I don't know if I like the path _kibana/stats. Don't we have everything at api/endpoint ? So i'd think it would be api/stats. Ultimately I defer to the monitoring team, I'm not familiar enough with our api to have a firm opinion.

Continuing to put it on the status api and migrate as a second step, or start with a new api for these stats?

Since we have identified ways that the existing status API should be split up and renamed to match the conventions of Elasticsearch (although having the /api prefix makes sense for Kibana), I would suggest continue to put these stats in the current status API

we could also cleanup the current API in that new API in terms of what it returns as it's a mix of "info" and "stats" even though it purports to be "status" only.

Cleaning up the existing API is important. Let's open another issue for that, and figure out what needs to be split, and what the different endpoints should be named.

Whether we use /_kibana/* or /api/* does not make a big difference to me other than to note that the rest of the stack has gone this direction. I do like our /api prefix particularly because it means we can setup request filters and such based on the flow of the URL itself. So perhaps /api/_kibana/stats might even be a better combo since the non-/api URLs have a very different meaning.

@jbudz and I discussed this a while back when working on #10180

Regarding prefixing the endpoints with underscores. While I believe we should align as much as possible with ES, this was done there to differentiate between types/documents as they are also on the root. We don't need to, and shouldn't, prefix our API endpoints.

At the time we decided to keep endpoint at api/status in lieu of renaming to stats and we can revisit that if we believe it's beneficial.

+1 to ^ and +1 on a new API, at this point they're hitting different services.

We have the ops interval for fetching server and os information. These are (relatively) quick reads and all have to do with how the kibana server is running. We need this information for the status page to say how kibana's running.

Adding saved object stats will make this too chatty and less focused. I don't think we should put the caching at this level, but at the consumer's level. Any time someone requests stats, we can hit es and return the most recent data.

I think @jbudz makes a good point. Nothing in the status page requires those saved object stats, so we shouldn't be making those extra queries if they aren't needed. This makes me lean towards a separate api/stats. Thoughts from the monitoring team?

I definitely would prefer a secondary API. I think it was @tsullivan that was the holdout there. :)

At the time, it seemed like making an additional API endpoint for these types of stats would cause some delay, but I'm all for doing it right the first time.

Yea, that was my bad @tsullivan, a closer look and it seems easy enough to do in a first pass, despite my initial hesitations. :) I will give it a go!

tsullivan

left a comment about using the authenticated client

tsullivan · 2017-05-17T18:32:18Z

+      const stats = getStats(
+        server.config().get('kibana.index'),
+        request,
+        server.plugins.elasticsearch.getCluster('admin').callWithRequest);


I think we want to use callWithInternalUser instead of callWithRequest. This might prove to be a bit of a headache though, since getStats relies on savedObjectsClient which expects callWithRequest

Because this uses callWithRequest, it takes the credentials of the user that calls the API, and requires that they have read access to the .kibana index (if X-Pack Security is set up).

When I call the API with a user that has no roles, I get this in the logs:

server log [18:26:09.028] [warning] Detected an unhandled Promise rejection. Error: action [indices:data/read/search] is unauthorized for user [no_role]

The branch has since been updated to accept a callAdminCluster function: https://github.com/elastic/kibana/pull/11632/files#diff-cc9fbe8850c8f590da1323a91d29717dR10

You could partially apply callWithRequest or pass callWithInternalUser directly https://github.com/elastic/kibana/pull/11632/files#diff-1ebbe7846164a2558bba9ae8d00e88f7R17

You could partially apply callWithRequest or pass callWithInternalUser directly

Cool, this is exactly what I would thinking would be a good solution here

I think we'll want the API endpoint to still use callWithRequest, and we could make the class interface for fetching optionally use an admin client. Would that work @tsullivan?

My concern with having callWithRequest used is that the calling user credentials might not have privilege to read the .kibana index. We may be dealing with a Monitoring-only user, for example. We talked about being safe in letting the API return the right data for any user, regardless of role, because there's no confidential information in this data - it's just counts of things.

I'm not sure I follow the point about "we could make the class interface for fetching optionally use an admin client" - does that go along with what Tyler suggests above? We just have to be sure that the API that Monitoring uses doesn't require any special roles for the calling user.

tsullivan · 2017-05-17T18:34:39Z

+export async function getStats(kibanaIndex, request, callWithRequest) {
+  const savedObjectsClient = new SavedObjectsClient(kibanaIndex, request, callWithRequest);
+  const dashResponse = await savedObjectsClient.find({ type: 'dashboard', perPage: 0 });
+  const visResponse = await savedObjectsClient.find({ type: 'visualization', perPage: 0 });


These stats might not need to be perfectly real-time. Feel free to do whatever you like with this comment, but one idea could be to create an internal poller that caches the stats every 10 minutes or so, and then when the API is called, it just returned the last stats from the cache.

On second thought, maybe this does need to be real-time 😁

tsullivan · 2017-05-17T18:51:07Z

Do the stats need to have count of Saved Searches and Index Patterns as well?

tsullivan

need an await along with call to getStats

tsullivan · 2017-05-17T19:06:55Z

    path: '/api/status',
-    handler: function (request, reply) {
+    handler: async function (request, reply) {
+      const stats = getStats(


Looks like this needs to be const stats = await getStats

alexfrancoeur · 2017-05-18T15:19:50Z

@tsullivan - ideally yes, we would have counts of saved searches and index patterns. @stacey-gammon is that readily available for this API to consume?

stacey-gammon · 2017-05-19T13:01:45Z

@tsullivan @alexfrancoeur -- yes I will add more stats. I only threw two in before sending it out, as I wanted to make sure the PR was on the right track before spending time adding more. I will add additional stats when I take into account the initial round of feedback and clean the PR up, to get it ready for review.

alexfrancoeur · 2017-05-19T15:22:19Z

I just had a quick chat with @stacey-gammon around how we're gathering this data. For overall dashboard, visualizations, saved searches and index pattern counts we are currently querying ES for each type with no hits. I don't believe this will work with visualization sub-types (which we'd also like to track). We'd likely need a more complex query.

My concern here is that there may be too many queries to ES if monitoring may be calling this new API every 10s or so. Given the amount of changes being made to how we index data with the removal of type in 6.0, it might be worth exploring the possibility of indexing the data differently to reduce the amount of ES queries. For example - we could make a single request that uses a filter aggregation on saved object type and a terms aggregation under visualization type.

I don't want to slow down the progress at all here, but thought it might be worth considering given the frequency we may be calling this API.

stacey-gammon · 2017-05-24T21:09:27Z

This PR has been superceded by this one: https://github.com/elastic/kibana/pull/11992/files now that saved object API is checked in. It uses the callWithInternalUser functionality and tracks more saved object type totals. I will need to sync with the operations team to figure out how to modify the index in order to track visualization sub types.

tylersmalley and others added 11 commits May 16, 2017 09:02

Adds saved object API

b250ad9

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

Fixes typo

621fd48

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

Remove kibana from saved object API path

197a5e4

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

Nests document in doc element for ES

005b029

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

Resolves tests for update API

8b0c32d

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

Prevent leaking of ES query to API

f6dd705

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

Adds version to saved objects API

af59a5c

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

Return version for searches

a8e3279

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

Removes ability to specify id on object creation

915c7df

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

Merge branch 'saved-objects-api' of https://github.com/tylersmalley/k…

73c7148

…ibana into tylersmalley-saved-objects-api

Simple stats api POC

89257bf

pickypg reviewed May 16, 2017

View reviewed changes

tsullivan reviewed May 17, 2017

View reviewed changes

tsullivan requested changes May 17, 2017

View reviewed changes

stacey-gammon added :Sharing v6.0.0 labels May 22, 2017

stacey-gammon closed this May 24, 2017

jimgoodwin added the v6.0.0-rc1 label Sep 28, 2017

stacey-gammon deleted the stats-api branch October 24, 2017 13:55

Conversation

stacey-gammon commented May 16, 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!

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!

tsullivan 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!

tsullivan commented May 17, 2017

Uh oh!

tsullivan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexfrancoeur commented May 18, 2017

Uh oh!

stacey-gammon commented May 19, 2017

Uh oh!

alexfrancoeur commented May 19, 2017

Uh oh!

stacey-gammon commented May 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants