Adding kernel specific metrics to nbresuse #40

[Gsbreddy]

This will address the issue #40

[Gsbreddy]

The sample metrics are attached here for reference. The kernel specific metrics can be found at the end of the file. This has hub metrics as well since I was testing from hub so please ignore them.

hubmetrics.txt

[jtpio]

Thanks @Gsbreddy!

[Gsbreddy]

Yes it does. Will add more features as discussed in future PR’s.

…

On Wed, 20 May 2020 at 23:16, Jeremy Tuloup ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In nbresuse/prometheus.py <#41 (comment)>: > + return None + + for session in sessions: + kernel = session['kernel'] + kernel_id = kernel['id'] + kernel_process = find_process(kernel_id) + if kernel_process: + kernel_memory[kernel_id] = kernel_process.memory_info()._asdict() + + raise gen.Return(kernel_memory) + + @staticmethod + def _kernel_processes(): + for proc in process_iter(): + try: + if 'ipykernel_launcher' in proc.cmdline(): It looks like this could be tweaked to also handle other kernels than ipykernel? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#41 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB26ANOWAMNOINIR6SS4AL3RSQJQNANCNFSM4NGFADAA> .

[jtpio]

Thanks @Gsbreddy for working on this.

I submitted a small fix in https://github.com/Gsbreddy/nbresuse/pull/1.

[jtpio]

Trying it locally with the xeus-python wheel (installed from PyPI). It looks like it is not picked up because the argv doesn't fully match the command of the process:

home/jtp/miniconda/envs/nbresuse/bin/python -m xpython_launcher -f /home/jtp/.local/share/jupyter/runtime/kernel-3cd39950-b840-44e4-b0e8-23ec9b780f1e.json

While the kernel.json spec in ${CONDA_PREFIX}/share/jupyter/kernels/xpython/kernel.json contains python3.8 only:

{
  "display_name": "xpython",
  "argv": [
      "python3.8",
      "-m",
      "xpython_launcher",
      "-f",
      "{connection_file}"
  ],
  "language": "python"
}

[Gsbreddy]

Looks like python3.8 in argv is getting expanded in process to that env specific python.

[jtpio]

Yes, with something like os.path.realpath("python3.8")

[jtpio]

It looks like it is coming from jupyter_client: https://github.com/jupyter/jupyter_client/blob/15b3b0566d962bbc042d21b858caeca2e870209a/jupyter_client/manager.py#L180-L189

Since argv[0] matches python3.8, it is then replaced by sys.executable.

[Gsbreddy]

Yeah, we can do the same here as well. Then the match will work.

[Gsbreddy]

I handled this using sys. Please check now.

[jtpio]

Just tested locally and the kernel_memory_usage shows up in the prometheus metrics after starting a new kernel:

# HELP kernel_memory_usage counter for kernel memory usage
# TYPE kernel_memory_usage gauge
kernel_memory_usage{kernel_id="06a46d91-5161-4e13-8800-9bca3f881a64"} 2.5636864e+07

However it will still be reported even after the kernel has been shut down. I guess we will want to remove it from the list? Otherwise the frontends will need to query the list of running kernels separately and filter the metrics taking into account only the running kernels?

[jtpio]

Although with Prometheus it's usually preferred for the metrics to be persisted (time series data).

[Gsbreddy]

This would give information on the usage trend of the user.

[jtpio]

Tested with ipykernel, xeus-python and the bash kernel, and they all seem to be listed in the metrics.

In follow-ups PRs, we could:

• add tests
• try to think of a way to query snapshots of the metrics. The previous API (with the JSON response) before the switch to Prometheus is still a convenient way for frontends to retrieve the data (see JupyterLab regression #36 for more context)

Thanks again @Gsbreddy for working on this 👍

cc @krinsman who has commit rights and might want to have a look.

[Gsbreddy]

I agree. I think we can go with #19 and have an API with previous JSON response(api/nbresuse/v1) for Jupyterlab to display on UI and leave /metrics for prometheus use. If not #19 then have to think of a way to achieve this.

I am working on making this work with JupyterLab. So I will update once done.

[jtpio]

I am working on making this work with JupyterLab. So I will update once done.

Using a JSON response from /api/nbresuse/v1? Or by parsing the Prometheus metrics with regexes?

[Gsbreddy]

Using JSON response. Similar to version 0.3.3

…

On Thu, 28 May 2020 at 16:25, Jeremy Tuloup ***@***.***> wrote: I am working on making this work with JupyterLab. So I will update once done. Using a JSON response from /api/nbresuse/v1? Or by parsing the Prometheus metrics with regexes? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#41 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB26ANKR6EO2CCDO22TLM2DRTY7JNANCNFSM4NGFADAA> .

[jtpio]

Sounds good!

[Gsbreddy]

I will raise a PR once I finish that work. Can we close this or should we wait till next PR?

[jtpio]

I am working on making this work with JupyterLab

Where do you plan to do that? In nbresuse? In JupyterLab? In another extension like the jupyterlab-system-monitor?

[Gsbreddy]

I am planning it in nbresuse and then make a subsequent change in JupyterLab to call new api(/api/nbresuse/v1) in nbresuse(As I mentioned, similar to 0.3.3).

[jtpio]

👍

(I thought it was about using the kernel metrics from this PR)

[Gsbreddy]

@krinsman @jtpio
Please review. -> https://github.com/Gsbreddy/nbresuse/pull/2
This solves yuvipanda#36

[jtpio]

Thanks @Gsbreddy, it looks promising!

I'll give it a try locally soon.

[Gsbreddy]

@jtpio

Tested with ipykernel, xeus-python and the bash kernel, and they all seem to be listed in the metrics.

In follow-ups PRs, we could:

• add tests
• try to think of a way to query snapshots of the metrics. The previous API (with the JSON response) before the switch to Prometheus is still a convenient way for frontends to retrieve the data (see JupyterLab regression #36 for more context)

Thanks again @Gsbreddy for working on this 👍

cc @krinsman who has commit rights and might want to have a look.

Can you take a look at this ?