Page MenuHomePhabricator

[keyserver] Add `$KEYSERVER_PROFILING_ENABLED` to toggle `node:cluster`
ClosedPublic

Authored by atul on Sep 27 2023, 9:18 AM.
Tags
None
Referenced Files
F3398848: D9301.diff
Mon, Dec 2, 12:44 AM
Unknown Object (File)
Fri, Nov 29, 12:21 PM
Unknown Object (File)
Tue, Nov 19, 4:56 PM
Unknown Object (File)
Thu, Nov 14, 9:26 AM
Unknown Object (File)
Thu, Nov 14, 9:26 AM
Unknown Object (File)
Thu, Nov 14, 9:26 AM
Unknown Object (File)
Thu, Nov 14, 9:25 AM
Unknown Object (File)
Thu, Nov 14, 9:17 AM
Subscribers
None

Details

Summary

The 0x tool mentioned in ENG-5078 does not work with node:cluster. We can modify keyserver.js to launch a single process when KEYSERVER_PROFILING_ENABLED is set. This is what I did in my personal dev environment to get 0x working.

Basically if KEYSERVER_PROFILING_ENABLED is set, we avoid cluster.fork()ing and just "spill" into the else block that would usually only be executed by child processes (cpid == 0). Previous cluster fork()ing logic continues to work as expected when KEYSERVER_PROFILING_ENABLED is unset.

Will users have to set KEYSERVER_PROFILING_ENABLED in their environment manually?

The yarn command for profiling will begin with KEYSERVER=true KEYSERVER_PROFILING_ENABLED=true node... to make this config "invisible" to the dev. As long as they run yarn profile-* things will work as expected.


Depends on D9300

Test Plan
  1. Added logging to ensure that conditionals behaved as expected when KEYSERVER_PROFILING_ENABLED is and isn't set.
  2. Successfully ran profiling session with KEYSERVER=true 0x -o -- node --trace-warnings --loader=./loader.mjs dist/keyserver

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul published this revision for review.Sep 27 2023, 9:19 AM
atul edited the summary of this revision. (Show Details)
atul edited the summary of this revision. (Show Details)
atul edited the summary of this revision. (Show Details)
ashoat added inline comments.
keyserver/src/keyserver.js
65 ↗(On Diff #31448)

Is changing this conditional necessary? Is cluster.isMaster not true in the single-process case?

This revision is now accepted and ready to land.Sep 27 2023, 9:39 AM
keyserver/src/keyserver.js
65 ↗(On Diff #31448)

It isn't necessary, I thought including it might make things a bit clearer but I'm not sure that's true. I'll remove it

remove extraneous condition

This revision was landed with ongoing or failed builds.Sep 27 2023, 10:04 AM
This revision was automatically updated to reflect the committed changes.