report the memory usage metrics as prometheus metrics #22
Conversation
|
@tommassino We would probably want to keep NBResuse compatible with Traitlets and the rest of the Jupyter ecosystem, for the purposes of long-term maintainability. Would it be possible to expand on these initial commits to use In any case this PR could be useful for restoring NBResuse's compatibility with Notebook (see: #17 ) as well as implementing a desired feature request for JupyterLab (see: jupyterlab/jupyterlab#7663 ). |
|
Hey @krinsman, ill look into it. The PR is pretty stale, however it seems like an easy modification. |
|
Check it out. About the warning threshold. Before they were dumped into the json, which was fine. But i dont really want to dump them as a prometheus metric, as they have nothing to do with metrics. Ideally I would like to somehow have them available on the frontend. However I do not know how to get config in the javascript, is that possible? Another thing is I removed the run_on_executor decorator from the cpu metrics, not sure why it was there and it did not seem necessary to me, feel free to explain and i can put it back somehow. I dont feel its necessary to extend IPythonHandler since this method does not actually handle any requests, which is the whole point of IPythonHandler. The handler simply updates the metrics that are registered in the prometheus publisher. But i dont really know much about jupyter extensions, so feel free to suggest changes. Or if there is some officially supported way to do a periodic callback, that would be probably good too. |
There was a problem hiding this comment.
If it's not clear what changes I'm asking for, or if you think they would be too difficult to implement, then please don't worry about it.
I just want to add the Prometheus feature to NBResuse slowly so that NBResuse doesn't suddenly break for people not using Prometheus. I also feel personally responsible for any changes that I merge into master, and so I feel uncomfortable merging a switch to mandatory Prometheus-based metric collection when I don't personally understand and thus can't personally vouch for all of the code involved.
That being said, this is a nice feature which I want NBResuse to have, especially since multiple people are already using it. I think I also recall Yuvi mentioning once that collecting metrics with Prometheus would ultimately make NBResuse more extensible with regards to the metrics it can collect. So I think the long term goal might be to switch entirely to Prometheus. But again, we would have to coordinate with the JupyterLab team about whether it would be possible to make Prometheus a mandatory dependency, and etc.
Also, if we have to make psutil optional in order for this to ever be merged into JupyterLab, then it probably wouldn't be that difficult to also make prometheus_client optional in an analogous way. My thinking about it is detailed more here. Part of the issue is that I'm confused about whether Prometheus also needs psutil installed in order to work -- if it does, then it seems we would ultimately have to make Prometheus optional anyway if psutil itself is being made optional.
| from notebook.base.handlers import IPythonHandler | ||
| from tornado import web | ||
|
|
||
| from nbresuse.prometheus import PrometheusHandler |
There was a problem hiding this comment.
After merging this, I might want to make prometheus optional by having the metrics reporting done this way if prometheus_client is available, and have it done with the old "prometheus-stupid" if not available.
Some people I have talked to said that they don't want to install Prometheus, I don't really understand why since I don't know anything about Prometheus to be honest.
If you could maybe implement this logic yourself (see e.g. this StackOverflow answer https://stackoverflow.com/a/24640526 ) it would be very helpful for me and save me some time after merging this PR. That being said if it's not clear what I'm requesting I can spend some hours myself implementing it.
| @@ -0,0 +1,64 @@ | |||
| from notebook.notebookapp import NotebookApp | |||
There was a problem hiding this comment.
I appreciate you using the same Callable here that was used elsewhere. Ultimately it probably doesn't matter from a functional point of view (i.e. no substance difference), but I imagine that future code maintainers will disproportionately be people (like myself) who have more experience with the "Jupyter"y tools like traitlets then other implementations of the same features, so sticking with those might enhance the code's long-term maintainability.
So anyway in the interests of long-term maintainability I am glad that you did that.
I also like the more sophisticated file structure (e.g. instead of squeezing everything into __init__.py) -- that sort of separation of concerns I think will also improve NBResuse's long-term maintainability. Which is something that would likely be important if it were to ever be merged into JupyterLab.
| return ( size / Math.pow(1024, i) ).toFixed(1) * 1 + ' ' + ['B', 'kB', 'MB', 'GB', 'TB'][i]; | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
So is the idea of this function to automatically parse the JSON emitted by the backend NBResuse server extension and format it correctly?
It does seem to substantially simplify the displayMetrics function, so it seems like a good idea to me. I guess I'm just somewhat unclear about its purpose based on the function name only.
| }else | ||
| return matches; | ||
| } | ||
|
|
||
| var displayMetrics = function() { | ||
| if (document.hidden) { | ||
| // Don't poll when nobody is looking | ||
| return; | ||
| } |
There was a problem hiding this comment.
Would this code be easily modifiable to also report CPU usage information if that is also available?
If not, don't worry about it, that would just be a nice extra plus, and I realize the fact that we can't assume/guarantee that the CPU information will be available for the client to parse probably increases the complexity of the logic required to implement this substantially.
|
@tommassino There might be some hiccups we have to deal with later, but that's fine because they won't affect anything already released on PyPI. And given how deeply prometheus is already deeply integrated into notebook and JupyterLab, it makes a lot of sense to integrate NBResuse into it as well. This will hopefully in the long term also make it easier to integrate NBResuse into them. So let me tag the commit corresponding to the 0.3.3 PyPI release first, to make sure everything is documented, and then I will go ahead and merge this. Good work with all of this, and thank you so much for contributing all of your time and intelligence to this project! |
|
Great news, maybe i can revive the branch with reporting metrics for specific kernels now :) |
@tommassino are you referring to this branch? https://github.com/Tommassino/nbresuse/tree/kernel-metrics It would indeed be really useful to have more granular metrics for each kernel! I guess we could open a new issue to track this? |
|
@tommassino ditto @jtpio's comment: that would be really great feature to have! If you've already implemented it and want to share it, please feel free to open a PR! |
Not sure whether this is something you would want to include in the extension, but since right now the extension overwrites prometheus metrics I wanted to fix that.