ui: implement support for server extensions#4192
Conversation
primiano
left a comment
There was a problem hiding this comment.
I left some comments. overall LG with few minor comments here and there.
to me the biggest blocker part that needs to be figured out is the interaction between core and this.
my theory is that this cannot be a pure extension (sorry for the pun). the core needs to know "something" about dynamically loadable extensions.
What I don't like is the fragile dance, where this extension loads, and then injects stuff into core, and then some other code needs to carefully await that. it's very very brittle.
I don't have a fully fledged proposal on top off my head, but let's discuss more tomorrow (book some time, steve should be also around).
In my mind the core needs to know, at least in part, of server extensions.
At least part of that types.json imho should be in core.
I don't think core needs to know about how to fetch stuff (Which can be delegated to a plugin like you did).
But in my mind core needs to know at least that "some plugins can provide server extensions, and when you need stuff like sql modules or descriptors, the core needs to query them"
This is the problem I see:
- Some of the stuff that a server offers, in theory is push-only, e.g. macros which could be injected at any time.
-I say in "theory" because in reality we are better off ensuring that they are registered very early on. because somebody might end up triggering a macro early on (can they do progammatically) and being confused about the fact it's not there (unless we say macros for now can only be invoked via user action) - some other stuff instead is "pull": for instance sql modules. there is a precise point in time, i believe, when core initializes TP, where all the modules MUST have been gathered. same for proto descriptors (maybe?)
this is what needs to be defined.
If there is something that is a "pull at a specific point in time (e.g. while initializing TP)" core must know about that concept, and the dependency needs to be handled with something like "your plugin when initializes sets something into core like registerSqlModuleProvider(...)"
the only question I have is whether over time we'll end up promoting 80% of the extensions bits to the core, at which point we are better off promoting the whole extension thing right now?
this is the biggest open question i have: some part of this MUST be promoted to core: how much ?
ui/src/core_plugins/dev.perfetto.ExtensionServers/add_extension_server_modal.ts
Outdated
Show resolved
Hide resolved
ui/src/core_plugins/dev.perfetto.ExtensionServers/extension_server.ts
Outdated
Show resolved
Hide resolved
ui/src/core_plugins/dev.perfetto.ExtensionServers/extension_server.ts
Outdated
Show resolved
Hide resolved
ui/src/core_plugins/dev.perfetto.ExtensionServers/extension_server.ts
Outdated
Show resolved
Hide resolved
ui/src/core_plugins/dev.perfetto.ExtensionServers/extension_server.ts
Outdated
Show resolved
Hide resolved
This CL changes the CSP such that it allows all https: fetches. This has a very nice property of allowing url= to now work for any publicly accessible https trace which can be fetched via a GET call. Also update the documentation to match.
This refactors the legacy "is_internal_user" script to be an implementation of the first type of "server extension" (i.e. data which can be loaded from a server separate to the UI). This allows for a smoother implementation of RFC-0005 when we do proper server extensions.
f1f7487 to
54662a3
Compare
🎨 Perfetto UI Build✅ UI build is ready: https://storage.googleapis.com/perfetto-ci-artifacts/gh-20962825552-1-ui/ui/index.html |
54662a3 to
31e4eb9
Compare
|
PTAL. There is a very interesting design question left about how we correctly namespace these things remaining but the code itself should now be in a much much better state. |
c6ab4fe to
cf56c32
Compare
cf56c32 to
09d9600
Compare
ui/src/core_plugins/dev.perfetto.ExtensionServers/add_extension_server_modal.ts
Outdated
Show resolved
Hide resolved
ui/src/core_plugins/dev.perfetto.ExtensionServers/add_extension_server_modal.ts
Outdated
Show resolved
Hide resolved
ui/src/core_plugins/dev.perfetto.ExtensionServers/add_extension_server_modal.ts
Outdated
Show resolved
Hide resolved
ui/src/core_plugins/dev.perfetto.ExtensionServers/add_extension_server_modal.ts
Outdated
Show resolved
Hide resolved
ui/src/core_plugins/dev.perfetto.ExtensionServers/add_extension_server_modal.ts
Outdated
Show resolved
Hide resolved
ui/src/core_plugins/dev.perfetto.ExtensionServers/add_extension_server_modal.ts
Outdated
Show resolved
Hide resolved
ui/src/core_plugins/dev.perfetto.ExtensionServers/add_extension_server_modal.ts
Outdated
Show resolved
Hide resolved
ui/src/core_plugins/dev.perfetto.ExtensionServers/extension_server.ts
Outdated
Show resolved
Hide resolved
primiano
left a comment
There was a problem hiding this comment.
LGTM % the comment on the modal.
ui/src/core_plugins/dev.perfetto.ExtensionServers/extension_server.ts
Outdated
Show resolved
Hide resolved
|
Sorry as discusse offline ignore my last comment on section.ts. I tought you were doing that in Modal.ts which is special as it isn't caled on every render function. |
…dev/lalitm/setting
This is an initial implementaton of RFC-0005
(#3227).