-
Notifications
You must be signed in to change notification settings - Fork 67
Add logging to the API server #1027
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
Conversation
- i also wanna add tests (but get eyes on this first) where stream processing is paused for a few secs in various places, to make sure those numbers come out in the logs. - *_PROBABLY_* shouldnt add too much load to the server? - in this, i am comparing 2 ways/places to store start time reference, i fear concurrent connections or threads may walk all over one or the other. one will probably be removed eventually. - couldnt quite figure out log statement formatting with named args/attributes... if anyone wants to help inform the ignorant, i am all ears.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great start. i havent gotten to the changes in dates.py
yet but i will soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like a typical successful request will have ~4 log emissions, right?
-
those messages appear to include:
- 1 pre-request message
- 1 post-request message
- 1+ [probable] sql execute message(s)
- 1 date range adjustment message [ & 1 more when guessing day/week (really only happens in wiki.py and twitter.py endpoints) ]
- 0-1 potential validation failure message
- 0+ potential more serious error message
-
we can also tune some log levels:
ValidationException
messages should be info- or warn-level instead of error- most of the stuff in
dates.py
can be debug-level (the one should stay error, though) - if we also make sure the prod webserver logs @ info and the dev environment logs @ debug, itll take 25% off of the message counts in elastic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think these last few changes will about do it for this pr!
can you provide us with sample log outputs to compare what they look like before and after this pr? maybe do one with the logs emitted when processing a request to the covidcast endpoint, and then one from some other endpoint?
|
@melange396 dates.py logs are still at INFO level, not DEBUG. is that intentional? |
@krivard - Rostyslav wanted to see the output of them in production, so we agreed to leave them INFO for some period of time. as soon as this pr gets merged, i will create an issue to turn them down. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 Nice work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well done!
Closes #1002.
Prerequisites:
dev
branchdev
Summary
Adds far more extra logging statements to the Covidcast server code. This includes:
Suggestions for other code paths or metrics that can be logged are welcome and encourage :)