I reflected a bit about the API paths we are locking into providing going onwards, and if we should adjust them or not in some way.

Specifically, reflection I reflected about:

• Multiple handlers register to handle certain routes now, for example servers-info could be handled by ServersInfoHandler and ListServersAPIHandler, and icon/<existing-server-process> could be a ServersAPIHandler if its named quirky.
  I think this can be fine, but I'm not fully confident. Should we work to ensure there isn't overlapping here?

I've not gained much experience on REST API design, I've mostly understood there can be quite a bit of things to consider. @bollwyvl @manics do you think we should go for the proposed API endpoints, or do you have suggestions on alternative endpoints?

Welp, declaring (and testing against) an OpenAPI spec would be a good place to start, so that one doesn't have to read the code to figure out what's going on. Prior art:

• https://github.com/jupyter-server/jupyter_server/blob/main/jupyter_server/services/api/api.yaml
• https://github.com/jupyterlab/jupyterlab_server/blob/main/jupyterlab_server/rest-api.yml

This can generate nice static docs, as well as help cleaning up some of the design decisions: if you can't express a pattern as a URL param or in a schema, maybe it's not a great idea.

It also then becomes possible to re-use that schema declaration to generate lightweight python and TS types, even if one doesn't do fully generated clients.

Agree with OpenAPI suggestion. That would make it more cleaner. Actually, servers-info endpoint is only handling "launcher" specific metadata whereas the new endpoints are handling server specific metadata. So, I think it is logical to keep them as separate. However, I think we need to change the name of servers-info endpoint as it is bit misleading? ServersAPIHandler and ListServersAPIHandler can be merged into one using query parameters.

@bollwyvl Cheers for the refs

This could be reasonable, but I'm not confident on it. Is it acceptable that we start this up automatically as soon as this file is loaded?

I'm not confident on the alternate options, but I figure this is a singleton instance of the class defined above, created when this python file is loaded. I figure options involve delaying this initialization.

I'm also thinking a bit about if monitor_server_proxy_procs should or shouldn't be part of the class itself. It is a function tied to a specific object of the class anyhow via the manager variable that could be self as well.

If not someone has a concrete change idea here to persue or generally considers this to be blocking, I don't think this comment should be blocking a merge.

Right. Importing this file shouldn't have side-effects like starting/changing processes or even things on the event loop, as there's no telling it will actually be there.

Ideally, as much business logic/exception handling as possible would move into the manager class, and out of the handlers and __init__.py. The server extension would then become a very small layer that just hands down the parent application into the manager.

Also, I'd recommend new-start async stuff to use stdlib asyncio or anyio primitives instead of tornado-specific ones. And indeed, if we could get out of the polling business, that would be ideal: the underlying processes probably do (or should) provide callbacks/events that can be used to directly update the manager state.

Assuming we do have to poll, for some reason, and as I don't actually see a drop-in for PeriodicCallback, this could be a ServerProxyManager.monitor() which fires off an infinite loop:

import traitlets
import asyncio 

class ServerProxyManager(ServerProxy):
    monitor_interval = traitlets.Int(10, help="Proxy polling interval in seconds").tag(config=True)
    monitoring = traitlets.Bool(False, help="Whether the manager should poll for proxy status")

    # this doesn't need to be a trait
    _monitor_task: asyncio.Task

    @traitlets.observe("monitoring")
    def on_monitoring_changed(self, change: traitlets.Bunch):
        if self._monitor_task:
            self._monitoring_task.cancel()
        if change.new:
            # keep a handle so it doesn't get GCd
            self._monitoring_task = asyncio.create_task(self._monitor_loop()) 

    async def _monitor_loop(self):
        while self.monitoring:
             # do the thing
             await asyncio.sleep(self.monitor_interval)

When this gets invoked from __init__.py:

def _load_jupyter_server_extension(nbapp: ServerApp):
    manager = ServerProxyManager(parent=nbapp)
    manager.monitor()

If a user gracefully terminates a server proxy manually outside of lab interface, that process will not be restarted as we are not using always_restart feature of simpervisor. So somehow we need to update manager to remove this server proxy app and that is why I included this polling. Simpervisor uses exit handlers internally but I think they are not configurable. So, I dont know how we can fire a callback when process exits gracefully.