Dedicated symbolication tool

[richard-fine]

Hello, Firefox team! I have been working with @vvuk on adopting Samply + the Firefox profiler tooling into our development processes at Unity. We thought you might be interested in this change that I put together over the past couple of days.

Problem

When creating a capture using a tool such as Samply, the trace is not initially symbolicated; symbolication is done on-demand, when loading the trace into the frontend.

The downside of this is the the trace file is not really portable - if you move it to another machine, you likely won't be able to symbolicate it, as the symbols might not exist on that other machine.

It's possible to work around this problem by launching the frontend immediately and triggering symbolication, but it's a bit awkward to do this, particularly in automation scenarios; it means launching and puppeteering an entire browser, and if Samply is being run on a machine where the default browser is, say, Safari, then it doesn't actually work correctly because of Safari's CORS behaviour with localhost.

Proposed solution

In order to avoid the need to launch an entire browser to get a profile symbolicated, this PR adds a small standalone NodeJS-based tool which can be run as a CLI process to symbolicate a profile. The components used by the frontend are reused so that the logic is shared, they're just packaged together into a minimal app with no React/Redux, etc.

Once built, the tool is at dist/symbolicator.js.

Usage

node dist/symbolicator.js \
    --input path/to/unsymbolicated/profile.json \
    --output path/to/write/symbolicated/profile.json \
    --server <URI of Mozilla Symbolication API endpoint such as the one exposed by Samply>

Future

Over time I'd like to try and integrate this logic directly into Samply (improving/replacing the --unstable-presymbolicate option that was recently added to it), but as someone who has only rudimentary Javascript and absolutely zero Rust, that'll take me a while :) so this tool might be a helpful step in the meantime.

I also looked into packaging the tool as an SEA, as I'd like to avoid having to carry around an entire Node install just to run this one script. However, there's a couple of supporting files that webpack produces (one of which is a WASM) and I'm not totally sure how to fix that up so it can be packaged correctly.

[julienw]

Thanks, this looks reasonable, what do you think @mstange ?

I've left a few comments to be fixed before landing but this looks OK to me otherwise. Of course you'll also need to fix the red CI, please ping if you need assistance on these.

It would also be good to add some simple tests exercising the code too.

[julienw]

Note that yarn lint-fix should be able to fix most linter error, just as yarn prettier-fix will fix most prettier-related issues.

[richard-fine]

Alright, that's everything in CI green apart from one issue with flow. Could I get some help with that? I have no idea why it finds the indicated code problematic.

For tests: I can have a try but I'm not sure about the best approach to take. I assume that the components I've stitched together already have unit tests. You want something like an e2e test?

[julienw]

For the flow error, I think this is because our (quite old) version of flow doesn't know about top level await, even though that's a thing for some time now. The fix would be to put all your top level code in an async function (maybe call it run) and call it at the end of the script.

For the tests, you can have a look at the existing related ones in src/test/components/TrackContextMenu.test.js. I'm looking for a specific test that would click on this option and check the result, maybe once for a global track and once for a local track. (Sorry, I got confused with another PR.)

As a test, if you do this run function as suggested above, maybe you can export it and run it from the test, that could be easier. I'm mostly looking for a non-regression test, because we won't probably use this file a lot ourselves, so making sure that we don't break it by mistake and it still works for you over time. You'll probably need to use the fetch mock (an example here, full documentation).
Keep it simple :-) If that's too much work I'll give it a try next week.

[mstange]

This is a great idea! I was hung up on thinking we needed to publish an npm module to unlock this use case. But just having a way to run symbolication from the command line if you have a clone of this repo around - that's so much simpler!

I think this would help our Firefox CI use case as well.

[richard-fine]

For the flow error, I think this is because our (quite old) version of flow doesn't know about top level await, even though that's a thing for some time now. The fix would be to put all your top level code in an async function (maybe call it run) and call it at the end of the script.

Are you sure? My (limited) understanding of NodeJS and promises is that putting the code in an async function and calling it would immediately return a Promise<void>, and Node will not wait for the promise to settle before exiting - it looks like I have to employ some ridiculous shenanigans with setInterval() to force Node keep pumping the event loop until the promise is actually settled. Perhaps updating flow to a newer version would be less painful?

As a test, if you do this run function as suggested above, maybe you can export it and run it from the test, that could be easier. I'm mostly looking for a non-regression test, because we won't probably use this file a lot ourselves, so making sure that we don't break it by mistake and it still works for you over time. You'll probably need to use the fetch mock (an example here, full documentation).

Alright, can do. I assume it's OK to have some test resources in the repo for this, i.e. non-symbolicated and symbolicated traces + a set of saved symbol-table responses for the mock to supply?

[julienw]

For the flow error, I think this is because our (quite old) version of flow doesn't know about top level await, even though that's a thing for some time now. The fix would be to put all your top level code in an async function (maybe call it run) and call it at the end of the script.

Are you sure? My (limited) understanding of NodeJS and promises is that putting the code in an async function and calling it would immediately return a Promise<void>, and Node will not wait for the promise to settle before exiting - it looks like I have to employ some ridiculous shenanigans with setInterval() to force Node keep pumping the event loop until the promise is actually settled. Perhaps updating flow to a newer version would be less painful?

I think Node will wait if we let it exit "naturally" (that is without calling process.exit). Please tell me if you see otherwise.

Updating flow is very painful for a project our size -- we want to move to typescript but haven't done that yet...

As a test, if you do this run function as suggested above, maybe you can export it and run it from the test, that could be easier. I'm mostly looking for a non-regression test, because we won't probably use this file a lot ourselves, so making sure that we don't break it by mistake and it still works for you over time. You'll probably need to use the fetch mock (an example here, full documentation).

Alright, can do. I assume it's OK to have some test resources in the repo for this, i.e. non-symbolicated and symbolicated traces + a set of saved symbol-table responses for the mock to supply?

Yes it's definitely OK, just try to make them not too big :-)

[codecov]

Codecov Report

Attention: Patch coverage is 77.50000% with 18 lines in your changes missing coverage. Please review.

Project coverage is 88.56%. Comparing base (5d53dcb) to head (a3441d2).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/symbolicator-cli/index.js	80.28%	14 Missing ⚠️
src/symbolicator-cli/webpack.config.js	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5123      +/-   ##
==========================================
+ Coverage   88.40%   88.56%   +0.16%     
==========================================
  Files         305      307       +2     
  Lines       27716    27794      +78     
  Branches     7515     7525      +10     
==========================================
+ Hits        24501    24616     +115     
+ Misses       2981     2961      -20     
+ Partials      234      217      -17     

Flag	Coverage Δ
	88.56% <77.50%> (+0.16%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[julienw]

Hi @richard-fine , this looks good to me, with the changes I added to the PR in Unity-Technologies#11

Once it's merged (but of course tell me what you think) I'll be able to merge to main!

[richard-fine]

@julienw I've merged your small modifications, thanks for those!

Don't merge this just yet though - I am not totally sure that the integration test is valid yet.

[richard-fine]

The expected value for my integration test was actually not symbolicated, and yet the test was passing. What I discovered was that the code I'd added to my custom SymbolStore implementation to dump out the symbol server response (to create symbol-server-response.json) was broken and causing an error to be thrown, but it looks like this error gets silently swallowed. SymbolStore.getSymbols defines an errorMap variable, and this map is populated with errors that come back from attempting to fetch symbols, but I don't see how the data in that map is surfaced to calling code. Am I missing something, or is that a bug?

Either way, I'm now confident that the test is actually working, so if you are happy enough with the code coverage, I think it should be OK to proceed to merge.

[julienw]

The expected value for my integration test was actually not symbolicated, and yet the test was passing. What I discovered was that the code I'd added to my custom SymbolStore implementation to dump out the symbol server response (to create symbol-server-response.json) was broken and causing an error to be thrown, but it looks like this error gets silently swallowed. SymbolStore.getSymbols defines an errorMap variable, and this map is populated with errors that come back from attempting to fetch symbols, but I don't see how the data in that map is surfaced to calling code. Am I missing something, or is that a bug?

It's surfaced in the errorCb callback, the last argument to getSymbols. Then it's handled in symbolicateProfile in

profiler/src/profile-logic/symbolication.js

Lines 891 to 899 in 5d53dcb

	(_request, error: Error) => {
	if (!(error instanceof SymbolsNotFoundError)) {
	// rethrow JavaScript programming error
	throw error;
	}
	// We could not find symbols for this library.
	console.warn(error);
	}
	);

. So if it's not a SymbolsNotFoundError, I think it should be errored properly, otherwise it's a bug. If it is a SymbolsNotFoundError then it's surfaced only as a console warning. Also it could have been a SymbolsNotFoundError for a wrong reason. Do you know which case this was?

I believe symbolicateProfile has never been thought as a proper API to be called from outside the project, rather the symbol store API has been. So there could be some room for improvement! But I wouldn't consider this a bug at the moment.

Either way, I'm now confident that the test is actually working, so if you are happy enough with the code coverage, I think it should be OK to proceed to merge.

[julienw]

Either way, I'm now confident that the test is actually working, so if you are happy enough with the code coverage, I think it should be OK to proceed to merge.

I can't change the PR so I can't make it uptodate. Can you please click the Update branch button ? :-)

[julienw]

Just merged, thanks for your contribution!