AspNetCore.Diagnostics.HealthChecks icon indicating copy to clipboard operation
AspNetCore.Diagnostics.HealthChecks copied to clipboard

Group healthchecks either via configured group, or discovered namespace

Open Meertman opened this issue 3 years ago • 8 comments

What this PR does / why we need it: This PR allows the end-user to group his health check endpoints into either custom defined groups, or in case the end-user is using the kubernetes discovery it will group the health checks using the namespace in which the pods that execute the healthchecks are hosted.

Which issue(s) this PR fixes: #827, #788, #690, groundwork for #809

Please reference the issue this PR will close: #827 #788 #690

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • [X] Code compiles correctly
  • [ ] Created/updated tests
  • [x] Unit tests passing
  • [X] End-to-end tests passing
  • [ ] Extended the documentation
  • [ ] Provided sample for the feature

Meertman avatar Aug 31 '21 15:08 Meertman

Credits where credits are due, the UI work has been done by @GeniaT

Meertman avatar Aug 31 '21 15:08 Meertman

I love the idea of adding groupings to the HealthCheckUI, but I had to add "dom.iterable" to tsconfig.json -> "compilerOptions" -> "lib" in HealthChecksUI project, to get this to run (in Visual Studio, not Docker).

And still when I run the HealthChecks.UI.Branding sample I see this: image And this when expanded (which can only be done with the expand all button: image

I would assume that groups are optional and that all health checks without a group would end up in a default group?

BoSoeborgPetersen avatar Sep 17 '21 08:09 BoSoeborgPetersen

@BoSoeborgPetersen, sorry, I did indeed forget to check in that change to the tsconfig.json file. (I added it to the PR)

So yes, health checks without a group end up in the default group. Which currently has no name :)

Meertman avatar Sep 17 '21 09:09 Meertman

@Meertman, from what I can tell, the backend works as expected, but there are some frontend issues:

There is a problem with the expand/collapse button at the group level:

  • Default group: it does nothing
  • Other groups: it works, but it also expands the health checks in the group I think the problem is that the typescript does not react well to the default group having null as the name

I would also suggest that:

  • Groups have a different background color than health checks
  • If only the default group exists, then just display the health checks without the group level (no API changes, just UI), this would ensure that current users don't see any changes because of this feature, in the UI, and they can then choose to use this feature. But maybe this should also be part of the API, so the data structure is the same for people not using this feature, otherwise we could break peoples use of the API from somewhere other than the UI.

I also found a possible mistake on line 58 in IEndpointRouteBuilderExtensions.cs, it should probably be AddAsync, instead of RemoveAsync.

I have added the background color for groups locally, and will look further at the other parts, but I am limited by my javascript/typescript/frontend skills, and I am also not sure where to push my changes, possibly a fork of your fork.

BoSoeborgPetersen avatar Sep 17 '21 12:09 BoSoeborgPetersen

It would be nice to be able to include the group name in webhook notifications. Perhaps even the state of the overall group? For example, if I am monitoring 10 endpoints within a group, I may care less if 1 of the 10 is offline since the other 9 could pick up the slack. However, if 9 of 10 are offline, then I would care much more.

smbecker avatar Dec 23 '21 20:12 smbecker

I am also limited by my knowledge of React, so if someone could take a look at the UI part, this might be merged as well?

Meertman avatar Mar 01 '22 11:03 Meertman

We are also really needing this functionality, is there any way to get this merged?

Moerup avatar May 06 '22 12:05 Moerup

@Meertman, from what I can tell, the backend works as expected, but there are some frontend issues:

There is a problem with the expand/collapse button at the group level:

  • Default group: it does nothing
  • Other groups: it works, but it also expands the health checks in the group I think the problem is that the typescript does not react well to the default group having null as the name

I would also suggest that:

  • Groups have a different background color than health checks
  • If only the default group exists, then just display the health checks without the group level (no API changes, just UI), this would ensure that current users don't see any changes because of this feature, in the UI, and they can then choose to use this feature. But maybe this should also be part of the API, so the data structure is the same for people not using this feature, otherwise we could break peoples use of the API from somewhere other than the UI.

I also found a possible mistake on line 58 in IEndpointRouteBuilderExtensions.cs, it should probably be AddAsync, instead of RemoveAsync.

I have added the background color for groups locally, and will look further at the other parts, but I am limited by my javascript/typescript/frontend skills, and I am also not sure where to push my changes, possibly a fork of your fork.

@BoSoeborgPetersen for your information, @Tom-De-Backer has implemented these changes and I've added them to the pull request.

@CarlosLanderas, @unaizorrilla, @carlosrecuero, @sungam3r, can we get these changes merged with the main repo?

Meertman avatar Oct 07 '22 14:10 Meertman

@Meertman Sorry but I'm not familiar and I don't use such areas of this repo as docker/JS/UI so better to ask maintainers. I just help to keep things moving in HC packages, CI and around some other stuff.

sungam3r avatar Oct 22 '22 21:10 sungam3r