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

Log how long each extension module takes to import #1171

Merged
merged 3 commits into from
Jan 12, 2023

Conversation

yuvipanda
Copy link
Contributor

@yuvipanda yuvipanda commented Jan 12, 2023

Based on some detailed debugging in
2i2c-org/infrastructure#2047 to figure out why a Jupyter Server process sometimes takes more than 30s
to start, the primary culprit was server extensions that took multiple seconds to just even import. Thanks to some ad-hoc patching (2i2c-org/infrastructure#2047 (comment)), I was able to figure out which were the slow extensions. This PR emits extension import time as log messages, so this information is much more visible.

I also explicitly chose info instead of debug, primarily because I believe it is very important to surface this performance information to users, so they can go bug the appropriate extension. Otherwise, it just feels like 'jupyter server is slow!'. This is compounded by the fact that while notebook server doesn't import disabled extensions, jupyter_server does seem to - so it's hard to isolate this.

Based on some detailed debugging in
2i2c-org/infrastructure#2047
to figure out why a Jupyter Server *process* somtimes takes more than
30s
to start, the primary culprit was server extensions that took
multiple seconds to just even import. Thanks to some ad-hoc
patching (2i2c-org/infrastructure#2047 (comment)),
I was able to figure out which were the slow extensions.
This PR emits extension import time as log messages, so this
information is *much* more visible.

I also explicitly chose info instead of debug, primarily because
I believe it is *very* important to surface this performance
information to users, so they can go bug the appropriate extension.
Otherwise, it just feels like 'jupyter server is slow!'. This is
compounded by the fact that while notebook server doesn't import
*disabled* extensions, jupyter_server does seem to - so it's hard
to isolate this.
@codecov
Copy link

codecov bot commented Jan 12, 2023

Codecov Report

Base: 79.93% // Head: 79.56% // Decreases project coverage by -0.36% ⚠️

Coverage data is based on head (7e6daf7) compared to base (a52709c).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1171      +/-   ##
==========================================
- Coverage   79.93%   79.56%   -0.37%     
==========================================
  Files          68       68              
  Lines        8124     8130       +6     
  Branches     1601     1602       +1     
==========================================
- Hits         6494     6469      -25     
- Misses       1205     1222      +17     
- Partials      425      439      +14     
Impacted Files Coverage Δ
jupyter_server/extension/utils.py 84.44% <100.00%> (+2.39%) ⬆️
jupyter_server/utils.py 81.76% <0.00%> (-3.53%) ⬇️
jupyter_server/services/contents/filemanager.py 73.42% <0.00%> (-1.86%) ⬇️
jupyter_server/extension/handler.py 67.69% <0.00%> (-1.54%) ⬇️
...ter_server/services/kernels/connection/channels.py 58.37% <0.00%> (-1.11%) ⬇️
jupyter_server/services/contents/fileio.py 89.06% <0.00%> (-1.05%) ⬇️
jupyter_server/base/handlers.py 78.43% <0.00%> (-0.40%) ⬇️
jupyter_server/serverapp.py 79.57% <0.00%> (-0.36%) ⬇️
jupyter_server/gateway/managers.py 83.13% <0.00%> (-0.24%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@yuvipanda
Copy link
Contributor Author

Lint failure seems totally unrelated?

@yuvipanda yuvipanda requested a review from jtpio January 12, 2023 05:11
@davidbrochart
Copy link
Contributor

Lint failure seems totally unrelated?

Thanks @yuvipanda, yes it is unrelated.

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Thanks!

@blink1073 blink1073 merged commit 8ca5f5e into jupyter-server:main Jan 12, 2023
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.

Thanks Steve! (Sorry, I had reviewed, but got side tracked prior to submitting my approval.)

Update: Damn, just realized I commented on the wrong PR! (Thanks Yuvi, and Steve & David anyway though 😄)

@yuvipanda
Copy link
Contributor Author

Thanks a lot for getting this out quickly, @davidbrochart and @blink1073 :)

yuvipanda added a commit to yuvipanda/jupyter_server that referenced this pull request Jan 14, 2023
Without this, the log messages printed in
jupyter-server#1171
don't actually make it out. Whoops!

LoggingConfigurable inherits from HasTraits, and sets up
Logging appropriately so we can have a log passed through.
yuvipanda added a commit to yuvipanda/jupyter_server that referenced this pull request Jan 14, 2023
Without this, the log messages printed in
jupyter-server#1171
don't actually make it out. Whoops!

LoggingConfigurable inherits from HasTraits, and sets up
Logging appropriately so we can have a log passed through.
kevin-bates pushed a commit that referenced this pull request Jan 14, 2023
Without this, the log messages printed in
#1171
don't actually make it out. Whoops!

LoggingConfigurable inherits from HasTraits, and sets up
Logging appropriately so we can have a log passed through.
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.

4 participants