Skip to content
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

Merged
merged 9 commits into from
May 20, 2022
Merged

Basic Event Bus #820

merged 9 commits into from
May 20, 2022

Conversation

Zsailer
Copy link
Member

@Zsailer Zsailer commented Apr 30, 2022

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 in jupyter_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:

  • Unit tests

@Zsailer Zsailer added this to the 2.0 milestone Apr 30, 2022
@codecov-commenter
Copy link

codecov-commenter commented May 1, 2022

Codecov Report

Merging #820 (cadc063) into main (d376780) will increase coverage by 0.13%.
The diff coverage is 90.19%.

@@            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     
Impacted Files Coverage Δ
jupyter_server/services/events/handlers.py 86.84% <86.84%> (ø)
jupyter_server/base/handlers.py 64.00% <100.00%> (+0.20%) ⬆️
jupyter_server/serverapp.py 65.28% <100.00%> (+0.22%) ⬆️
jupyter_server/services/events/bus.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d376780...cadc063. Read the comment docs.

@blink1073
Copy link
Contributor

Heads up, you'll have to resolve the conflict by adding the jupyter_telemetry dependency to pyproject.toml

@Zsailer
Copy link
Member Author

Zsailer commented May 4, 2022

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.

@ellisonbg
Copy link
Contributor

ellisonbg commented May 4, 2022 via email

@Zsailer
Copy link
Member Author

Zsailer commented May 4, 2022

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 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.

@ellisonbg
Copy link
Contributor

ellisonbg commented May 5, 2022 via email

@Zsailer
Copy link
Member Author

Zsailer commented May 11, 2022

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.

Comment on lines +1572 to +1576
event_bus = Instance(
EventBus,
allow_none=True,
help="An EventBus for emitting structured event data from Jupyter Server and extensions.",
)
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining.

@kiendang
Copy link
Contributor

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.

@3coins
Copy link
Contributor

3coins commented May 12, 2022

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
Do you mean rest api endpoints? These are one of the line items added in the task list in the parent issue. It would be helpful if you can review and provide feedback. Thanks for your help.
#780 (comment)

@Zsailer
Copy link
Member Author

Zsailer commented May 12, 2022

In the previous PR #364 we also used entrypoints

Do you mean rest api endpoints?

@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.

@kiendang
Copy link
Contributor

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?

Sure. Will submit a PR once this is merged.

@3coins
Copy link
Contributor

3coins commented May 19, 2022

@Zsailer Are there any other changes you are planning to add to this PR?

@Zsailer
Copy link
Member Author

Zsailer commented May 19, 2022

@3coins, I think we need to create a jupyter_events package to avoid depending on jupyter_telemetry before 2.0 final release. We can do this in another PR, though. Otherwise, I think this is ready to go.

@Zsailer
Copy link
Member Author

Zsailer commented May 19, 2022

@blink1073 the "Check Release" workflow seems to be failing because it cannot find the bootstrap.css. Is this related to the hatch build system switch?

If so, I think we can merge here and iterate on the event work.

@blink1073
Copy link
Contributor

Is this related to the hatch build system switch?

Yeah, I'll look into this today. Thanks for working on this!

@blink1073 blink1073 merged commit 1361a37 into jupyter-server:main May 20, 2022
@afshin
Copy link
Contributor

afshin commented May 20, 2022

Sweet! 🎉

@3coins
Copy link
Contributor

3coins commented May 21, 2022

@3coins, I think we need to create a jupyter_events package to avoid depending on jupyter_telemetry before 2.0 final release. We can do this in another PR, though. Otherwise, I think this is ready to go.

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants