Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adopt changes from upstream pull requests #26

Open
jonhoo opened this issue Feb 3, 2019 · 11 comments
Open

Adopt changes from upstream pull requests #26

jonhoo opened this issue Feb 3, 2019 · 11 comments
Labels
bug Something isn't working enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@jonhoo
Copy link
Owner

jonhoo commented Feb 3, 2019

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 jonhoo added bug Something isn't working enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Feb 3, 2019
@jbcden
Copy link

jbcden commented Feb 13, 2019

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!

@ErichDonGubler
Copy link

ErichDonGubler commented Feb 13, 2019

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

@jonhoo
Copy link
Owner Author

jonhoo commented Feb 13, 2019

@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!

@ErichDonGubler
Copy link

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

@versable
Copy link

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

@jonhoo jonhoo pinned this issue Mar 19, 2019
jonhoo pushed a commit that referenced this issue Mar 24, 2019
This patch ports brendangregg/FlameGraph#189 to inferno. The original author
has consented to us porting their work to this project:
#26 (comment).

Fixes #50.
@versable
Copy link

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

@jonhoo
Copy link
Owner Author

jonhoo commented Mar 27, 2019

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

@AnderEnder
Copy link
Contributor

@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
Copy link
Contributor

AnderEnder commented Jun 17, 2020

@jonhoo, What do you think about "Support AsyncProfiler generated stack trace": brendangregg/FlameGraph#234

@jonhoo
Copy link
Owner Author

jonhoo commented Jun 17, 2020

@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"?

@AnderEnder
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants