Skip to content

Conversation

marksvc
Copy link
Collaborator

@marksvc marksvc commented Aug 21, 2025

This change is Reviewable

Copy link

codecov bot commented Aug 21, 2025

Codecov Report

❌ Patch coverage is 92.10526% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.26%. Comparing base (dd3c323) to head (c88531f).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/RealtimeServer/common/resource-monitor.ts 92.10% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3390      +/-   ##
==========================================
+ Coverage   82.16%   82.26%   +0.09%     
==========================================
  Files         608      608              
  Lines       36111    36127      +16     
  Branches     5896     5900       +4     
==========================================
+ Hits        29672    29719      +47     
+ Misses       5578     5547      -31     
  Partials      861      861              

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

@marksvc
Copy link
Collaborator Author

marksvc commented Aug 21, 2025

Previously the resource usage reports were being written to the application location. This was okay for some investigation, but they seem to get deleted from there, probably when we do a release. This PR changes the output path so they are stored in a more persistent location, like ~/.local/share/sf-resource-reports. It also allows overriding the location with environment variable SF_RESOURCE_REPORTS_PATH, which isn't immediately useful but might be helpful depending on how HOME resolves on Windows.

This PR may have more changes than expected for a change to output path. I wanted to use the async mkdir(), but didn't want to run it without await in the constructor (lest a recording possibly happen before the mkdir finished). I ended up putting the mkdir in / called from record() and making record() be async.

Since record() is now async, I adjusted some of the other methods being called to also be async.

Setting label "testing-not-required" since it's perhaps more complex than it's worth, and wouldn't really be testing something I haven't already tested on my own machine.

And that said, I am leave the PR as a draft for now because when it is merged I want to check its behaviour on QA (which will be a bit different than how it behaves on a workstation.)

@marksvc marksvc marked this pull request as ready for review August 21, 2025 19:46
Copy link
Collaborator Author

@marksvc marksvc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion


-- commits line 2 at r1:
This PR is ready to be reviewed, but I want to do the merge myself so I can test something on QA soon after.

Copy link
Collaborator

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve, though I don't really understand why the script cares about awaiting the creating of the directory, but not the awaiting of the writing the file. Both are operations that happen asynchronously, and it seems the same could be achieved by just putting the creating of the directory directly before the writing of the file.

Also, it's kind of odd to see:

import * as fs from 'fs';
import { mkdir } from 'fs/promises';

... and then call

 fs.promises.writeFile

It seems like if you want the promise version of fs functions, you could just import them all the same way you import mkdir.

@Nateowami reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @marksvc)

Copy link
Collaborator Author

@marksvc marksvc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @Nateowami . I modified resource-monitor.ts so it calls await mkdir inside of saveToCsv. That makes sense. And I stopped storing the base output path and instead learn it from the path in the saveToCsv method.

I also made a change such that if both XDG_DATA_HOME and HOME are not defined, then the output directory will be current working directory + sf-resource-reports, rather than current working directory + '.local/share/sf-resource-reports'. I moved logic about deciding on the output directory into a method. I added some tests about the determination of output directory.

I don't really understand why the script cares about awaiting the creating of the directory, but not the awaiting of the writing the file.

I was concerned about mkdir not finishing before the code was asked to save data because it would crash if it tried to save and the output directory was not yet created.

I suppose a caller could repeatedly request that reports be written, in which case it could be important to wait for writing a file.

What do you think about the changes?

could just import them all the same way you import mkdir.

Done.

BTW this PR can be tested by running SF with environment SF_SIGUSR2_ACTION=resourceUsage, then sending SIGUSR2 with

pkill --signal SIGUSR2 --full --  'node .* --port 5002'

Then looking for files:

find ~/.local/share -mmin -1 -ls

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @marksvc)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants