OpenConext / Monitor-bundle

Openconext bundle for monitoring endpoints
Apache License 2.0
5 stars 2 forks source link

shows "status: up" with missing sqlite database #20

Open tacman opened 4 months ago

tacman commented 4 months ago

I updated the bundle, when to /internal/health, all good.

I then deleted my sqlite database, and it still said it was good.

However, a bad postgres connection does correctly show status: down.

parijke commented 4 months ago

@tacman I do not undertstand this correctly I think. You state SQLite and Postgress. Can you share the exact error from the log? And the exact steps to reproduce?

tacman commented 4 months ago

Here's a simple demo that uses sqlite. When you delete the database the home page returns a 500, but the monitor shows it's okay.

git clone git@github.com:survos-sites/barcode-demo.git && cd barcode-demo
composer install
symfony server:start -d
symfony open:local 
symfony open:local --path="/internal/health"
read -p "press any key to delete the database" -n 1 -s
rm products.db
symfony open:local 
symfony open:local --path="/internal/health"
tvdijen commented 4 months ago

https://github.com/survos-sites/barcode-demo/blob/main/config/packages/doctrine.yaml#L41 I think you're caching query results.

tacman commented 4 months ago

That's only in prod.

The issue is, of course, that the website is down if you delete the database and open the site. I guess that's what I'm expecting the monitor to catch. If the home page is broken but the /health status says "up", I must be mis-configuring something.

parijke commented 3 months ago

@tacman Thanks for the repo, it helps me investigating the problem(s)

I am currently investigating it... seeing that for SQLite, the behaviour is that the database is created (empty) if it is not there. Theherfor, the check that it exists and a connection can be made will succeed.

Then we check if it has tables, and only then, it will execute the test query. If not, nothing happens at the moment. Maybe we should throw an exception if the tables are empty as well.

@thijskh do we want to support SQLite as well?

parijke commented 3 months ago

@tacman can you test this PR? https://github.com/OpenConext/Monitor-bundle/pull/22/

In the manuals, the default behaviour of the Sqlite3 driver, is to create the database(file) if it does not exists https://www.php.net/manual/en/sqlite3.open.php

tacman commented 3 months ago

Hmm. So if the database exists, it passes. Then there are no tables (which is legit), so it passes.

Gut feeling is that the better solution is to allow the user to define one or more queries that can optionally be run, as discussed before.

Or drop SQLite? I use SQLite for testing so I don't have to spin up a database, but on the production site, where I use Postgres, the monitor works as expected.

tacman commented 3 months ago

Ugh, I forget how to test a PR on a 3rd-party library!

MKodde commented 3 months ago

@tacman sometimes I clone the project with the vender code I want to test. And then create a symlink to that clone in my vendor folder. Alternatively you can add a 'repositories' section in your composer.json. And install the monitor bundle dependency from a branch you specify there.

https://getcomposer.org/doc/05-repositories.md

thijskh commented 1 month ago

Gut feeling is that the better solution is to allow the user to define one or more queries that can optionally be run, as discussed before.

We'd definitely be open to a PR that would provide the SQL query as a configurable string.