Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
tacman opened this issue May 30, 2024 · 10 comments
Open

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

tacman opened this issue May 30, 2024 · 10 comments

Comments

@tacman
Copy link
Contributor

tacman commented May 30, 2024

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.

@tacman tacman changed the title shows up with sqlite database shows "status: up" with missing sqlite database May 30, 2024
@parijke
Copy link
Contributor

parijke commented May 31, 2024

@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
Copy link
Contributor Author

tacman commented May 31, 2024

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 [email protected]: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
Copy link
Contributor

tvdijen commented May 31, 2024

@tacman
Copy link
Contributor Author

tacman commented May 31, 2024

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
Copy link
Contributor

parijke commented Jun 2, 2024

@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
Copy link
Contributor

parijke commented Jun 2, 2024

@tacman can you test this PR?
#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
Copy link
Contributor Author

tacman commented Jun 2, 2024

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
Copy link
Contributor Author

tacman commented Jun 2, 2024

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

@MKodde
Copy link
Member

MKodde commented Jun 3, 2024

@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
Copy link
Member

thijskh commented Aug 21, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants