search

facebook / memlab

A framework for finding JavaScript memory leaks and analyzing heap snapshots
https://facebook.github.io/memlab/
MIT License
4.36k stars 119 forks source link

JSON output #128

Closed aelij closed 2 weeks ago

aelij commented 3 weeks ago

PR changes:

Open questions:

Fixes #127

facebook-github-bot commented 3 weeks ago

Hi @aelij!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

facebook-github-bot commented 3 weeks ago

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

JacksonGL commented 3 weeks ago

Thanks for the PR @aelij

I left a few comments on the PR: https://github.com/facebook/memlab/pull/128/commits/266315a7c4ee8ceb4523baf1c04c74be824280b8

When output is set to JSON, it implies the --sc option and directs all logs to stderr. This allows to easily capture a clean JSON output using memlab > result.json.

Redirecting all console output to stderr in JSON mode doesn't feel write, maybe we can redirect those output to /dev/null in JSON mode

Should all analyses support JSON output? I only added the ones I need at the moment.

Ideally yes, and it would be great to extend the base class of all analysis to allow the override of JSON output implementation. but I don't have a bandwidth to add support for all of them. So let's add it case by case for now.

The output from getJSONifyableObject has inconsistent casing (e.g. snake self_size vs. camel incomingEdgeCount). Is it a breaking change to change this so it's all the same? Which case is preferrred?

All snake case properties like self_size are inherited from the v8 heap snapshot, just to preserve the original property from heap snapshot. It is a breaking change to change to change all to camel case.

aelij commented 3 weeks ago

Redirecting all console output to stderr in JSON mode doesn't feel write, maybe we can redirect those output to /dev/null in JSON mode

This is a common practice in CLIs. stderr isn't only used for errors, it's used for any non-primary output. If we redirect to /dev/null there's no option to capture these logs, which may be essential for diagnostics if the program fails. As JSON output is a new option, this won't break existing users.

This is why I added the writeRaw method. The rest of the class deals with writing logs. Writing the output should have different handling. If I were to use topLevel both would be written to the same stream. Plus topLevel also does some string manipulations and uses the chalk class which could add unnecessary chars to the JSON. I can add writing to this.log.push() to writeRaw so the JSON will also be written to the file, although IMO the log file does not need to include it.

All snake case properties like self_size are inherited from the v8 heap snapshot, just to preserve the original property from heap snapshot. It is a breaking change to change to change all to camel case.

Then perhaps I should create a different object for JSON output instead of relying on getJSONifyableObject?

Thanks

JacksonGL commented 3 weeks ago

Thanks @aelij

This is a common practice in CLIs. stderr isn't only used for errors, it's used for any non-primary output.

TIL

If we redirect to /dev/null there's no option to capture these logs, which may be essential for diagnostics if the program fails. As JSON output is a new option, this won't break existing users.

MemLab additionally captures and writes a copy of all console output to a log file in $(memlab get-default-work-dir)/data/cur/. The new json output also needs to be included in the log. That's why I was suggesting using info.topLevel (but now I realize that topLevel output is also redirected to a different stream, so now I see why you are adding writeRaw)

If I were to use topLevel both would be written to the same stream. Plus topLevel also does some string manipulations and uses the chalk class which could add unnecessary chars to the JSON.

info.TopLevel just uses the input string without adding chalk styles: https://github.com/facebook/memlab/blob/88bce924d7dd1bd219632262c0d531c7946a0067/packages/core/src/lib/Console.ts#L111

I can add writing to this.log.push() to writeRaw so the JSON will also be written to the file

Yes, please add this to writeRaw. The writing log file is meant to capture all console outputs.

Could you also add additional muteConfig checks in writeRaw? Something like:

if (this.config.muteConfig?.muteRaw) {
  return;
}

Then perhaps I should create a different object for JSON output instead of relying on getJSONifyableObject?

I think it depends on what you want, if you want naming consistency, then create a different object for JSON output. If you want the JSON output to serialize and preserve the object properties, then use getJSONifyableObject. Perhaps we can also add a new CLI option for switching between these two modes.

aelij commented 2 weeks ago

Pushed an update. I changed writeRaw to writeOutput and added a muteOutput config.

facebook-github-bot commented 2 weeks ago

@JacksonGL has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot commented 2 weeks ago

@JacksonGL merged this pull request in facebook/memlab@d2ce8362b3f3755363fd905923123531ce578411.

aelij commented 2 weeks ago

Thanks, @JacksonGL!