Emit events from the kernels service and gateway client #1252

rajmusuku · 2023-03-31T22:43:31Z

Event emitters using Jupyter events for GatewayClient and GatewayMappingKernelManager.

Introduced event schemas for GatewayClient and GatewayMappingKernelManager
Injected jupyter event logger from jupyter_events in the above classes.
Intercepted the Restful calls into Enterprise Gateway.
Emit event messages within Jupyter server.

welcome · 2023-03-31T22:43:33Z

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

codecov · 2023-03-31T22:50:10Z

Codecov Report

Patch coverage: 77.41% and project coverage change: +0.56 🎉

Comparison is base (ca4b062) 79.88% compared to head (9c5db61) 80.45%.

❗ Current head 9c5db61 differs from pull request most recent head 28bc3ab. Consider uploading reports for the commit 28bc3ab to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1252      +/-   ##
==========================================
+ Coverage   79.88%   80.45%   +0.56%     
==========================================
  Files          69       69              
  Lines        8299     8416     +117     
  Branches     1607     1635      +28     
==========================================
+ Hits         6630     6771     +141     
+ Misses       1226     1219       -7     
+ Partials      443      426      -17

Impacted Files	Coverage Δ
jupyter_server/serverapp.py	`80.63% <ø> (+1.50%)`	⬆️
jupyter_server/gateway/gateway_client.py	`74.03% <43.33%> (-2.39%)`	⬇️
jupyter_server/gateway/managers.py	`85.37% <79.41%> (-0.11%)`	⬇️
jupyter_server/services/kernels/kernelmanager.py	`83.54% <93.33%> (+1.72%)`	⬆️

... and 7 files with indirect coverage changes

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

rajmusuku · 2023-03-31T22:56:57Z

fixes #1253 #1253

kevin-bates

Hi @rajmusuku,

Thank for you opening this pull request, this is really great! I do have a concern that it introduces a schema that is specific to gateway kernel management when a general kernel management schema would be sufficient (and desired) and I think we should consider adopting a common schema for kernel management and from which GatewayKernelManager also emits events. We introduced ServerKernelManager earlier for this specific reason for non-gateway kernels.

Since this PR is beating us to the mark for generic kernel management events, I think it would be good to at least generalize the schema you have here to be the schema used for kernel management events in general. We could then extend ServerKernelManager to emit the events from the same schema.

I'd be happy to help out with this if you like. Thanks for getting this ball rolling!

jupyter_server/event_schemas/gateway/client/v1.yaml

jupyter_server/event_schemas/gateway/mapping_kernel_manager/v1.yaml

jupyter_server/gateway/managers.py

rajmusuku · 2023-04-01T00:23:28Z

@kevin-bates Thanks for your comment. It make sense to generalize the schema for Kernel service, in fact that was the initial thought to emit all the Kernel events. I would be more interested to capture failures than the regular transactions. Please let me know how can we work together on it.

kevin-bates · 2023-04-03T15:29:00Z

Hi @rajmusuku. I think we should go ahead and generalize the kernel lifecycle event schema. I think it makes sense to include a failure indication on the events but we can talk about. We should review the generalized schema in the server and kernel meeting on Thursday.

If you'd like to get together prior to Thursday, please reach out to me via the email address associated with my GH profile. If others would like to be involved in the initial discussion, please comment with where meeting invitations can be sent.

cc: @Zsailer

jupyter_server/event_schemas/gateway/client/v1.yaml

jupyter_server/event_schemas/kernels/v1.yaml

jupyter_server/event_schemas/gateway/client/v1.yaml

jupyter_server/event_schemas/kernels/v1.yaml

Zsailer · 2023-04-06T14:50:21Z

Thanks for the ping, @kevin-bates. This is great!

I'll spend some time today reviewing this more closing. I've been OOO sick the past week, so I'm just now catching up.

rajmusuku · 2023-04-07T00:40:05Z

@kevin-bates @Zsailer Pushed changes as per our conversation. Please take a look, and let me know. Thanks!

Zsailer · 2023-04-07T03:31:20Z

Thanks @rajmusuku!

I also push a new commit to your branch that begins emitting events from the kernel manager directly. If we agree with this approach, we can remove the emission from the MappingKernelManager.

We should also add some unit tests around these events. I'll start working on this...

Zsailer · 2023-04-07T03:52:45Z

Ok, I've added unit tests too 🚀

jupyter_server/event_schemas/kernel_actions/v1.yaml

jupyter_server/gateway/managers.py

kevin-bates · 2023-04-07T15:20:50Z

jupyter_server/services/kernels/kernelmanager.py

+    async def start_kernel(self, *args, **kwargs):
+        result = await super().start_kernel(*args, **kwargs)
+        self.emit(
+            schema_id="https://events.jupyter.org/jupyter_server/kernel_actions/v1",


Could we make this a constant? Unlike the field names, this will likely change (per version) and seems worthy of a constant.

Yeah. The only reason I didn't do this was, in the future, we might have multiple kernel schemas—e.g. a kernel state schema—so I was trying to make each call to the emit(...) method explicit and decoupled. That said, I don't think that's a strong enough argument. I can make a constant for this...

jupyter_server/services/kernels/kernelmanager.py

jupyter_server/event_schemas/gateway_client/v1.yaml

jupyter_server/gateway/gateway_client.py

jupyter_server/gateway/managers.py

jupyter_server/services/kernels/kernelmanager.py

Zsailer · 2023-04-12T23:53:57Z

Thank for tracking down the codecov issue, @kevin-bates!

We can address the that mess in a separate PR. 😅

kevin-bates

This is great stuff - thank you @rajmusuku and @Zsailer!

@rajmusuku - I'm hoping you can take this for a spin in your environment where you actually have an event listener configured.

rajmusuku · 2023-04-13T04:14:37Z

Thank you @Zsailer, @kevin-bates for your input and help. Yes, will verify it tomorrow and let you know. However, we can merge this PR.

kevin-bates · 2023-04-13T14:39:35Z

I'm hoping you can take this for a spin in your environment where you actually have an event listener configured.

However, we can merge this PR.

Hi @rajmusuku - I made my comment because there aren't gateway-specific tests for confirming the event's emission from GatewayKernelManager. I apologize for not being more clear about that aspect but was hesitant to ask. I can try to address this today

kevin-bates · 2023-04-14T14:38:52Z

@rajmusuku - I apologize for not getting to this yesterday. However, at closer glance, the following tests are failing as part of this PR and we should address this anyway - beyond adding code to validate emitted events...

tests/test_gateway.py::test_gateway_session_lifecycle[False] FAILED      [  3%]
tests/test_gateway.py::test_gateway_session_lifecycle[True] FAILED       [  3%]
tests/test_gateway.py::test_gateway_kernel_lifecycle[False] FAILED       [  4%]
tests/test_gateway.py::test_gateway_kernel_lifecycle[True] FAILED        [  4%]
tests/test_gateway.py::test_gateway_shutdown[True] FAILED                [  4%]
tests/test_gateway.py::test_gateway_shutdown[False] FAILED               [  4%]

These failures were lost in the general failures for the codecov snafu. I'll try to take a closer look shortly. (Right, famous last words. 😄). We'll need to hold the merge until these are resolved.

kevin-bates · 2023-04-14T16:51:59Z

Welp, it turns out this comment was incorrect, and derivation from AsyncIOLoopKernelManager is (somehow) problematic. I'd like to better understand this before duplicating the decorator. When deriving from AILKM, we are unable to obtain an instance here:

km = self.kernel_manager_factory(parent=self, log=self.log)

kevin-bates · 2023-04-14T17:51:00Z

Welp, it turns out #1252 (comment) was incorrect, and derivation from AsyncIOLoopKernelManager is (somehow) problematic. I'd like to better understand this before duplicating the decorator.

It (actually) turns out to have been an incompatibility between GatewayKernelManager and ServerKernelManager because the latter (correctly) types last_activity and the former was setting its value to None. I've gone ahead and set initial values (to execution_state as well) and pushed the correction. The gateway tests should pass.

I'd still like to get some event testing for gateway completed and will focus on that shortly. Sorry for the delays!

…eway-client

kevin-bates · 2023-04-14T20:56:27Z

jupyter_server/services/kernels/kernelmanager.py

+    @default("event_logger")
+    def _default_event_logger(self):
+        """Initialize the logger and ensure all required events are present."""
+        if self.parent is not None and hasattr(self.parent, "event_logger"):


@Zsailer - because the event_logger trait is on the ServerKernelManager and not the AsyncMappingKernelManager, do we need the double-hop of parent to access the configurable on ServerApp here? I.e.,:

Suggested change

if self.parent is not None and hasattr(self.parent, "event_logger"):

if self.parent is not None and self.parent.parent is not None and hasattr(self.parent.parent, "event_logger"):

tests/services/kernels/test_events.py

rajmusuku · 2023-04-15T19:16:35Z

@Zsailer @kevin-bates Just tested with my extension, however, the event listener is not catching the emitted events. Will do some investigation and let you know.

rajmusuku · 2023-04-17T04:34:26Z

@Zsailer @kevin-bates Just tested with my extension, however, the event listener is not catching the emitted events. Will do some investigation and let you know.

@Zsailer @kevin-bates Let's merge this PR as we are good with all the changes. I will work on more testing with my extension. Please let me know if you have any questions.

kevin-bates · 2023-04-17T15:04:59Z

@Zsailer @kevin-bates Just tested with my extension, however, the event listener is not catching the emitted events. Will do some investigation and let you know.

@rajmusuku - how are you registering your handler on your event logger? I found that I had to use register_handler() rather than directly append it to (or set) the logger's handler array.

Let's merge this PR as we are good with all the changes.

@blink1073 - Do you have an idea regarding the code coverage stall?

blink1073 · 2023-04-17T15:26:36Z

I removed those checks from the main branch, new PRs should not have them.

Zsailer · 2023-04-17T15:39:54Z

Just tested with my extension, however, the event listener is not catching the emitted events. Will do some investigation and let you know.

I think we should add documentation about events in jupyter_server before merging. I'll work on this today.

kevin-bates · 2023-04-17T15:41:07Z

@Zsailer - I'm good to go here, are you good to go here? 😄

Zsailer · 2023-04-17T15:42:33Z

I'm good to go here, are you good to go here?

Let me open a separate PR showing the documentation I propose.

We don't have to block on that other PR being merged, but lets at least wait until I open that documentation PR (give me a hour or two).

Zsailer · 2023-04-17T20:49:57Z

Ok, I opened #1260 to begin documenting the events emitted by Jupyter Server. I'll keep that in draft state until I'm able to get the auto-generation of event schema documentation working.

In the meantime, I think this is ready to go. Merging!

welcome · 2023-04-17T20:52:08Z

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️

kevin-bates reviewed Apr 1, 2023

View reviewed changes

kevin-bates added the feature label Apr 1, 2023

kevin-bates reviewed Apr 4, 2023

View reviewed changes

jupyter_server/event_schemas/gateway/client/v1.yaml Outdated Show resolved Hide resolved

kevin-bates reviewed Apr 4, 2023

View reviewed changes

jupyter_server/event_schemas/kernels/v1.yaml Outdated Show resolved Hide resolved

kevin-bates reviewed Apr 4, 2023

View reviewed changes

jupyter_server/event_schemas/gateway/client/v1.yaml Outdated Show resolved Hide resolved

kevin-bates reviewed Apr 5, 2023

View reviewed changes

jupyter_server/event_schemas/kernels/v1.yaml Outdated Show resolved Hide resolved

Zsailer mentioned this pull request Apr 6, 2023

Meeting Notes 2023 jupyter-server/team-compass#45

Closed

Zsailer force-pushed the raj/events-gateway-client branch from 0f77466 to 74b76e2 Compare April 7, 2023 04:04

kevin-bates reviewed Apr 7, 2023

View reviewed changes

jupyter_server/event_schemas/kernel_actions/v1.yaml Show resolved Hide resolved

kevin-bates reviewed Apr 7, 2023

View reviewed changes

jupyter_server/gateway/managers.py Outdated Show resolved Hide resolved

kevin-bates reviewed Apr 7, 2023

View reviewed changes

jupyter_server/services/kernels/kernelmanager.py Outdated Show resolved Hide resolved

kevin-bates reviewed Apr 7, 2023

View reviewed changes

jupyter_server/services/kernels/kernelmanager.py Outdated Show resolved Hide resolved

Zsailer reviewed Apr 7, 2023

View reviewed changes

jupyter_server/event_schemas/gateway_client/v1.yaml Outdated Show resolved Hide resolved

Zsailer reviewed Apr 7, 2023

View reviewed changes

jupyter_server/gateway/gateway_client.py Outdated Show resolved Hide resolved

Zsailer reviewed Apr 7, 2023

View reviewed changes

jupyter_server/gateway/managers.py Outdated Show resolved Hide resolved

Zsailer reviewed Apr 7, 2023

View reviewed changes

jupyter_server/gateway/managers.py Outdated Show resolved Hide resolved

Zsailer changed the title ~~Event emitters using Jupyter events for GatewayClient and GatewayMappingKernelManager~~ Event events from the kernels service and gateway client Apr 7, 2023

rajmusuku commented Apr 10, 2023

View reviewed changes

jupyter_server/services/kernels/kernelmanager.py Outdated Show resolved Hide resolved

rajmusuku commented Apr 10, 2023

View reviewed changes

jupyter_server/services/kernels/kernelmanager.py Outdated Show resolved Hide resolved

Zsailer mentioned this pull request Apr 10, 2023

allow a 'message' field in an event schema jupyter/jupyter_events#72

Merged

clean-up after code review

b669c81

Zsailer force-pushed the raj/events-gateway-client branch from 7d0f52c to b669c81 Compare April 12, 2023 23:49

kevin-bates approved these changes Apr 13, 2023

View reviewed changes

Make GatewayKernelManager a compatible subclass to ServerKernelManager

4f0050a

Merge remote-tracking branch 'upstream' into rajmusuku-raj/events-gat…

fb6575f

…eway-client

kevin-bates self-requested a review April 14, 2023 20:51

kevin-bates reviewed Apr 14, 2023

View reviewed changes

tests/services/kernels/test_events.py Show resolved Hide resolved

kevin-bates added 2 commits April 14, 2023 17:35

Add events to gateway lifecycle test

56af18e

Access event_logger trait from ServerApp

28bc3ab

Zsailer mentioned this pull request Apr 17, 2023

Document event logging #1260

Closed

1 task

Zsailer enabled auto-merge (squash) April 17, 2023 20:50

Zsailer disabled auto-merge April 17, 2023 20:50

Zsailer merged commit 5c49253 into jupyter-server:main Apr 17, 2023

Zsailer mentioned this pull request Apr 20, 2023

Add event emitters using Jupyter events for GatewayClient and GatewayMappingKernelManager #1253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit events from the kernels service and gateway client #1252

Emit events from the kernels service and gateway client #1252

rajmusuku commented Mar 31, 2023

welcome bot commented Mar 31, 2023

codecov bot commented Mar 31, 2023 •

edited

Loading

rajmusuku commented Mar 31, 2023

kevin-bates left a comment

rajmusuku commented Apr 1, 2023

kevin-bates commented Apr 3, 2023

Zsailer commented Apr 6, 2023

rajmusuku commented Apr 7, 2023

Zsailer commented Apr 7, 2023

Zsailer commented Apr 7, 2023

kevin-bates Apr 7, 2023

Zsailer Apr 7, 2023

Zsailer commented Apr 12, 2023

kevin-bates left a comment

rajmusuku commented Apr 13, 2023

kevin-bates commented Apr 13, 2023

kevin-bates commented Apr 14, 2023

kevin-bates commented Apr 14, 2023

kevin-bates commented Apr 14, 2023

kevin-bates Apr 14, 2023

rajmusuku commented Apr 15, 2023

rajmusuku commented Apr 17, 2023

kevin-bates commented Apr 17, 2023

blink1073 commented Apr 17, 2023 •

edited

Loading

Zsailer commented Apr 17, 2023

kevin-bates commented Apr 17, 2023

Zsailer commented Apr 17, 2023

Zsailer commented Apr 17, 2023

welcome bot commented Apr 17, 2023

	if self.parent is not None and hasattr(self.parent, "event_logger"):
	if self.parent is not None and self.parent.parent is not None and hasattr(self.parent.parent, "event_logger"):

Emit events from the kernels service and gateway client #1252

Emit events from the kernels service and gateway client #1252

Conversation

rajmusuku commented Mar 31, 2023

welcome bot commented Mar 31, 2023

codecov bot commented Mar 31, 2023 • edited Loading

Codecov Report

rajmusuku commented Mar 31, 2023

kevin-bates left a comment

Choose a reason for hiding this comment

rajmusuku commented Apr 1, 2023

kevin-bates commented Apr 3, 2023

Zsailer commented Apr 6, 2023

rajmusuku commented Apr 7, 2023

Zsailer commented Apr 7, 2023

Zsailer commented Apr 7, 2023

kevin-bates Apr 7, 2023

Choose a reason for hiding this comment

Zsailer Apr 7, 2023

Choose a reason for hiding this comment

Zsailer commented Apr 12, 2023

kevin-bates left a comment

Choose a reason for hiding this comment

rajmusuku commented Apr 13, 2023

kevin-bates commented Apr 13, 2023

kevin-bates commented Apr 14, 2023

kevin-bates commented Apr 14, 2023

kevin-bates commented Apr 14, 2023

kevin-bates Apr 14, 2023

Choose a reason for hiding this comment

rajmusuku commented Apr 15, 2023

rajmusuku commented Apr 17, 2023

kevin-bates commented Apr 17, 2023

blink1073 commented Apr 17, 2023 • edited Loading

Zsailer commented Apr 17, 2023

kevin-bates commented Apr 17, 2023

Zsailer commented Apr 17, 2023

Zsailer commented Apr 17, 2023

welcome bot commented Apr 17, 2023

codecov bot commented Mar 31, 2023 •

edited

Loading

blink1073 commented Apr 17, 2023 •

edited

Loading