-
Notifications
You must be signed in to change notification settings - Fork 310
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
Basic Event Bus #820
Basic Event Bus #820
Conversation
Codecov Report
@@ Coverage Diff @@
## main #820 +/- ##
==========================================
+ Coverage 69.98% 70.12% +0.13%
==========================================
Files 63 65 +2
Lines 7570 7621 +51
Branches 1262 1268 +6
==========================================
+ Hits 5298 5344 +46
- Misses 1892 1895 +3
- Partials 380 382 +2
Continue to review full report at Codecov.
|
Heads up, you'll have to resolve the conflict by adding the |
One thing brought up off-line was the need for a message buffer in the EventBus. If the event bus websocket closes, messages should be buffered until the next time a websocket connects. |
Do we need to handle the case of multiple frontends and making sure each of
them has gotten each message?
…On Wed, May 4, 2022 at 9:45 AM Zachary Sailer ***@***.***> wrote:
One thing brought up off-line was the need for a message buffer in the
EventBus.
If the event bus websocket closes, messages should be buffered until the
next time a websocket connects.
—
Reply to this email directly, view it on GitHub
<#820 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAGXUHAUGTD2KDWL34RUWDVIKSUFANCNFSM5UXQMZ2Q>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
--
Brian E. Granger
Senior Principal Technologist, AWS AI/ML ***@***.***)
On Leave - Professor of Physics and Data Science, Cal Poly
@ellisonbg on GitHub
|
Yes, definitely! This should already work here. Each frontend opens its own websocket; when the websocket is opened, it appends a new logging handler to main EventBus that routes event data to that websocket. When the websocket closes, it removes its logging handler. @afshin and I discussed how this should map to events for a specific user in a multi-user context. Since the server knows "who" the user is (even easier with the new identity API), we can add proper routing so that users only see events relevant to them. I think the buffering and user filtering can be handled at the logging handler layer. The EventBus object receives, validates, and emits all events. The individual logging handlers take care of where these messages should go—following how the standard python logging library work. |
Another question that came up for me: how will we route events? Will we use
a topic/subtopic system, similar to how ZeroMQ pub/sub channels work? Will
we have a more complex rule/pattern matching system that determines when
events are sent (see AWS Event Bridge (
https://docs.aws.amazon.com/eventbridge/latest/userguide/eb-rules.html) for
an example of this type of routing). I am guessing we want to keep is as
simple as possible, but we still may need some level of semantic routing.
…On Wed, May 4, 2022 at 10:32 AM Zachary Sailer ***@***.***> wrote:
Do we need to handle the case of multiple frontends and making sure each of
them has gotten each message?
Yes, definitely! This should already work here. Each frontend opens its
own websocket; when the websocket is opened, it appends a new logging
handler to main EventBus that routes event data to that websocket. When the
websocket closes, it removes its logging handler.
@afshin <https://github.com/afshin> and I discussed how this should map
to events for a specific user in a multi-user context. Since the server
knows "who" the user is (even easier with the new identity API), we can add
proper routing so that users only see events relevant to them.
I think the buffering and user filtering can be handled at the logging
handler layer. The EventBus object receives, validates, and emits all
events. The individual logging handlers take care of where these messages
should go—following how the standard python logging library work.
—
Reply to this email directly, view it on GitHub
<#820 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAGXUHBW36TEF33BFQKZEDVIKYCJANCNFSM5UXQMZ2Q>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Brian E. Granger
Senior Principal Technologist, AWS AI/ML ***@***.***)
On Leave - Professor of Physics and Data Science, Cal Poly
@ellisonbg on GitHub
|
Let's discuss this at tomorrow's meeting, but I think this is ready to go. This adds the necessary base layer for an event system in Jupyter Server. We can enhance its functionality over time. |
event_bus = Instance( | ||
EventBus, | ||
allow_none=True, | ||
help="An EventBus for emitting structured event data from Jupyter Server and extensions.", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Event bus is instantiated down on line 1925. What is the purpose of this statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is merely defining the trait type and validation that traitlets should do once the trait is created on line 1925. This doesn't do the actual instantiation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining.
In the previous PR #364 we also used entrypoints to allow server extensions to log server events and importlib_resources to load schemas during schema registration but these could be discussed more and added later. |
@kiendang |
@kiendang's referring to discussion we had around using Python entry-points to make schemas defined/living in separate python packages (e.g. extensions) discoverable by Jupyter Server. I think would be a nice feature to add soon, but I agree that we can do this in a follow up PR. @kiendang, would you be interested in tackling this? Entrypoints shouldn't be our only way of discovering schemas, since we don't want to require extension authors to write a Python package to register schemas. There are many use cases where someone might want to register and log events through Jupyter Server from a non-Python-based extension. |
Sure. Will submit a PR once this is merged. |
Co-authored-by: Afshin Taylor Darian <git@darian.email>
@Zsailer Are there any other changes you are planning to add to this PR? |
@3coins, I think we need to create a |
@blink1073 the "Check Release" workflow seems to be failing because it cannot find the If so, I think we can merge here and iterate on the event work. |
Yeah, I'll look into this today. Thanks for working on this! |
Sweet! 🎉 |
@Zsailer, I don't have permissions to create a new package in jupyter, I can work with you to move the relevant pieces from telemetry if you get a chance to create one. |
Adds a basic
EventBus
as a service to Jupyter Server.Part of #780.
Leaving this in draft state for further discussion at our next Jupyter Server Meeting.
The tests show examples of how this can be used. It uses the
EventLog
object injupyter_telemetry
to load schemas and validate events against those schemas.Adds a
/api/events/subscribe
endpoint that opens a websocket where events will be broadcast and client applications can listen. The handler is properly "auth"-ed (authentication/authorization).I made the
EventBus
a singleton, so that it can be easily accessed between the core server, extensions, and handlers. We might want to discuss if this is the best design, since singletons can be a little tricky to maintain.Ping @afshin
Todo: