Approval to move node-heapdump into foundation

[mhdawson]

Requesting approval to move github.com/bnoordhuis/node-heapdump to github.com/nodejs/node-heapdump

It been discussed here by the post-mortem group in nodejs/post-mortem#40. Current proposal is to bring it in and then PR to accommodate changes/suggestions if they make sense (for example changing to use inspector protocol if that works)

There are 3 key artifacts for post-mortem diagnosis including:

• heapdumps
• first capture data (node-report)
• core dumps (llnode work is to allow working with those).

Bringing in node-heapdump would make sure we have heapdumps covered. It seems widely used.

On approval we'd add in the Node.js governance and then transfer over.

[jasnell]

+1 .... great to see this moving forward.

[nebrius]

Forgive my ignorance, I haven't bumped into this project before, so my questions may have obvious answers. Is node-heapdump currently being used in the Node project itself as a dependency, or is it currently a standalone project? Along those lines, if we brought this into the foundation, would the long term goal be to include it in Node?

[jasnell]

@nodejs/tsc ... please weigh in on this. I'd like to see if we can get consensus on this without calling for a formal vote.

@nodejs/post-mortem ... please see @nebrius' question above.

[cjihrig]

I don't see a problem with bringing it into the foundation, but I'm curious why we need to.

[misterdjules]

@nebrius

Is node-heapdump currently being used in the Node project itself as a dependency, or is it currently a standalone project?

It is a standalone project.

Along those lines, if we brought this into the foundation, would the long term goal be to include it in Node?

I don't know. It's been pointed out by @bnoordhuis that V8 may soon expose an API that more or less provides the same facilities as node-heapdump. If/when that's the case, it's possible that it wouldn't necessarily make sense to bring node-heapdump into node's core, because the functionality would already be there.

Not considering the possiblity of that V8 API, there have been discussions around integrating some of nodejs/node-report's functionality in core, including e.g generating heap dumps on out of memory errors, but so far I've been opposed to a lot of what has been suggested. I would think that if node-heapdump was integrated into core, then it could be used to generate these dumps. See nodejs/post-mortem#44 for more context.

Disclaimer: I'm not too familiar with node-heapdump, or with the intentions of the people who advocate for integrating node-report in core, so please take my comments with a grain of salt. I'm just trying to provide more context.

[mikeal]

Am I reading the license correctly, that two files are Copyright Strongloop and the rest is Ben, or is it the other way around? https://github.com/bnoordhuis/node-heapdump/blob/master/LICENSE

[bnoordhuis]

The V8 API shim is copyright StrongLoop (but guess who its author is), everything else is copyright me.

It's been pointed out by @bnoordhuis that V8 may soon expose an API that more or less provides the same facilities as node-heapdump.

Yes, with the caveat that the inspector probably isn't a solution for large heap snapshots, they make the process OOM.

I'll let others make the case why the module should be moved to the org. I personally don't have a strong opinion but I'm okay with handing it over.

[sam-github]

I don't have opinion yet on whether it should be moved to the org, but I'm pretty confident that if there were any license concerns (shouldn't be, its all MIT), that IBM would do whatever it takes to smooth them over (IBM owns all StrongLoop copyrights now).

I think the pro for moving this in is that getting heapdumps from node is a critical step in diagnosing memory leaks. @bnoordhuis 's module does this perfectly, and exists, and he maintains it, so there isn't anything preventing access to the module.

However... having such a critical diagnostic feature seem to come from the node foundation and be obviously supported by the diagnostic WG would perhaps help the community understand that node DOES support and value diagnostics, and that trouble-shooting tools are available when memory leaks occur. It also might spread the support burden, though I don't think its much of a burden for Ben.

I look forward to V8 having heapdump as a feature, perhaps even available via the inspector protocol, but I don't think its that relevant to this conversation. node 4.x thru 7.x are not likely to get that feature if its tied to a V8 update, so we'll still need node-heapdump for quite some time. If V8 does grow heapdump features, though, it would make a good case for bulding node-heapdump into core to implement whatever the V8 API ends up being. EDIT: that wasn't clear enough - I mean build node-heapdump into core for the LTS versions that won't get a heapdump API tied to the latest V8, so diagnostic capabilities are uniformly available across LTS releases and Current.

@ofrobots is there any specific links to a WIP or proposal for heapdump APIs from V8? Either C++, js, or inspector APIs?

[mhdawson]

My rational is that:

• heapdumps
• first capture data (node-report)
• core dumps (llnode work is to allow working with those).

Are three key elements that Node.js need to have covered for post-mortem diagnosis. If there was active innovation in external modules to cover this in the ecosystem then leaving it to that would make sense. But I don't think that's the case, therefore, we should be supporting work on them in the foundation repos, and even look at if there are benefits from a tighter integration with core.

In terms of the license, I'd not noticed the StrongLoop copyright, but if we get the ok to move forward I'll figure out what we need to do on that front.

[ofrobots]

@sam-github the PR #12263 brings in JS bindings for the inspector which has support for heap dumps.

…

On Tue, Apr 25, 2017 at 2:25 PM Michael Dawson ***@***.***> wrote: My rational is that: — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#257 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAE0qT52qQlBM9MCEgyR6klvwZri5MECks5rzmTigaJpZM4NEl52> .

[joshgav]

@mhdawson your rationale for bringing this in makes sense, but if we think the future of heap dumps will be via Inspector I'm concerned that we're mainly putting ourselves on the hook for long-term maintenance of a legacy project, which may not be a good use of our limited resources.

For comparison, we could make a decent argument for bringing node-inspector into the Foundation, but Inspector has now obsoleted it.

From a similar perspective, ChakraCore is working on supporting the Inspector interfaces but will likely never support the legacy heapdump APIs in V8 (right @digitalinfinity?).

[mhdawson]

The thought was that if inspector is the way to get heap dumps going forward then we'd update the module to do it in that way. I know that @bnoordhuis had some concerns with respect to whether heap dumps via the inspector would work as well as what is in the heapdump module in OOM type scenarios. My main goal is to get to the point were we have a standard way for generating heapdumps that we can point people to, with the flexibility to change the implementation behind the scenes.

[Fishrock123]

Not sure what my opinion is but it seems the discussion is settling on this may not be necessary?

[mhdawson]

@Fishrock123 its more that it is stalled since I've not had the time/energy to pursue. I still think its important we have some built in way to easily get heapdumps.

[jasnell]

Ping. Status on this?

[MylesBorins]

+1 to this... anything blocking this from moving forward? I think we can drop tsc-review

[mhdawson]

@MylesBorins there were some suggestions that we should do it another way and I've not had the cycles to push the conversation to conclusion. If it does not happen before then I'm hoping we can come to agreement in the upcoming Diagnostics summit.

[jasnell]

Is there a next step here or can this be closed?

[mhdawson]

@nodejs/tsc any TSC objections to this moving forward ? If not I'll ask Ben to start the processes of adding the required governance docs so we can move over.

[cjihrig]

It's still not clear to me why we need the heapdump module under the foundation when the inspector module in core can do the same thing? Is it OOM scenarios? If so, how are the two different? I'm not trying to block it, I just want to understand.

[Trott]

@nodejs/tsc any TSC objections to this moving forward ? If not I'll ask Ben to start the processes of adding the required governance docs so we can move over.

According to our current policy, this cannot happen without a vote by both the TSC and CommComm:

No repository may be deleted, transferred into, or transferred out of the Node.js Foundation GitHub Organization without a simple majority of both the TSC and CommComm in favor of the action.

Honestly, you might be best served by opening a PR to change the policy rather than following the policy. I think a reasonable policy would be to open an issue in the admin repo, ping TSC and CommComm, and wait 72 hours for objections.

[mcollina]

SGTM

[joyeecheung]

@Trott PR opened in #495

[fhinkel]

Another, please vote whether this tooling can move to the foundation. Ping @nodejs/tsc and @nodejs/community-committee. Please vote whether you're in favor of moving node-heapdump to the foundation, see #492 (comment).
Please use thumbs up/down on the first post.

[Trott]

@mhdawson Should this be closed? If not, would you be opposed to closing it here and open an issue in the admin repo instead?

[mhdawson]

I'm ok with closing it. I've had people tell me we should be using the inspector protocol instead, but I still need to understand if that is practical and if it has disadvantages over node-heapdump.

I'll close this one and then open a new one in admin if it turns out we still want to pull it in.