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

Consistent logging method #607

Merged

Conversation

mwakaba2
Copy link
Member

@mwakaba2 mwakaba2 commented Nov 6, 2021

Problem

This closes #605

Two methods in the class ZMQChannelsHandler were using the logging function directly instead of self.log method to emit logs. We already have a default format set for all Jupyter server logs, so this PR updates the following methods' logging calls to keep the format consistent.

  • on_kernel_restarted
  • on_restart_failed

Changes made

  • Replace all logs emitted with direct use of logging with the instantiated class's own logging method: self.log.
  • Add optional log parameter to list_running_servers to add missing warning log for failed server info file deletion.

Tests

While trying to reproduce the logs with logging, I found out that logs emitted with the logging function weren't getting propagated.

To reproduce the issue, I tested logging in the ZMQChannelsHandler.on_iopub method - code. (I didn't know how to reproduce/trigger calls to ZMQChannelsHandler.<on_kernel_restarted, on_restart_failed>). I replaced self.log withlogging.

def on_iopub(msg):
            self.log.debug("!!!!!!!!!!! UPDATED LOGGING METHOD")
            logging.debug("Nudge: IOPub received: %s", self.kernel_id)
            if not iopub_future.done():

To emit the logs, I did the following:

  1. Start Jupyter server: jupyter server --ServerApp.log_level=DEBUG --ServerApp.jpserver_extensions='{"jupyterlab": True}'
  2. Create a python kernel
  3. Restarte the python kernel

The "IoPub received" log wasn't emitted.

[D 2021-11-06 10:15:34.214 ServerApp] !!!!!!!!!!! UPDATED LOGGING METHOD
[D 2021-11-06 10:15:34.214 ServerApp] Nudge: resolving iopub future: e1b8b87c-4d10-4782-bad7-50e7cf1cf732
[D 2021-11-06 10:15:34.214 ServerApp] Nudge: shell info reply received: e1b8b87c-4d10-4782-bad7-50e7cf1cf732

With self.log, the logs are emitted:

[D 2021-11-06 10:15:07.030 ServerApp] !!!!!!!!!!! UPDATED LOGGING METHOD
[D 2021-11-06 10:15:07.030 ServerApp] Nudge: IOPub received: 1e48ec11-17c0-487b-bd53-f24bce96617a
[D 2021-11-06 10:15:07.030 ServerApp] Nudge: resolving iopub future: 1e48ec11-17c0-487b-bd53-f24bce96617a
[D 2021-11-06 10:15:07.031 ServerApp] Nudge: shell info reply received: 1e48ec11-17c0-487b-bd53-f24bce96617a

At some point, we may want to figure out a way to encourage usage of self.log instead of logging.

@mwakaba2 mwakaba2 self-assigned this Nov 6, 2021
@codecov-commenter
Copy link

codecov-commenter commented Nov 6, 2021

Codecov Report

Merging #607 (e4a8cb4) into master (3ecb4e0) will decrease coverage by 0.08%.
The diff coverage is 12.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #607      +/-   ##
==========================================
- Coverage   77.42%   77.34%   -0.09%     
==========================================
  Files         110      110              
  Lines       10252    10252              
  Branches     1258     1259       +1     
==========================================
- Hits         7938     7929       -9     
- Misses       1917     1927      +10     
+ Partials      397      396       -1     
Impacted Files Coverage Δ
jupyter_server/services/kernels/handlers.py 60.82% <0.00%> (-0.11%) ⬇️
jupyter_server/serverapp.py 65.47% <16.66%> (-0.07%) ⬇️
jupyter_server/services/kernels/kernelmanager.py 79.79% <0.00%> (-2.06%) ⬇️
jupyter_server/tests/services/kernels/test_api.py 97.02% <0.00%> (-1.99%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ecb4e0...e4a8cb4. Read the comment docs.

@mwakaba2
Copy link
Member Author

mwakaba2 commented Nov 6, 2021

I also did a quick check for any log.warn usage in the codebase, but I found none. All the warning log calls are using log.warning.

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @mwakaba2 - thank you.

@kevin-bates kevin-bates merged commit a1b013e into jupyter-server:master Nov 6, 2021
@mlucool
Copy link

mlucool commented Nov 6, 2021

Thanks!

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

Successfully merging this pull request may close these issues.

Logging: all logs should use the same format
4 participants