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.

Sign up for GitHub

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

Jump to bottom

Refine logging of database creation #721

Open
danielballan opened this issue Apr 19, 2024 · 1 comment · May be fixed by #726
Open

Refine logging of database creation #721

danielballan opened this issue Apr 19, 2024 · 1 comment · May be fixed by #726
Assignees
@dylanmcreynolds

Comments

@danielballan
Copy link
Member

I like that #714 logs catalog creation, but I think ERROR-level logging is not quite right. I suspect the original thinking was, "We're capturing 'standard error', so log to 'error'." But you get effects like this which I think can create the wrong impression that something is broken:

tiled/_tests/test_access_control.py::test_service_principal_access 
-------------------------------------------------------- live log call ---------------------------------------------------------
2024-04-19 10:36:30 [   ERROR] Subprocess stderr: Database sqlite+aiosqlite:////tmp/pytest-of-dallan/pytest-80/test_service_principal_access0/catalog.db is new. Creating tables.
Database initialized.
 (adapter.py:1314)
2024-04-19 10:36:32 [   ERROR] Subprocess stderr:  (adapter.py:1314)
PASSED

Maybe we can change this to INFO-level logging. I am imagine @dylanmcreynolds will be happy as long as the log message is visible for his use case(s). Can you past in here the workflows where you want to see this log message, Dylan?
@dylanmcreynolds dylanmcreynolds self-assigned this Apr 19, 2024
@dylanmcreynolds
Copy link
Contributor

Yes, the intent was that we log the database creation in "info" while logging any errors seen in "error". This was a miss. As a user I can't imagine not wanting to see everything. If there are deployment processes that treat error differently from info, then we have to think really hard about how to do this.

The issue of having no logging at all has hit me a few times in container scenarios (usually messing up an environment variable for database settings that results in the database not initing...then when tiled starts up, it errors out because the database is not configured and there's no good indication that this is because the subprocess failed. I know that we need to be cautious of over-logging, but I think it's worth being slightly verbose on startup. I assigned this issue to myself.

@dylanmcreynolds dylanmcreynolds linked a pull request Apr 23, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dylanmcreynolds dylanmcreynolds

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix subprocess logging during database creation dylanmcreynolds/tiled
2 participants
@danielballan @dylanmcreynolds