-
Notifications
You must be signed in to change notification settings - Fork 67
Add inspection modes to Python client library #1418
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
melange396
left a comment
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.
Pretty nice!
Could you, instead of or in addition to being in the main log messages, put all the variables reported into their own keyword arguments? That will allow for easier post-processing, among other things.
src/client/delphi_epidata.py
Outdated
| """Make request with a retry if an exception is thrown.""" | ||
| request_url = f"{Epidata.BASE_URL}/{endpoint}/" | ||
| if Epidata.debug: | ||
| Epidata.logger.info(f"Sending GET request to URL: {request_url} | params: {params} | headers: {_HEADERS}") |
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.
should probably log Epidata.auth here too since its used in the request
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.
suggest(non-blocking): since we're using structlog, you can feed params in like a dict and it will handle the formatting, like
Epidata.logger.info("Sending GET request", url=request_url, params=params, headers=_HEADERS)I would play with this and make sure the output looks reasonable (since I haven't done it myself), but since params and _HEADERS are probably dicts, their repr() method should make them look nice too. And then structlog will render it all in a consistent JSON.
dshemetov
left a comment
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.
Small suggestion about log message formatting, otherwise looks good 👍
src/client/delphi_epidata.py
Outdated
| """Make request with a retry if an exception is thrown.""" | ||
| request_url = f"{Epidata.BASE_URL}/{endpoint}/" | ||
| if Epidata.debug: | ||
| Epidata.logger.info(f"Sending GET request to URL: {request_url} | params: {params} | headers: {_HEADERS}") |
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.
suggest(non-blocking): since we're using structlog, you can feed params in like a dict and it will handle the formatting, like
Epidata.logger.info("Sending GET request", url=request_url, params=params, headers=_HEADERS)I would play with this and make sure the output looks reasonable (since I haven't done it myself), but since params and _HEADERS are probably dicts, their repr() method should make them look nice too. And then structlog will render it all in a consistent JSON.
dshemetov
left a comment
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.
lgtm
melange396
left a comment
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.
add in the auth logging, and then i think this is done!
Co-authored-by: george <[email protected]>
|
|
@melange396 @dshemetov re-requesting review as the latest commit invalidated the "approved" checkmarks :) |




Closes #1321.
Summary:
Adds two debug modes to the Python client library (JS and R clients TBD):
debuglogs info about HTTP requests and responses.sandboxprevents any HTTP requests from actually executing, allowing users to test a long-running or expensive script without any actual server load.Prerequisites:
devbranchdev