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

Show API summary #56

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

davidbrochart
Copy link
Contributor

See #55

@davidbrochart davidbrochart marked this pull request as draft December 22, 2021 12:11
@davidbrochart
Copy link
Contributor Author

Now looks like this. Websockets are listed at the bottom with a red background for the paths (warning for some potential security issue).

image

@davidbrochart davidbrochart marked this pull request as ready for review December 23, 2021 13:46
@adriendelsalle
Copy link
Member

Thanks @davidbrochart ! I'll have a look

@adriendelsalle
Copy link
Member

@davidbrochart what about making rich an optional dependency with conditional import and optional display of the summary?

@davidbrochart
Copy link
Contributor Author

We discussed that with @SylvainCorlay, but the point of this PR is making it mandatory to display the API, so that you don't allow e.g. websockets by mistake.
If depending on rich is an issue, we could also show the summary without it, but it probably wouldn't be as beautiful 😄
On the other hand, I think we could make FPS a Rich/Textual application, instead of having a stream of log messages.

@adriendelsalle
Copy link
Member

Hey @davidbrochart sorry for the delay..
I don't see the point of displaying the API in the console at server start-up except for development when you need to check quickly if everything is correctly implemented.
It's convenient but it also doesn't substitute to unit/integration tests (which is the correct way to do it IMHO) at plugins and app levels to ensure code quality and features fully operational.

I would prefer to make this a dev extra dependency, maybe also move this to fps-uvicorn plugin since the CLI is there and I'm not sure this makes sense for fps itself to print console outputs.

On the other hand, I think we could make FPS a Rich/Textual application, instead of having a stream of log messages.

I don't really get it, could you please expand a bit more?

@fcollonval
Copy link
Member

I support @adriendelsalle on this one. The goal is to have a plugin approach to be able to build the lighter server possible. So everything should be optional including the way we want to display output.

For example log message are really important to be kept as such in deployed scenarios where admin sys usually aggregate them in monitoring third-party service.

@davidbrochart
Copy link
Contributor Author

I don't see the point of displaying the API in the console at server start-up except for development when you need to check quickly if everything is correctly implemented.

It can also be helpful to ensure there is no security risk, especially if you want to check if the websocket endpoints are served.

On the other hand, I think we could make FPS a Rich/Textual application, instead of having a stream of log messages.

I don't really get it, could you please expand a bit more?

FPS could be have terminal UI, show a dashboard etc.

I support @adriendelsalle on this one. The goal is to have a plugin approach to be able to build the lighter server possible. So everything should be optional including the way we want to display output.

I think FPS being a TUI doesn't break the plugin approach, and it can still be lightweight.

For example log message are really important to be kept as such in deployed scenarios where admin sys usually aggregate them in monitoring third-party service.

We could also dump logs to a file.

@fcollonval
Copy link
Member

The main point here is separation of concern. FPS is (and should focus) on being a pluggable server. Any kind on UI should be a plugin or a separate beast. So why not placing this in a plugin. Indeed the current PR is not breaking it. But we should keep leverage the plugin system every time we can otherwise we will create yet another mammoth out of laziness.

@davidbrochart
Copy link
Contributor Author

I agree with that.
This PR needs access to some of FPS's internals, so I guess we should provide an API to expose them.

@davidbrochart
Copy link
Contributor Author

This last commit implements (the beginning of) a dashboard, as an FPS plugin. It currently only shows the API summary.
The approach here is to keep FPS as an application that logs to stdout/stderr, and fps-dashboard extracts information from these streams. The log's format will need to be specified.
fps-dashboard executes in a different process than fps-uvicorn.

@davidbrochart
Copy link
Contributor Author

Actually I'm realizing that the dashboard doesn't need to be an FPS plugin. Maybe it should live in the jupyverse repository, since it will probably be specific to Jupyter.

@davidbrochart
Copy link
Contributor Author

This PR now only consists of adding a show_endpoints option that logs the HTTP and WebSocket entpoints.
The jupyverse dashboard will consume this information.

@davidbrochart
Copy link
Contributor Author

@adriendelsalle OK to merge?

@davidbrochart
Copy link
Contributor Author

Kindly pinging @adriendelsalle.

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

Successfully merging this pull request may close these issues.

3 participants