Open alangpierce opened 6 years ago
I just want to add a strong +1 to this.
We have a custom dashboard built in React which allows developers to take requests to servers and dive into the performance/etc of it. We have our own flamegraph tool currently (built upon https://github.com/bvaughn/react-flame-graph) which has the advantage of being deeply integrated into the dashboard, but which lacks all of the power and ease of use of Speedscope.
I'd love to be able to integrate it as a React component and just drop it in; it looks like potentially we could replicate what the views/
directory's code does to integrate it, although I'm not sure if that's a use case you're looking to support. I'd really like to avoid iframeing it in if at all possible (we'd want to integrate the toolbar UI into our app natively).
What would be great is what @alangpierce suggests, splitting the packages out. This would make it clearer where the API boundaries are and what cases you're looking to support. It looks like the separation in the codebase here is already pretty clean. Additionally, the CSP issue mentioned above seems to have been addressed in #140 as part of #137.
Hi @rmccue!
At a really high level, I'm not opposed to making speedscope available as a drop-in library, though I'd prefer to have the absolute minimum surface area exposed that would make this possible, which I think would be something like speedscope.mountInto(domNode)
and speedscope.removeFrom(domNode)
or something to that effect (you'd probably also want something like speedscope.loadFile(...)
. That way it could be integrated into applications not using Preact or React at all. I'm not interested in maintaining several different wrappers for different UI libraries present and future. I'm also not interested in providing explicit ways of theming speedscope, at least not at the moment.
Here are some barriers you'd hit trying to do this:
vw
and vh
units liberally, which would probably be annoying if you're building this to integrate into a larger application with co-existing views on the same screen.You're right though that the CSP issue should be resolved. No more eval!
Re: the approach of making a duplicate implementation of views/
, you're certainly welcome to do that by forking this repo or merging it into yours, but I don't intend to preserve a public API that exposes everything that's accessible in that directory. It's not just data APIs that are outside of views/
, it's also WebGL APIs, and making those publicly accessible is definitely not something I want to do, since I want to have the ability to refactor the WebGL APIs however I wish without breaking compatibility.
Thanks for the info @jlfwong, incredibly useful.
I wouldn't want this to increase your maintenance burden, and I think your approach of exposing a simple mount point makes sense: it keeps the surface area small as you say, but also hides all the complexity that Speedscope takes care of.
I'm not certain where your theoretical API's boundary is though; would the mountInto
be mounting the Speedscope application? (i.e. essentially just making the root node for mounting configurable) Or do you mean something a little deeper than that? (e.g. Application's renderContent
)
I'd be happy to take a pass at implementing this (although our current implementation works, so not a super high priority from our end).
Given the limitations and the limited API, it might also make sense for an iframe to be the API that's exposed here. I think that'd be fine for our use case, and perhaps all that needs then is documentation. (That said, I did hit an issue locally when trying to do this where Firefox refuses to load the iframe; it shows no errors in the page's console, but triggers errors in the browser's console, which seems to be some sort of bug. Maybe the trace is too big.)
Just as a bit of context: we have a list of all requests made to the server, and the raw trace data for those stored (in AWS X-Ray). Users can dive into any request and view the performance via a flamegraph, but there's additional data like database queries/etc available as well, so we have a tabbed interface. A loadFile
wouldn't be necessary for us, as we have the data loaded from an API already (although we could point Speedscope to another API endpoint that does it server-side).
For us, the trace data is already provided by the surrounding context, so apart from the view selector, the other toolbar items don't make a huge amount of sense for us. (I don't really care about theming, it's more that "import" doesn't make sense in that context; a disableImport
flag would work just as well.)
(There's a bunch of other intricacies that I won't go into, but I think those points are common to others looking to embed.)
I'm not certain where your theoretical API's boundary is though; would the mountInto be mounting the Speedscope application? (i.e. essentially just making the root node for mounting configurable) Or do you mean something a little deeper than that? (e.g. Application's renderContent)
To be honest, I haven't thought deeply about this, but the thought I had was to expose a function mountInto
that basically does this:
So, yes, I think that's basically just making the root node configurable.
A loadFile wouldn't be necessary for us, as we have the data loaded from an API already (although we could point Speedscope to another API endpoint that does it server-side).
Oh, sorry -- I should've been clearer here. The API I was proposing as probably necessary wasn't one to perform requests or anything, it was one to just accept a binary ball and load the profile. Something like this hack I have in place for making the CLI work:
So basically some stable API for calling the Application.loadProfile
.
So I was thinking the API would look something like this:
interface SpeedscopeInstance {
// Resolves after the profile has loaded and the DOM has updated
loadProfilesFromText(fileName: string, content: string): Promise<void>
loadProfilesFromArrayBuffer(fileName: string, contents: ArrayBuffer): Promise<void>
// Unmount the application from the DOM, remove any event listeners
unmount(): Promise<void>
}
namespace speedscope {
// Construct the speedscope application and put it into the default state where
// You can drop files onto it and have it load things
mountInto(element: HTMLElement): Promise<SpeedscopeInstance>
}
And then usage could look something like this:
const speedscopeInstance = await speedscope.mountInto(document.getElementById("wherever-you-want-to-mount"));
await speedscopeInstance.loadProfile("test.txt", "a;b\na")
console.log("Profile loaded!")
It's possible that the minimal API for this needs to be larger than this in order for it to be useful as a library -- I suspect to discover that you'd need to do the necessary changes to make the actual integration you're trying to make.
Given the limitations and the limited API, it might also make sense for an iframe to be the API that's exposed here.
If you're interested, since speedscope is all static files and has a zip file you can download and just open an HTML file from here: https://github.com/jlfwong/speedscope/releases, having an automated update process that downloads the newest copy for iframe embedding without needing to worry about cross-domain conflicts should be pretty easy.
That was similar to the approach I took when trying to get speedscope integrated as the default profile visualization tool in stackprof: https://github.com/tmm1/stackprof/pull/100, except that I use npm pack
there to avoid needing to manually download zipfiles.
If there are things preventing speedscope from being usable inside of an iframe if the iframe is hosted from the same domain, then I'm definitely open to PRs that fix that.
Greetings folks! Just wanted to mention that I also would love to have this feature, and I'm happy to help out if there are specific things I can do to push it along. My needs are pretty similar to others here:
npm
modulediv
contained within a broader applicationloadFromJson
or similar function that takes the trace data@rmccue Are you still planning on trying to take a pass at implementing this? If not, no worries -- just trying to get a pulse on who's doing what to prevent duplication of work.
I'm reluctant to do this myself because I don't have a clear idea of exactly what the minimum API needed to satisfy your real use cases is.
If you're interested, since speedscope is all static files and has a zip file you can download and just open an HTML file from here: https://github.com/jlfwong/speedscope/releases, having an automated update process that downloads the newest copy for iframe embedding without needing to worry about cross-domain conflicts should be pretty easy.
That was similar to the approach I took when trying to get speedscope integrated as the default profile visualization tool in stackprof: tmm1/stackprof#100, except that I use npm pack there to avoid needing to manually download zipfiles.
If there are things preventing speedscope from being usable inside of an iframe if the iframe is hosted from the same domain, then I'm definitely open to PRs that fix that.
To hang off this - assuming I download a .zip
from the releases page and vendor it with my project, embedding it on some page in an <iframe />
, how would I go about passing some profile data to it?
The easiest thing would be to have the URL set to somewhere it can download the data from: https://github.com/jlfwong/speedscope#importing-via-url
Much appreciated!
I'm making a chrome devtools extension to surface the results of server-side profiling, and I tried hacking speedscope in as a React component, but I ran into some issues. From my perspective, it would be great if I could install speedscope as a library and render it as a React component with the full text of the profile as a prop.
Here's a partial list of issues that would need to be addressed for this to happen:
npm link
the TypeScript into my node_modules and compile it with webpack, but that ran into various issues. If you want to avoid the React dependency (or peer dependency), you could ship a@speedscope/core
and separate@speedscope/react
package, or something like that.So at least for now, it looks like an iframe (or external window) is the nicest way to use speedscope, and from what I can see that works fine. But it would be cool if it could be more integrated into a larger tool.