From d49ee8e355e5d52a198cd3a2c9a4a6724bf4256d Mon Sep 17 00:00:00 2001 From: John Cowen Date: Thu, 10 Feb 2022 15:28:26 +0000 Subject: [PATCH] ui: Ensure proxy instance health is taken into account in Service Instance Listings (#12279) We noticed that the Service Instance listing on both Node and Service views where not taking into account proxy instance health. This fixes that up so that the small health check information in each Service Instance row includes the proxy instances health checks when displaying Service Instance health (afterall if the proxy instance is unhealthy then so is the service instance that it should be proxying) * Refactor Consul::InstanceChecks with docs * Add to-hash helper, which will return an object keyed by a prop * Stop using/relying on ember-data type things, just use a hash lookup * For the moment add an equivalent "just give me proxies" model prop * Start stitching things together, this one requires an extra HTTP request ..previously we weren't even requesting proxies instances here * Finish up the stitching * Document Consul::ServiceInstance::List while I'm here * Fix up navigation mocks Name > Service --- .changelog/12279.txt | 3 + .../consul/instance-checks/README.mdx | 96 +++++++++++++++++++ .../consul/instance-checks/index.hbs | 67 ++++++++----- .../consul/instance-checks/index.js | 50 ---------- .../consul/instance-checks/index.scss | 24 +++++ .../consul/service-instance/list/README.mdx | 83 ++++++++++++++++ .../consul/service-instance/list/index.hbs | 54 +++++++++-- ui/packages/consul-ui/app/helpers/to-hash.js | 12 +++ ui/packages/consul-ui/app/models/node.js | 2 + .../consul-ui/app/styles/components.scss | 1 + .../app/templates/dc/nodes/show/services.hbs | 8 +- .../app/templates/dc/services/show.hbs | 1 + .../templates/dc/services/show/instances.hbs | 17 +++- .../dc/services/instances/navigation.feature | 6 +- 14 files changed, 335 insertions(+), 89 deletions(-) create mode 100644 .changelog/12279.txt create mode 100644 ui/packages/consul-ui/app/components/consul/instance-checks/README.mdx delete mode 100644 ui/packages/consul-ui/app/components/consul/instance-checks/index.js create mode 100644 ui/packages/consul-ui/app/components/consul/instance-checks/index.scss create mode 100644 ui/packages/consul-ui/app/components/consul/service-instance/list/README.mdx create mode 100644 ui/packages/consul-ui/app/helpers/to-hash.js diff --git a/.changelog/12279.txt b/.changelog/12279.txt new file mode 100644 index 0000000000..efded667de --- /dev/null +++ b/.changelog/12279.txt @@ -0,0 +1,3 @@ +```release-note:bugfix +ui: Ensure proxy instance health is taken into account in Service Instance Listings +``` diff --git a/ui/packages/consul-ui/app/components/consul/instance-checks/README.mdx b/ui/packages/consul-ui/app/components/consul/instance-checks/README.mdx new file mode 100644 index 0000000000..1899ab6feb --- /dev/null +++ b/ui/packages/consul-ui/app/components/consul/instance-checks/README.mdx @@ -0,0 +1,96 @@ +# Consul::InstanceChecks + +A presentational component to show an overview/summary of service or node +health. + +```hbs preview-template +
+
With no checks
+ +
+
+
With all passing check
+ +
+
+
With a warning check
+ +
+
+
With a critical check
+ +
+
+
Nodes with a critical check
+ +
+``` + +## Arguments + +| Argument | Type | Default | Description | +| --- | --- | --- | --- | +| `type` | `(service | node )` | | A simple string to use for labelling | +| `items` | `Array` | | An array of Consul healthchecks | + + +## See + +- [Template Source Code](./index.hbs) + +--- diff --git a/ui/packages/consul-ui/app/components/consul/instance-checks/index.hbs b/ui/packages/consul-ui/app/components/consul/instance-checks/index.hbs index 2effe31c1b..1cd603a9e5 100644 --- a/ui/packages/consul-ui/app/components/consul/instance-checks/index.hbs +++ b/ui/packages/consul-ui/app/components/consul/instance-checks/index.hbs @@ -1,30 +1,49 @@ - {{#if (eq this.healthCheck.check 'empty') }} -
+{{#let + (group-by "Status" (or @items (array))) +as |grouped|}} + {{! Check the status of each Status of checks in order }} + {{! First one to have more than one check wins }} + {{! Otherwise we are empty }} + {{#let + (or + (if (gt grouped.critical.length 0) grouped.critical) + (if (gt grouped.warning.length 0) grouped.warning) + (if (gt grouped.passing.length 0) grouped.passing) + (array) + ) + as |checks|}} + {{#let + checks.firstObject.Status + as |status|}} +
{{capitalize @type}} Checks
-
No {{@type}} checks
+ {{#let + (or + (if (eq status 'critical') 'failing') + (if (eq status 'warning') 'with a warning') + status + ) + as |humanized|}} +
+ {{or + (if (eq checks.length 0) (concat 'No ' @type ' checks')) + (if (eq checks.length @items.length) (concat 'All ' @type ' checks ' humanized)) + (concat checks.length '/' @items.length ' ' @type ' checks ' humanized) + }} +
+ {{/let}}
- {{else}} - {{#if (eq this.healthCheck.count @items.length)}} -
-
- - {{capitalize @type}} Checks - -
-
All {{@type}} checks {{this.healthCheck.status}}
-
- {{else}} -
-
- - {{capitalize @type}} Checks - -
-
{{this.healthCheck.count}}/{{@items.length}} {{@type}} checks {{this.healthCheck.status}}
-
- {{/if}} - {{/if}} \ No newline at end of file + {{/let}} + {{/let}} +{{/let}} \ No newline at end of file diff --git a/ui/packages/consul-ui/app/components/consul/instance-checks/index.js b/ui/packages/consul-ui/app/components/consul/instance-checks/index.js deleted file mode 100644 index 2c78a553f0..0000000000 --- a/ui/packages/consul-ui/app/components/consul/instance-checks/index.js +++ /dev/null @@ -1,50 +0,0 @@ -import Component from '@glimmer/component'; - -export default class ConsulInstanceChecks extends Component { - get healthCheck() { - let ChecksCritical = 0; - let ChecksWarning = 0; - let ChecksPassing = 0; - - this.args.items.forEach(item => { - switch (item.Status) { - case 'critical': - ChecksCritical += 1; - break; - case 'warning': - ChecksWarning += 1; - break; - case 'passing': - ChecksPassing += 1; - break; - default: - break; - } - }); - - switch (true) { - case ChecksCritical !== 0: - return { - check: 'critical', - status: 'failing', - count: ChecksCritical, - }; - case ChecksWarning !== 0: - return { - check: 'warning', - status: 'with warning', - count: ChecksWarning, - }; - case ChecksPassing !== 0: - return { - check: 'passing', - status: 'passing', - count: ChecksPassing, - }; - default: - return { - check: 'empty', - }; - } - } -} diff --git a/ui/packages/consul-ui/app/components/consul/instance-checks/index.scss b/ui/packages/consul-ui/app/components/consul/instance-checks/index.scss new file mode 100644 index 0000000000..d97fe7b1f3 --- /dev/null +++ b/ui/packages/consul-ui/app/components/consul/instance-checks/index.scss @@ -0,0 +1,24 @@ +.consul-instance-checks { + & { + @extend %horizontal-kv-list; + } + dt::before { + @extend %as-pseudo; + } + &.passing dt::before { + @extend %with-check-circle-fill-mask; + color: rgb(var(--tone-green-500)); + } + &.warning dt::before { + @extend %with-alert-triangle-mask; + color: rgb(var(--tone-orange-500)); + } + &.critical dt::before { + @extend %with-cancel-square-fill-mask; + color: rgb(var(--tone-red-500)); + } + &.empty dt::before { + @extend %with-minus-square-fill-mask; + color: rgb(var(--tone-gray-500)); + } +} diff --git a/ui/packages/consul-ui/app/components/consul/service-instance/list/README.mdx b/ui/packages/consul-ui/app/components/consul/service-instance/list/README.mdx new file mode 100644 index 0000000000..c42da04a44 --- /dev/null +++ b/ui/packages/consul-ui/app/components/consul/service-instance/list/README.mdx @@ -0,0 +1,83 @@ +# Consul::ServiceInstance::List + +A presentational component to show a list of Service Instances. The component +will display slightly different information based on whether you want the list +in a `@node` view or not. + +Please note: A nice refactor here would be to let the node information be +added from the outside via a slot. Once component is using the new row based +scrollpane this can probably be looked at. + +```hbs preview-template +
+
With no node given (i.e. from within a Service)
+ + +
+``` + +Component configured to show a list from within a node page. + +```hbs preview-template +
+
With a node given (i.e. from within a Node)
+ + + +
+``` + +## Arguments + +| Argument | Type | Default | Description | +| --- | --- | --- | --- | +| `items` | `Array` | | An array of Consul Service Instances | +| `proxies` | `Array` | | An array of Consul Proxy Service Instances from the same Service | +| `routeName` | `String` | | An Ember routeName for where clicking a row takes you | +| `node` | `(Object | boolean)` | | Whether to show a node like view | + + +## See + +- [Template Source Code](./index.hbs) + +--- diff --git a/ui/packages/consul-ui/app/components/consul/service-instance/list/index.hbs b/ui/packages/consul-ui/app/components/consul/service-instance/list/index.hbs index eca61b81fa..36284cecbc 100644 --- a/ui/packages/consul-ui/app/components/consul/service-instance/list/index.hbs +++ b/ui/packages/consul-ui/app/components/consul/service-instance/list/index.hbs @@ -1,3 +1,8 @@ +{{#let + + (to-hash @proxies "Service.Proxy.DestinationServiceID") + +as |proxies|}} {{/if}} + +{{#let + (get proxies item.Service.ID) +as |proxy|}} + {{#let + (merge-checks + (array + item.Checks + (or + proxy.Checks + (array) + ) + ) + ) + as |checks|}} + {{#if @node}} - - + + {{else}} - - - + + + + {{/if}} -{{#if item.ProxyInstance}} +{{#if proxy}}
@@ -73,5 +108,10 @@ as |item index|>
{{/if}} + + {{/let}} +{{/let}} + - \ No newline at end of file + +{{/let}} \ No newline at end of file diff --git a/ui/packages/consul-ui/app/helpers/to-hash.js b/ui/packages/consul-ui/app/helpers/to-hash.js new file mode 100644 index 0000000000..60d60e4ce0 --- /dev/null +++ b/ui/packages/consul-ui/app/helpers/to-hash.js @@ -0,0 +1,12 @@ +import { helper } from '@ember/component/helper'; +import { get } from '@ember/object'; + +export default helper(([arrayLike = [], prop], hash) => { + if (!Array.isArray(arrayLike)) { + arrayLike = arrayLike.toArray(); + } + return arrayLike.reduce((prev, item, i) => { + prev[get(item, prop)] = item; + return prev; + }, {}); +}); diff --git a/ui/packages/consul-ui/app/models/node.js b/ui/packages/consul-ui/app/models/node.js index 874fe83e53..a403291cfe 100644 --- a/ui/packages/consul-ui/app/models/node.js +++ b/ui/packages/consul-ui/app/models/node.js @@ -28,6 +28,8 @@ export default class Node extends Model { // MeshServiceInstances are all instances that aren't connect-proxies this // currently includes gateways as these need to show up in listings @filter('Services', item => item.Service.Kind !== 'connect-proxy') MeshServiceInstances; + // ProxyServiceInstances are all instances that are connect-proxies + @filter('Services', item => item.Service.Kind === 'connect-proxy') ProxyServiceInstances; @computed('Checks.[]', 'ChecksCritical', 'ChecksPassing', 'ChecksWarning') get Status() { diff --git a/ui/packages/consul-ui/app/styles/components.scss b/ui/packages/consul-ui/app/styles/components.scss index 00e9d2981d..febe005c35 100644 --- a/ui/packages/consul-ui/app/styles/components.scss +++ b/ui/packages/consul-ui/app/styles/components.scss @@ -80,6 +80,7 @@ @import 'consul-ui/components/consul/upstream/list'; @import 'consul-ui/components/consul/upstream-instance/list'; @import 'consul-ui/components/consul/health-check/list'; +@import 'consul-ui/components/consul/instance-checks'; @import 'consul-ui/components/consul/exposed-path/list'; @import 'consul-ui/components/consul/external-source'; @import 'consul-ui/components/consul/kind'; diff --git a/ui/packages/consul-ui/app/templates/dc/nodes/show/services.hbs b/ui/packages/consul-ui/app/templates/dc/nodes/show/services.hbs index 3025734eca..770386b376 100644 --- a/ui/packages/consul-ui/app/templates/dc/nodes/show/services.hbs +++ b/ui/packages/consul-ui/app/templates/dc/nodes/show/services.hbs @@ -28,8 +28,9 @@ as |route|> ) route.model.item.MeshServiceInstances + route.model.item.ProxyServiceInstances - as |sort filters items|}} + as |sort filters items proxies|}}
{{#if (gt items.length 0) }} @@ -44,7 +45,6 @@ as |route|> @filter={{filters}} /> {{/if}} - {{! filter out any sidecar proxies }} as |collection|> diff --git a/ui/packages/consul-ui/app/templates/dc/services/show.hbs b/ui/packages/consul-ui/app/templates/dc/services/show.hbs index 5bfd29fd52..fb8b6a4704 100644 --- a/ui/packages/consul-ui/app/templates/dc/services/show.hbs +++ b/ui/packages/consul-ui/app/templates/dc/services/show.hbs @@ -214,6 +214,7 @@ as |items item dc|}} @name={{routeName}} @model={{assign (hash items=items + proxies=proxies item=item tabs=tabs ) route.model}} diff --git a/ui/packages/consul-ui/app/templates/dc/services/show/instances.hbs b/ui/packages/consul-ui/app/templates/dc/services/show/instances.hbs index f89f1c93ba..5cd8d22973 100644 --- a/ui/packages/consul-ui/app/templates/dc/services/show/instances.hbs +++ b/ui/packages/consul-ui/app/templates/dc/services/show/instances.hbs @@ -29,8 +29,9 @@ as |route|> ) route.model.items + route.model.proxies.firstObject -as |sort filters items|}} +as |sort filters items proxyMeta|}} {{#if (gt items.length 0) }} {{/if}} + {{#if proxyMeta.ServiceName}} + + {{/if}} {{! Service > Service Instance view doesn't require filtering of proxies }} diff --git a/ui/packages/consul-ui/tests/acceptance/dc/services/instances/navigation.feature b/ui/packages/consul-ui/tests/acceptance/dc/services/instances/navigation.feature index 931bc30530..6aa63c7bc4 100644 --- a/ui/packages/consul-ui/tests/acceptance/dc/services/instances/navigation.feature +++ b/ui/packages/consul-ui/tests/acceptance/dc/services/instances/navigation.feature @@ -11,14 +11,14 @@ Feature: dc / services / instances / navigation And 3 instance models from yaml --- - Service: - Name: service-0 + Service: service-0 ID: service-a Node: Node: node-0 Checks: - Status: critical - Service: - Name: service-0 + Service: service-0 ID: service-b Node: Node: node-0 @@ -29,7 +29,7 @@ Feature: dc / services / instances / navigation # proxy on request.0 from yaml', 'And 1 proxy on request.1 from yaml' or # similar - Service: - Name: service-0-proxy + Service: service-0-proxy ID: service-a-proxy Node: Node: node-0