-
Notifications
You must be signed in to change notification settings - Fork 51
introduce support for log level configuration #299
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
fix #160 and introduce multiple level of logs
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.
Thanks this is great. Just a couple of things though:
- I'm assuming you verified manually that the log level gets picked up from the env and then you can see log lines being output at that level or higher but not below. E.g. if env log level = info, then you should not see any debug lines in the logs.
- Have a look at the other comment in the review---trifling point :-)
- I added your TODOs about failing geocoding tests to the list of things to look into for #292, so we can deal with that separately, hope that's okay.
src/translators/crate.py
Outdated
self.cursor = self.conn.cursor() | ||
|
||
# IMPORTANT this reduce queries to crate, but it also means | ||
# that you need to redeploy ql in case of major crate update |
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.
Are you saying this because of global state? There's no global state here, you'll get a fresh translator instance each time the notify endpoint gets hit:
- https://github.com/smartsdk/ngsi-timeseries-api/blob/master/src/reporter/reporter.py#L172
- https://github.com/smartsdk/ngsi-timeseries-api/blob/master/src/translators/factory.py#L28
- https://github.com/smartsdk/ngsi-timeseries-api/blob/master/src/translators/crate.py#L1029
- https://github.com/smartsdk/ngsi-timeseries-api/blob/master/src/translators/base_translator.py#L19
Notice the use of a context manager
If that's what you were worrying about I suggest we get rid of this sentence "but it also means that you need to redeploy ql in case of major crate update" since it's going to be confusing.
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.
then we can remove the comment, still is a query that consume time and 99% with the same result, so if we have somewhere this a static value that is not refreshed every notification, may be a better option.
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 updated the TODO text
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.
yep, excellent point, I opened a separate issue just for that: #302
Yes, have verified that, not sure we can develop a test though. It may be quite a pain.
Did that
Top! |
totally. visual inspection is more than enough for me. Thanks for the PR, awesome work. I'm going to merge if there's nothing else you'd like to add. |
yes please fill free to merge |
fix #160 and introduce multiple level of logs
LOGLEVEL
env variable