[server/status] implement generic status tracking#7459
[server/status] implement generic status tracking#7459spalger merged 7 commits intoelastic:masterfrom
Conversation
In elastic#7333 we needed the ability to set the server status from outside of a plugin, but statuses were implemented in a way that coupled them to plugins. This let to reaching in and setting the status of a plugin from the server. Rather than extending the undesirable coupling of status & plugin I've instead made the server status service support creating more generic status tracker objects, and extended it's API to include plugin-specific methods like `createForPluginId(pluginId)` and `getStateForPluginId(pluginId)`. With the new API the settings service will be able to create it's own status object with `kbnServer.status.create('settings')` rather than reaching into the kibana plugin and setting its status.
94da008 to
cbd7654
Compare
c382f2d to
fe67508
Compare
|
|
||
| create(plugin) { | ||
| return (this._created[plugin.id] = new Status(plugin, this.server)); | ||
| create(id) { |
There was a problem hiding this comment.
This PR creates two types of statuses: plugin status and service status. Since we have a method called createForPlugin for attaching a new Plugin status, should we rename the create method to createForService for consistency?
There was a problem hiding this comment.
I don't really care for the "service" distinction personally, perhaps I should remove it from the status page logic. Primary reason being that services are only one thing that could use this new generic status, and since there aren't any "service" specific properties to this I don't think it makes sense to be createForService()
There was a problem hiding this comment.
Understood: there's no need for a separate createForService() method — because there's no need for a separate ServiceStatus class that's not going to have any specific properties (unlike for plugins). Agree with this bit.
I guess what's bugging me is that the class hierarchy doesn't match the distinction on the status page. There it looks like there are two distinct kinds of statuses, one for services and one for plugins. Maybe we just need to name the first block of generic statuses on the status page something other than service status, but I'm drawing a blank at the moment. Also this is veering into bikeshedding territory so I'm happy to just go with "service status" if we can't come up with something else soon :)
There was a problem hiding this comment.
My original instinct was to just list the ids for each status and let those draw their own distinction, if that's enough for you then it's enough for me!
There was a problem hiding this comment.
I like this! It more consistently mirrors the class hierarchy where everything is a Status, just that if a status has more specific properties like in the case of plugins, there's a subclass for it.
ec4cd1d to
8cb1694
Compare
| ui.statuses = data.status.statuses; | ||
| ui.name = data.name; | ||
|
|
||
| ui.statuses = _.groupBy(data.status.statuses, s => s.plugin ? 'plugins' : 'generic'); |
There was a problem hiding this comment.
The check for s.plugin here seems to break abstraction. What do you think of the Status class having an isPluginStatus method that abstracts away this check, to with similar methods like createForPlugin and getForPluginId and then using that method here instead?
There was a problem hiding this comment.
I would be up for this, but this logic is all front-end, where it's just processing a list of JSON objects
There was a problem hiding this comment.
I believe this is moot now, based on the discussion in https://github.com/elastic/kibana/pull/7459/files#r67094458
| </h4> | ||
|
|
||
| <table class="plugin_status_breakdown" ng-if="ui.statuses"> | ||
| <table class="statuses" data-test-subj="statusBreakdown" ng-if="ui.statuses"> |
There was a problem hiding this comment.
Not a blocker for this PR, more for my own curiosity: why did you create the separate data-test-subj attribute? Is it something only our tests look for or does it have another purpose?
There was a problem hiding this comment.
Never mind, found the answer at https://github.com/elastic/kibana/pull/7459/files#diff-f8a58592f3622c5e7bf777330afcf177R17 :)
|
LGTM! |
…Status [server/status] implement generic status tracking Former-commit-id: 7af3e7e
In #7333 we needed the ability to set the server status from outside of a plugin, but statuses were implemented in a way that coupled them to plugins. This led to reaching in and setting the status of the Kibana plugin. Rather than extending the undesirable coupling of status & plugin I've instead made the server status service support creating more generic status tracker objects, and extended it's API to include plugin-specific methods like
createForPlugin(plugin)andgetStateForPluginId(pluginId).With the new API the settings service will be able to create it's own status object with
kbnServer.status.create('settings')rather than reaching into the kibana plugin and setting its status.See ycombinator#1 for an implementation of a more generic status, specifically ycombinator@1556b81