adding awareness event when open and close websockets #246

jzhang20133 · 2024-03-12T16:26:43Z

Adding awareness event when open and close websockets

welcome · 2024-03-12T16:26:46Z

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! 🎉

github-actions · 2024-03-12T16:26:55Z

👈 Launch a Binder on branch jzhang20133/jupyter-collaboration/main-2

jupyter_collaboration/handlers.py

jupyter_collaboration/events/awareness.yaml

jupyter_collaboration/utils.py

jupyter_collaboration/events/awareness.yaml

davidbrochart · 2024-03-13T08:32:31Z

jupyter_collaboration/handlers.py

@@ -284,6 +286,8 @@ def on_close(self) -> None:
            # keep the document for a while in case someone reconnects
            self.log.info("Cleaning room: %s", self._room_id)
            self.room.cleaner = asyncio.create_task(self._clean_room())
+        if isinstance(self.room, DocumentRoom):


Why not emitting the event for all room types?

For YdocWebsocketHandler that handle global awareness websocket, its room type will be TransientRoom. I found its room id is fixed, called "JupyterLab:globalAwareness" and its creating and closing does not indicate any user joining or leaving the collaboration on a document. Hence we only emit when a websocket close where its room type is DocumentRoom.

Global awareness being a TransientRoom doesn't mean that clients leaving a document in a TransientRoom should not emit an event. Maybe the event should not be emitted specifically when the room ID is JupyterLab:globalAwareness? This room ID is a bit unfortunate, as it should probably be a standard in the Jupyter ecosystem and not particularly for JupyterLab, so a better would have been Jupyter:globalAwareness.
@Zsailer do you think this room ID should be specified somewhere in the Jupyter schemas, since it's probably a convention we want to rely on?

We would need to rely on RoomId to identity which document that the host has left from and close all the guest websockets associated as clients in that DocumentRoom. For a TransientRoom it seems that it is not tied to a specific document and mainly used by global awareness. We only want to emit an event when open and on_close has been successful as well.

All I'm saying is that you could do something like that:

Suggested change

if isinstance(self.room, DocumentRoom):

if self.room.room_id != "JupyterLab:globalAwareness":

A TransientRoom is not just for global awareness, it could be for any other collaborative document that doesn't need to be persisted, and you probably want to emit join/leave events for them too.

davidbrochart · 2024-03-13T08:33:11Z

jupyter_collaboration/handlers.py

@@ -297,6 +301,13 @@ def _emit(self, level: LogLevel, action: str | None = None, msg: str | None = No

        self.event_logger.emit(schema_id=JUPYTER_COLLABORATION_EVENTS_URI, data=data)

+    def _emit_awareness_event(self, user: str, action: str, msg: str | None = None) -> None:


Rename user to username?

will address

dlqqq

@jzhang20133 Thank you opening this PR! I left a comment below, but David Brochart's review covers the areas of feedback blocking this PR from being merged. 😁

All I would like to add is that it's possible to emit events through a decorator interface. Emitting events imperatively (as done in this PR) is less preferable, since it's really easy for a developer to accidentally delete the line emitting the event. Using a decorator also allows for this feature to be added without any changes to the method definition, which I take as a sign of clean code.

To implement a decorator interface, you would want to define an @emit_awareness_event decorator that looks something like:

def emit_awareness_event(action: str, message: Optional[str] = None):
    def decorator(method):
        async def method_stub(self, *args, **kwargs):
            return_value = await method(self, *args, **kwargs)
            data = { "action": action, ... }
            self.event_logger.emit(..., data=data)
            return return_value
        return method_stub
    return decorator

After doing so, the method declarations would look like:

    @emit_awareness_event("join")
    async def open(self, room_id):
        ...

    @emit_awareness_event("leave")
    def on_close(self) -> None:
        ...

Let me know if this all sounds like a good idea for future work. If so, I can track this in a new issue so as to not block this PR. Thanks! 🤗

dlqqq · 2024-03-13T21:14:02Z

jupyter_collaboration/handlers.py

@@ -297,6 +301,13 @@ def _emit(self, level: LogLevel, action: str | None = None, msg: str | None = No

        self.event_logger.emit(schema_id=JUPYTER_COLLABORATION_EVENTS_URI, data=data)

+    def _emit_awareness_event(self, user: str, action: str, msg: str | None = None) -> None:


(no action needed) I think we should prefer declaring the type of msg as Optional[str] instead of str | None, since the vertical bar syntax for type unions is only available for Python 3.10+. I thought the convention in Jupyter packages was to support every currently maintained version of Python, the lowest being Python 3.8 at present.

However, I also noticed that the rest of the source code uses the str | None. I've opened an issue so we can discuss this as a team. No action needed on this PR, just a comment.

It's fine to use str | None. Jupyter collaboration supports Python 3.8 and above. Union types as X | Y are allowed for python<3.10 with:

from __future__ import annotations

dlqqq · 2024-03-13T21:28:19Z

jupyter_collaboration/handlers.py

@@ -284,6 +286,8 @@ def on_close(self) -> None:
            # keep the document for a while in case someone reconnects
            self.log.info("Cleaning room: %s", self._room_id)
            self.room.cleaner = asyncio.create_task(self._clean_room())
+        if isinstance(self.room, DocumentRoom):


jzhang20133 · 2024-03-13T23:44:18Z

def emit_awareness_event(action: str, message: Optional[str] = None):
    def decorator(method):
        async def method_stub(self, *args, **kwargs):
            return_value = await method(self, *args, **kwargs)
            data = { "action": action, ... }
            self.event_logger.emit(..., data=data)
            return return_value
        return method_stub
    return decorator

I really love this idea! I just tried it out. In this use case, we only emit when websocket serving a DocumentRoom is successfully opened and closed and in open(self, room_id) method, when initialize room fails, it handles certain type exception by closing websocket with error message and swallows other type of exceptions. And for that failure cases, we won't emit this event. We would need to change original logic in open method in order to leverage decorator which defeat the purpose.

davidbrochart

Thanks @jzhang20133, I think this is good to go, but it would be great if you could add a test.

jzhang20133 · 2024-03-18T18:18:10Z

Thanks @jzhang20133, I think this is good to go, but it would be great if you could add a test.

@davidbrochart Thank you for reviewing the PR. I have added a test to cover emit awareness event.

Zsailer

This looks good, @jzhang20133!

I think we need to set the minimum version of Jupyter Events to the latest release, v0.10.0.

davidbrochart

Thanks @jzhang20133.

welcome · 2024-03-19T15:27:37Z

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

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

davidbrochart · 2024-03-20T14:28:04Z

@jzhang20133 Test test_room_handler_doc_client_should_emit_awareness_event fails in the CI, would you mind taking a look? Thanks!

Zsailer · 2024-03-20T22:29:38Z

@davidbrochart fixed here: #258

jzhang20133 requested review from davidbrochart, Zsailer and dlqqq March 12, 2024 16:30

jzhang20133 added the enhancement New feature or request label Mar 12, 2024

Zsailer reviewed Mar 12, 2024

View reviewed changes

jupyter_collaboration/handlers.py Outdated Show resolved Hide resolved

jupyter_collaboration/events/awareness.yaml Outdated Show resolved Hide resolved

jupyter_collaboration/events/awareness.yaml Outdated Show resolved Hide resolved

jzhang20133 force-pushed the main-2 branch from dfa1e7d to 7d47952 Compare March 12, 2024 19:58

davidbrochart reviewed Mar 13, 2024

View reviewed changes

dlqqq reviewed Mar 13, 2024

View reviewed changes

jzhang20133 force-pushed the main-2 branch from 7d47952 to 44d0f0d Compare March 13, 2024 23:51

jzhang20133 requested review from Zsailer, davidbrochart and dlqqq March 13, 2024 23:52

dlqqq mentioned this pull request Mar 14, 2024

Prefer decorators for emitting awareness events #249

Open

jzhang20133 force-pushed the main-2 branch from 44d0f0d to 9645266 Compare March 14, 2024 19:34

davidbrochart reviewed Mar 15, 2024

View reviewed changes

jzhang20133 force-pushed the main-2 branch from 9645266 to 8e9196b Compare March 18, 2024 18:16

jzhang20133 requested a review from davidbrochart March 18, 2024 18:18

adding awareness event when open and close websockets

1c2ed57

jzhang20133 force-pushed the main-2 branch from 8e9196b to 1c2ed57 Compare March 18, 2024 23:15

Zsailer approved these changes Mar 19, 2024

View reviewed changes

davidbrochart approved these changes Mar 19, 2024

View reviewed changes

jzhang20133 merged commit 265df16 into jupyterlab:main Mar 19, 2024
18 of 20 checks passed

Zsailer mentioned this pull request Mar 20, 2024

Bump jupyter_events to 0.10.0 #258

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding awareness event when open and close websockets #246

adding awareness event when open and close websockets #246

jzhang20133 commented Mar 12, 2024 •

edited

welcome bot commented Mar 12, 2024

github-actions bot commented Mar 12, 2024

davidbrochart Mar 13, 2024

dlqqq Mar 13, 2024

jzhang20133 Mar 13, 2024

davidbrochart Mar 14, 2024

jzhang20133 Mar 14, 2024

davidbrochart Mar 14, 2024

jzhang20133 Mar 14, 2024

davidbrochart Mar 13, 2024

jzhang20133 Mar 13, 2024

dlqqq left a comment

dlqqq Mar 13, 2024

davidbrochart Mar 14, 2024

dlqqq Mar 13, 2024

jzhang20133 commented Mar 13, 2024

davidbrochart left a comment

jzhang20133 commented Mar 18, 2024

Zsailer left a comment

davidbrochart left a comment

welcome bot commented Mar 19, 2024

davidbrochart commented Mar 20, 2024

Zsailer commented Mar 20, 2024

	if isinstance(self.room, DocumentRoom):
	if self.room.room_id != "JupyterLab:globalAwareness":

		@@ -297,6 +301,13 @@ def _emit(self, level: LogLevel, action: str \| None = None, msg: str \| None = No

		self.event_logger.emit(schema_id=JUPYTER_COLLABORATION_EVENTS_URI, data=data)

		def _emit_awareness_event(self, user: str, action: str, msg: str \| None = None) -> None:

adding awareness event when open and close websockets #246

adding awareness event when open and close websockets #246

Conversation

jzhang20133 commented Mar 12, 2024 • edited

welcome bot commented Mar 12, 2024

github-actions bot commented Mar 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlqqq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzhang20133 commented Mar 13, 2024

davidbrochart left a comment

Choose a reason for hiding this comment

jzhang20133 commented Mar 18, 2024

Zsailer left a comment

Choose a reason for hiding this comment

davidbrochart left a comment

Choose a reason for hiding this comment

welcome bot commented Mar 19, 2024

davidbrochart commented Mar 20, 2024

Zsailer commented Mar 20, 2024

jzhang20133 commented Mar 12, 2024 •

edited