liip / LiipMonitorBundle

Integrates the LiipMonitor library into Symfony
http://liip.ch
467 stars 103 forks source link

[RFC] 3.x #281

Closed kbond closed 9 months ago

kbond commented 1 year ago

Looking for feedback on the next iteration of this bundle.

See README for docs, specifically the upgrade notes.

TODO

wouterj commented 1 year ago

Removing the JSON endpoint is a bit of a bummer for us, as we use this endpoint in our uptime monitor (monitoring the server being up, and having access to all important external services in 1 call). But it's quite easy to introduce this in our applications instead, and I can understand why you don't want to penetrate the application layer in a bundle :)

kbond commented 1 year ago

I'm cool keeping the json endpoint but it would be a specific implementation. If there are specific standard formats different services require, I'm fine adding these. The main thing I'd like to remove is the html web interface.

I've made the ResultSet JsonSerializable with a basic implementation. Apps can add their own normalizer(s).

There are just so many ways you'd want the endpoint to behave. Use status codes? Do warnings constitute failures?

Should I perhaps keep the existing json endpoints as-is so there's no BC break there?

wouterj commented 1 year ago

There are just so many ways you'd want the endpoint to behave. Use status codes? Do warnings constitute failures?

Should I perhaps keep the existing json endpoints as-is so there's no BC break there?

About the behavior: I have experience with 3 uptime monitors, and all of them required me to specify when something is considered up (e.g. status code, parsing JSON, etc.). I think the current JSON endpoint is great for this purpose, as it allows quick set-up (using globalStatus) and more fine-grained control (using each check's status).

Ultimately, it's your call. I can see the value in not having to maintain the routing config and controller, and implementing this in an application is very easy thanks to the serializable results.

tarjei commented 11 months ago

I would +1 the JSON endpoint. Also I use the nagios checks all the time. They do not depend on anything other than the JSON endpoing so the code there is mainly just basic pointers for people running various monitoring tools.

What I would love to see is an option for a check to add a link in it's response so that an operator can follow that link to some kind of interface for handling the error. For example when I got some failing errors in a queue, linking to a list of the events in that wueue that failed is very usefull. Also this can be used to add runbooks and other documentation to a monitor.

kbond commented 11 months ago

Thanks for the feedback @tarjei. What you're describing is actually why I'm against having the end point to the bundle itself: too many custom implementations. This PR improves the DX of the bundle to a point where this is trivial. For instance, I use a health monitoring service in my app and with this PR it's just a few lines of bridge code in a controller. It takes the check results and maps them to the json this service requires.

Here's the simplest controller:

#[Route('/health-check/{suite?}', name: 'health_check')]
class HealthCheckController extends AbstractController
{
    public function __invoke(CheckRegistry $checkRegistry, ?string $suite): Response
    {
        $results = $checkRegistry->suite($suite)->run();

        return new JsonResponse(status: $results->failures()->count() ? 502 : 200);
    }
}

I'm not against adding standard, health monitoring service specific ones. For instance, I use OhDear - I'd be fine adding an OhDear controller. Maybe we can add the simplest nagios implementation? (I know nothing about nagios)