inferno icon indicating copy to clipboard operation
inferno copied to clipboard

Adopt changes from upstream pull requests

Open jonhoo opened this issue 5 years ago • 11 comments

The upstream FlameGraph project has a number of outstanding pull requests that fix bugs and add or improve features. We should take a look through them and see whether we may be able to incorporate some of them here! I propose we take the following approach for each one:

  • First, see whether the change is still relevant in inferno
  • Then, comment on the issue asking the original author whether they mind having their change adopted into inferno (we can't just take without permission -- copyright and licensing is no joke!)
  • If they agree, create a tracking issue on inferno describing the original PR, link to it + the author's agreement to port, and add the adoption label. Then, either starting writing a PR if you want to implement it yourself, or just leave it there for others to adopt!
  • If they don't respond, and you believe it to be an important feature, feel free to create a corresponding issue on inferno and write a PR, but note explicitly in both that you don't have the author's approval to port the change. We won't merge those until we do, but we can at least do the work in advance!
  • If they explicitly say they're unwilling to allow the change to be made in inferno, well, then we're our of luck. The exception would be if you can implement the feature without consulting their changes at all!

We should also probably take a look through known FlameGraph issues and see which ones also apply to inferno. For those that do, we should add tracking issues here, link to the original issue, and ideally also add a failing test case where possible!

jonhoo avatar Feb 03 '19 00:02 jonhoo

Hi @versable I am trying to help out the inferno project which is a rewrite of Flamegraph in Rust. Amoung other improvements to the port, we are looking at existing open changes in this project that we might be able to bring over. Would you be ok with us adapting your PR for SVG improvements for inferno? I am not certain all of the changes apply, but at least a few of them seem very promising!

jbcden avatar Feb 13 '19 12:02 jbcden

~@jonhoo: It looks like there are currently 2 PRs in the queue, one of which you seem to be actively handling (https://github.com/jonhoo/inferno/pull/32) and the other is waiting on the OP to fix some upstream issues they discovered with the PR (https://github.com/jonhoo/inferno/pull/38). Is there anything left for this issue specifically?~

ErichDonGubler avatar Feb 13 '19 17:02 ErichDonGubler

@ErichDonGubler ah, so this issue is about PRs to the original Perl implementation that we may be able to port over to the Rust version. I'm sure there are plenty of PRs from there that would be good to port!

jonhoo avatar Feb 13 '19 17:02 jonhoo

Ah, derp, sorry, that's my lack of reading. I apologize!

ErichDonGubler avatar Feb 13 '19 17:02 ErichDonGubler

@jbcden, Sure, go ahead. Very interesting project btw, I love it!

versable avatar Feb 17 '19 11:02 versable

Is there any interest in https://github.com/brendangregg/FlameGraph/pull/198? If so, I'd love to get my feet wet with Rust, and this would be really up my alley ;-)

versable avatar Mar 27 '19 06:03 versable

@versable oooh, that seems really neat, so absolutely! I'd be happy to help with a PR :)

jonhoo avatar Mar 27 '19 13:03 jonhoo

@versable, looks like this PR is completely in JS, so you can easily backport your changes into: https://github.com/jonhoo/inferno/blob/master/src/flamegraph/flamegraph.js

AnderEnder avatar Mar 28 '19 16:03 AnderEnder

@jonhoo, What do you think about "Support AsyncProfiler generated stack trace": https://github.com/brendangregg/FlameGraph/pull/234/

AnderEnder avatar Jun 17 '20 07:06 AnderEnder

@AnderEnder That looks like a good thing to support, though the fix looks dubious to me. The proposed regex, ^[^/].*\., matches anything that starts with a non-slash and contains a full stop, which seems overly broad to say "it's Java"?

jonhoo avatar Jun 17 '20 12:06 jonhoo

@jonhoo, I am waiting for some examples to try generate new regex.

AnderEnder avatar Jun 18 '20 21:06 AnderEnder