fps icon indicating copy to clipboard operation
fps copied to clipboard

Show API summary

Open davidbrochart opened this issue 3 years ago • 14 comments

See #55

davidbrochart avatar Dec 22 '21 12:12 davidbrochart

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 avatar Dec 22 '21 14:12 davidbrochart

Thanks @davidbrochart ! I'll have a look

adriendelsalle avatar Jan 04 '22 13:01 adriendelsalle

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

adriendelsalle avatar Jan 06 '22 11:01 adriendelsalle

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 :smile: On the other hand, I think we could make FPS a Rich/Textual application, instead of having a stream of log messages.

davidbrochart avatar Jan 06 '22 12:01 davidbrochart

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?

adriendelsalle avatar Jan 14 '22 09:01 adriendelsalle

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.

fcollonval avatar Jan 14 '22 09:01 fcollonval

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.

davidbrochart avatar Jan 14 '22 12:01 davidbrochart

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.

fcollonval avatar Jan 14 '22 12:01 fcollonval

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 avatar Jan 14 '22 12:01 davidbrochart

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 avatar Feb 15 '22 19:02 davidbrochart

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 avatar Feb 16 '22 00:02 davidbrochart

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 avatar Feb 16 '22 13:02 davidbrochart

@adriendelsalle OK to merge?

davidbrochart avatar Feb 16 '22 20:02 davidbrochart

Kindly pinging @adriendelsalle.

davidbrochart avatar Feb 21 '22 21:02 davidbrochart