server.js: Add optional <profile> cmd line arg by kazarmy · Pull Request #4452 · firefox-devtools/profiler

kazarmy · 2023-01-31T11:57:00Z

This pr adds a one-shot way via server.js to run Firefox Profiler with a local profile file. It can be used in tandem with a shell script like the following:

#!/bin/sh

PROFILEPATH=$(realpath "$1")
PWD=$(pwd)
cd ~/git/firefox-profiler; yarn start "$PROFILEPATH"; cd "$PWD"

I have seen and run samply and I think that:

There really is no need to run more than 1 server.
It appears easier to customize and prototype in a dynamic language like JavaScript rather than a compiled language like Rust. For example, changing the opened browser here is just a matter of modifying webpack.local-config.js.

Note that I have nothing against samply, Rust, or the people working on them. I am quite happy with putting all this in a private fork or unmaintained public fork. If this pr was accepted, it would be somewhat harder to switch to a dev server from another package e.g. Vite, though probably not impossible.

Future work for this would include:

Adding support for opening a profile in a running server. This probably would not require working HMR.
Support for sources appear to be straightforward.

If you have read this far, thanks for the attention!

codecov · 2023-01-31T12:03:59Z

Codecov Report

Patch coverage has no change and project coverage change: +0.03 🎉

Comparison is base (b178dad) 88.70% compared to head (fb1b549) 88.73%.

❗ Current head fb1b549 differs from pull request most recent head 59ed346. Consider uploading reports for the commit 59ed346 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4452      +/-   ##
==========================================
+ Coverage   88.70%   88.73%   +0.03%     
==========================================
  Files         285      284       -1     
  Lines       25682    25643      -39     
  Branches     6914     6898      -16     
==========================================
- Hits        22780    22755      -25     
+ Misses       2698     2684      -14     
  Partials      204      204

Impacted Files	Coverage Δ
src/components/app/SourceFetcher.js	`70.00% <0.00%> (-7.42%)`	⬇️
src/profile-logic/import/chrome.js	`94.60% <0.00%> (-0.21%)`	⬇️
src/profile-logic/tracks.js	`82.66% <0.00%> (-0.12%)`	⬇️
src/profile-logic/process-profile.js	`91.00% <0.00%> (-0.08%)`	⬇️
src/components/app/Home.js	`71.11% <0.00%> (ø)`
src/test/fixtures/profiles/tracks.js	`94.59% <0.00%> (ø)`
src/test/fixtures/profiles/processed-profile.js	`94.57% <0.00%> (ø)`
src/utils/query-api.js
src/app-logic/url-handling.js	`87.42% <0.00%> (+0.04%)`	⬆️
src/profile-logic/processed-profile-versioning.js	`85.50% <0.00%> (+0.13%)`	⬆️
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

mstange · 2023-01-31T12:49:56Z

I see, so the use case here is:

You have a Chrome / node / perf script profile file, i.e. a type of profile which doesn't require symbolication.
And you want to load this profile into a locally-running development build of the profiler, because you are working on the profiler itself.

That seems like a fine use case to make easier.

FWIW, samply load is much more complicated because it supports symbolication for C++ / Rust code, and can be used to open unsymbolicated profiles from samply but also from Firefox, such as profiles generated by MOZ_PROFILER_SHUTDOWN or by manual calls to Services.profiler.dumpProfileToFileAsync. But if that functionality is not needed and samply is not already installed on the system anyway, then I agree that compiling samply just for this is quite overkill.

kazarmy · 2023-01-31T12:56:48Z

That doesn't mean that samply can't be modified to support unsymbolicated profiles, but I did get the impression that samply was mainly an internal tool. I don't really expect a profiler user to have much trouble compiling samply though, even with the out-of-date rustc in Ubuntu 22.04.

kazarmy · 2023-01-31T12:59:24Z

That doesn't mean that samply can't be modified to provide source code for unsymbolicated profiles ... sorry about the confusion

kazarmy · 2023-01-31T13:09:33Z

... because you are working on the profiler itself.

For now at least, and because a development build is good enough for me right now. This pr might or might not be abuse of a dev server.

mstange · 2023-01-31T13:20:07Z

Just to clarify, what I meant to say is that I approve of the approach in this PR. (But I'll leave the code review to Julien or Nazim.)

As for making the source view work for JS profiles in a secure way, I'm curious about your solution, and we can discuss it later in a separate PR about it.

kazarmy · 2023-01-31T13:39:55Z

Just to clarify, what I meant to say is that I approve of the approach in this PR.

Thanks! Admittedly, it was hard to determine whether you were being sarcastic or not 😅

As for making the source view work for JS profiles in a secure way, I'm curious about your solution

It probably will just involve the user ensuring that the server is on 127.0.0.1 and not 0.0.0.0 or a public IP address.

…nto server.js-profile-arg

julienw

Thanks for the patch!

I'm not a big fan of the approach but if that makes your life easier I'm not against it.

It would be good to update the contributing doc, for example part 2 in this paragraph could be the right location.

Looking at your initial description I'm thinking that because you're using a shell script already, maybe you could expose your local directory directly using either python's http.server or the local-web-server node module (that we already have installed in the profiler project as node_modules/.bin/ws) in background, then start our server in background too, and finally open the firefox window using the URL from this server.

But maybe another vision is that it's what our server.js should do for an easier development environment, and that's why, again, I'm not against this PR. So happy to move it forward!

julienw · 2023-02-09T15:44:18Z

    y18n "^5.0.5"
    yargs-parser "^21.0.0"

+yargs@^17.6.2:


By running npx yarn-deduplicate this merges the too yargs 17.x entries, so this reduces the amount of new packages.

Okay 6723ea8

julienw · 2023-02-09T16:06:01Z

 );

+const argv = yargs(hideBin(process.argv))
+  .command('* [<profile>]', 'Open Firefox Profiler, on <profile> if included.')


I believe we don't need the * because this is useful to run the command by default -- but that's not the case here. (although the doc is misleading so I'll let you double check my affirmation).

Also, specifying both [] and <> which both have some specific behavior is recipe for future disaster when they'll change their internal iomplementations, so I'd suggest to use just [] which seems to be the one we want really here. Tell me if I'm wrong here too :-)

Suggested change

.command('* [<profile>]', 'Open Firefox Profiler, on <profile> if included.')

.command('[profile]', 'Open Firefox Profiler, on [profile] if included.')

On yargs 17.6.2, omitting the * doesn't work -- without it, yargs refuses to process a profile arg that is given on the command line. I did think about using open instead of the default command, but I don't see any other function for server.js other than to (eventually) open profiles.

Sorry about the <>, initially the profile arg was required, and I just slapped on the [] as it seemed the natural thing to do, without looking at the documentation. Thanks for the catch!

julienw · 2023-02-09T16:43:31Z

+      express.static(path.resolve(path.dirname(argv.profile)))
+    );
+    return middlewares;
+  };


I'm not a big fan of this solution, because 1/ this exposes a full directory (even though you're doing some filtering), 2/ this plugs into the dev server which might not work anymore in the future, 3/ this moves away from my longterm wish that we get rid of server.js at all in favor of using webpack serve directly (not sure if that will be possible though).

What about creating a separate server using node's http.createServer that would serve just this profile data?
This could look something like this:

if (argv.profile) { // Spin up a simple http server serving our file. const profilePort = 4241; const profileHostname = "127.0.0.1"; const server = http.createServer((req, res) => { const fileStream = fs.createReadStream(argv.profile); fileStream.pipe(res); }); server.listen(profilePort, profileHostname); const profileFromUrl = `${profilerUrl}/from-url/${ encodeURIComponent(`http://${profileHostname}:${profilePort}/`) }`; // Close the server on CTRL-C process.on("SIGINT", () => server.close()); process.on("SIGTERM", () => server.close()); // Keep the rest of your code .... }

Then in the future we can easily extract this to another file and use it elsewhere.

What do you think?

I think that any reduction in dependency on the (possibly capricious) internal behavior of webpack-dev-server is a good thing, and I've integrated the code above (279453c). Changes include:

The port for the profile server is OS-allocated instead of fixed.

The host for the profile server is the same as for the webpack server.

CORS handling.

The open package is now an explicit dependency.

For consistency, the options in serverConfig.open are honored. Theoretically, the port for the webpack server can be OS-allocated as well but that should be in a different PR since it might be complicated.

Co-authored-by: Julien Wajsberg <felash@gmail.com>

kazarmy · 2023-02-12T06:57:39Z

It would be good to update the contributing doc, for example part 2 in this paragraph could be the right location.

I have added the shell script I'm using to bin/ (after modifying it to use relative dirs) and updated both CONTRIBUTING.MD and loading-in-profiles.md (5d0a1cc c0c40bd).

…nto server.js-profile-arg

kazarmy · 2023-02-12T12:14:38Z

Okay I think that's it.

…nto server.js-profile-arg

julienw · 2023-02-17T17:59:39Z

Hey! Just a quick note that we've seen your update but just didn't have the chance to look at it during this week. It's still on our radar, thanks for your patience!

kazarmy · 2023-02-18T00:18:41Z

No worries :) I see that you have a lot on your plate.

julienw

Thanks for your patience!

I'm still not 100% convinced to have this as part of the server.js file. Indeed this would work only when the server isn't running yet; if it's already running, this would just fail. I feel like that a standalone nodejs script that would:

spawn a local server like you did already
open the profiler with a from-url URL properly filled in

would be more interesting.

This would have the following features:

support --release and --dev to decide whether to open profiler.firefox.com or localhost:4242
in the case of --release, we need a https server. So maybe a first implementation wouldn't support this case.
In the case of --dev, it would be good to check if the server runs already; if it doesn't run then in that case launch yarn start in the background first.

that said I'm fine with landing your patch as it is with the small comments fixed :-)

julienw · 2023-02-23T13:49:18Z

@@ -0,0 +1,16 @@
+#!/bin/sh
+# This script is primarily for running Firefox Profiler on a saved profile.


Can you please add a few examples about how to use it in the comments here?

Also what do you think about naming this file launch-fp.sh so that it's more obvious about what it does?

Can you please add a few examples about how to use it in the comments here?

Ok 974ad6d

Also what do you think about naming this file launch-fp.sh so that it's more obvious about what it does?

No strong opinions on this and I'm symlinking to it anyway so ok 974ad6d

julienw · 2023-03-03T16:15:34Z

 > `https://profiler.firefox.com/from-file/`

-When you're on [the home page](https://profiler.firefox.com) files can be loaded by either dragging over the profiler.firefox.com client, or using the file upload input.
+When you're on [the home page](https://profiler.firefox.com) files can be loaded by either dragging over the profiler.firefox.com client, or using the file upload input. On Linux, the provided [fp.sh](../bin/fp.sh) script can load files.


I feel like this addition should be in another paragraph though. Indeed we're really talking about the from-file datasource here.

What about moving this doc in the URL part above, below the part Here is an example profile server written in Node.js::

Note that if you have a copy of the project locally, you can simply add the file path as a parameter to yarn start and a server serving this file will be spawned for you. On Linux, the provided launch-fp.sh script can do this for you as well, from any working directory.

Ok lgtm with some minor editing (file -> profile, and removed 'simply' because alex complained) fe0bbab fb1b549

Indeed we're really talking about the from-file datasource here.

The document looked like it was for users that are also developers rather than Firefox Profiler developers specifically.

…nto server.js-profile-arg

kazarmy · 2023-03-04T09:27:26Z

Thanks for your patience!

Not sure whether I should have been pestering you lol.

Indeed this would work only when the server isn't running yet; if it's already running, this would just fail.

In the case of --dev, it would be good to check if the server runs already; if it doesn't run then in that case launch yarn start in the background first.

The plan is to put down somewhere a port-specific flag file when the server starts and then to check that file on script start to see whether a server is running. Since the file might not be cleaned up if the server crashes, the user should be informed whenever a relevant flag file is detected.

I feel like that a standalone nodejs script ...

This might cause code duplication but it apparently makes it easier to switch dev servers if needed so sgtm.

julienw

Thx!

kazarmy · 2023-03-16T12:41:04Z

I feel like that a standalone nodejs script that would:

spawn a local server like you did already

open the profiler with a from-url URL properly filled in

would be more interesting.

It seems a bit tricky to pull out the following code:

profiler/server.js

Lines 94 to 108 in 69ed07f

    
           // Delete "open" target (if any) in serverConfig. 
        
           if ( 
        
             typeof serverConfig.open === 'object' && 
        
             !Array.isArray(serverConfig.open) && 
        
             serverConfig.open !== null 
        
           ) { 
        
             delete serverConfig.open.target; 
        
           } else { 
        
             delete serverConfig.open; 
        
           } 
        
           // Save and delete "open" property from serverConfig so that 
        
           // webpack-dev-server doesn't open anything in tandem. 
        
           const openOptions = serverConfig.open; 
        
           delete serverConfig.open;

into that standalone nodejs script since it might require modifying the local webpack.local-config.js file somehow, but I'll probably figure out something by this week.

kazarmy · 2023-03-20T12:37:52Z

"by this week" unfortunately has become "delayed indefinitely" but not losing hope that the above will eventually be resolved

server.js: Add optional <profile> cmd line arg

4c3150e

kazarmy commented Jan 31, 2023

View reviewed changes

Comment thread server.js Outdated

kazarmy added 4 commits February 4, 2023 12:26

Merge branch 'main' of https://github.com/firefox-devtools/profiler i…

297ca04

…nto server.js-profile-arg

Rename profileUrl to profilerUrl

082befc

Refactor out the profile dir

37a1ead

Filter out /profiles/ URLs that are not the profile file

cf97a59

julienw reviewed Feb 9, 2023

View reviewed changes

kazarmy marked this pull request as draft February 10, 2023 10:21

Remove angle brackets around arg name

f44d606

Co-authored-by: Julien Wajsberg <felash@gmail.com>

kazarmy force-pushed the server.js-profile-arg branch from b8d494d to f44d606 Compare February 10, 2023 13:20

Spin up node:http server instead for profile

279453c

kazarmy force-pushed the server.js-profile-arg branch from 3b0f0c9 to 279453c Compare February 11, 2023 10:20

kazarmy added 2 commits February 12, 2023 14:32

Add fp.sh script and add to docs

5d0a1cc

Fix Shellcheck errors

c0c40bd

kazarmy changed the title ~~server.js: Add optional <profile> cmd line arg~~ server.js: Add optional [profile] cmd line arg Feb 12, 2023

kazarmy changed the title ~~server.js: Add optional [profile] cmd line arg~~ server.js: Add optional <profile> cmd line arg Feb 12, 2023

kazarmy added 3 commits February 12, 2023 19:54

Merge branch 'main' of https://github.com/firefox-devtools/profiler i…

098dacc

…nto server.js-profile-arg

Run npx yarn-deduplicate

6723ea8

Run yarn install

c1ee8cf

kazarmy marked this pull request as ready for review February 12, 2023 12:14

Merge branch 'main' of https://github.com/firefox-devtools/profiler i…

a534862

…nto server.js-profile-arg

julienw reviewed Mar 3, 2023

View reviewed changes

kazarmy added 5 commits March 4, 2023 15:59

Rename fp.sh to launch-fp.sh

1722a14

Add launch-fp.sh examples

974ad6d

Add note to from-url datasource section instead

fe0bbab

Merge branch 'main' of https://github.com/firefox-devtools/profiler i…

c43077e

…nto server.js-profile-arg

Remove simply word from loading-in-profiles.md

fb1b549

julienw self-requested a review March 6, 2023 10:56

julienw approved these changes Mar 9, 2023

View reviewed changes

Merge branch 'main' into server.js-profile-arg

59ed346

julienw enabled auto-merge (squash) March 9, 2023 14:36

julienw merged commit 085a26a into firefox-devtools:main Mar 9, 2023

kazarmy mentioned this pull request Mar 10, 2023

server.js: Enable webpack filesystem caching #4523

Merged

julienw mentioned this pull request Mar 27, 2023

When launching a file automatically, provides the file name to the UR… #4550

Merged

kazarmy mentioned this pull request May 11, 2023

server.js: Add --config flag for different local webpack config files #4613

Merged

kazarmy mentioned this pull request Jun 4, 2023

server.js: Refactor profile server code into own file #4656

Merged

	.command('* [<profile>]', 'Open Firefox Profiler, on <profile> if included.')
	.command('[profile]', 'Open Firefox Profiler, on [profile] if included.')

		@@ -0,0 +1,16 @@
		#!/bin/sh
		# This script is primarily for running Firefox Profiler on a saved profile.

Conversation

kazarmy commented Jan 31, 2023

Uh oh!

codecov Bot commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mstange commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kazarmy commented Jan 31, 2023

Uh oh!

kazarmy commented Jan 31, 2023

Uh oh!

kazarmy commented Jan 31, 2023

Uh oh!

mstange commented Jan 31, 2023

Uh oh!

kazarmy commented Jan 31, 2023

Uh oh!

Uh oh!

julienw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kazarmy Feb 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kazarmy commented Feb 12, 2023

Uh oh!

kazarmy commented Feb 12, 2023

Uh oh!

julienw commented Feb 17, 2023

Uh oh!

kazarmy commented Feb 18, 2023

Uh oh!

julienw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kazarmy commented Mar 4, 2023

Uh oh!

julienw left a comment

Choose a reason for hiding this comment

Uh oh!

kazarmy commented Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kazarmy commented Mar 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Jan 31, 2023 •

edited

Loading

mstange commented Jan 31, 2023 •

edited

Loading

kazarmy Feb 11, 2023 •

edited

Loading

kazarmy commented Mar 16, 2023 •

edited

Loading