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

Replace the custom flamegraph viewer with speedscope #100

Open
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@jlfwong
Collaborator

jlfwong commented Jul 8, 2018

This PR replaces the custom flamegraph viewer introduced by @aroben in #74 (which sounds like it was a huge improvement over the existing SVG viewer!) with an integration of a high performance, WebGL-based, language agnostic profile viewer that I've been working on called speedscope: https://github.com/jlfwong/speedscope.

2018-07-07 23 18 16

If you don't think this is a good fit for this repository, no worries! I'm happy to either just close this PR, or to change it to be a dramatically reduced version which just makes it easier to output a format that speedscope can consume (basically just the JSON serialized version of a stackprof profile; speedscope has built-in code to handle import of stackprof profiles: https://github.com/jlfwong/speedscope/blob/master/import/stackprof.ts)

A script is included (vendor/speedscope/update.sh) to handle updating speedscope in the future to pull in the latest version.

It should be able to easily handle profiles at least as large as the existing viewer, and zooming & panning should remain 60fps within those very large profiles through efficient use of the GPU for rendering.

Test Plan:
Ran the following:

$ ruby sample.rb
$ bundle exec bin/stackprof --flamegraph-viewer=/tmp/stackprof.dump

I also preserved the original workflow of doing a separate compilation & opening step:

$ ruby sample.rb
$ bundle exec bin/stackprof --flamegraph=/tmp/stackprof.dump > /tmp/flamegraph
$ bundle exec bin/stackprof --flamegraph-viewer=/tmp/flamegraph

This should open the profile in-browser in both Linux and OS X using whatever your default configured browser is. I've only tested on OS X to date on Firefox & Chrome, but it shouldn't be OS dependent.

Screenshots:

image
image
image

@jlfwong

This comment has been minimized.

Show comment
Hide comment
@jlfwong

jlfwong Jul 20, 2018

Collaborator

@tmm1 @aroben Ping? Is there someone else I should be sending this to?

Collaborator

jlfwong commented Jul 20, 2018

@tmm1 @aroben Ping? Is there someone else I should be sending this to?

@jlfwong jlfwong requested a review from tmm1 Jul 20, 2018

@jlfwong jlfwong requested a review from aroben Jul 20, 2018

@tmm1 tmm1 unassigned aroben Jul 20, 2018

@tmm1 tmm1 removed the request for review from aroben Jul 20, 2018

@tmm1

This comment has been minimized.

Show comment
Hide comment
@tmm1

tmm1 Jul 20, 2018

Owner

I haven't looked at this code, but in theory this is fine by me.

I'm not using or maintaining this library anymore. @itsderek23 and @tenderlove are running the show here now.

Owner

tmm1 commented Jul 20, 2018

I haven't looked at this code, but in theory this is fine by me.

I'm not using or maintaining this library anymore. @itsderek23 and @tenderlove are running the show here now.

@jlfwong jlfwong requested review from tenderlove and itsderek23 and removed request for tmm1 Jul 20, 2018

@jlfwong jlfwong assigned tenderlove and itsderek23 and unassigned tmm1 Jul 20, 2018

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Jul 25, 2018

Collaborator

This is really awesome. I'm 👍 on this, but I need to test it with our app first

Collaborator

tenderlove commented Jul 25, 2018

This is really awesome. I'm 👍 on this, but I need to test it with our app first

@itsderek23

This comment has been minimized.

Show comment
Hide comment
@itsderek23

itsderek23 Jul 25, 2018

Collaborator

Yes - the viewer looks great @jlfwong.

I haven't used the current flamegraph viewer significantly.

@jlfwong - have you ran this against output from a Rails app? That's pretty common in the Ruby world.

Collaborator

itsderek23 commented Jul 25, 2018

Yes - the viewer looks great @jlfwong.

I haven't used the current flamegraph viewer significantly.

@jlfwong - have you ran this against output from a Rails app? That's pretty common in the Ruby world.

@NickLaMuro

This comment has been minimized.

Show comment
Hide comment
@NickLaMuro

NickLaMuro Jul 25, 2018

Contributor

I have some large samples sitting around somewhere that I have run against https://github.com/ManageIQ/manageiq , so I can give that a shot at some point tonight and report back.

(Update: I will do this tomorrow... it is late 🌝 )

Contributor

NickLaMuro commented Jul 25, 2018

I have some large samples sitting around somewhere that I have run against https://github.com/ManageIQ/manageiq , so I can give that a shot at some point tonight and report back.

(Update: I will do this tomorrow... it is late 🌝 )

@NickLaMuro

So I think I might have more to say, but since I don't even have merge rights and have just been "nerd sniped" into reviewing because I use this repo myself regularly, I gave some comments.

Mostly personal opinion here, so feel free to take it or leave it.

Seems cool regardless!

-Nick

Show outdated Hide outdated bin/stackprof
Show outdated Hide outdated lib/stackprof/report.rb
@@ -0,0 +1,16 @@
#!/bin/bash

This comment has been minimized.

@NickLaMuro

NickLaMuro Jul 25, 2018

Contributor

Could this possibly turned into a Ruby script/rake task?

The Gem::Package::TarReader should be able to handle the Tar portion of things (cross platform), and then you can basically use FileUtils for the rest.

Obviously npm is still required and would need a shell out, but that is just a given and as you have stated it is just a developer dependency.

@NickLaMuro

NickLaMuro Jul 25, 2018

Contributor

Could this possibly turned into a Ruby script/rake task?

The Gem::Package::TarReader should be able to handle the Tar portion of things (cross platform), and then you can basically use FileUtils for the rest.

Obviously npm is still required and would need a shell out, but that is just a given and as you have stated it is just a developer dependency.

This comment has been minimized.

@jlfwong

jlfwong Jul 25, 2018

Collaborator

It could be, but I'm not sure it's worth doing -- is the target benefit to make it possible for a Windows-only maintainer to run the update script?

@jlfwong

jlfwong Jul 25, 2018

Collaborator

It could be, but I'm not sure it's worth doing -- is the target benefit to make it possible for a Windows-only maintainer to run the update script?

This comment has been minimized.

@NickLaMuro

NickLaMuro Jul 26, 2018

Contributor

So there are a few non-"but what about WIN-DOSE" reasons I tend to always push for this myself, and it very much is pedantic in most cases... BUT YOU ASKED FOR IT! (he says sarcastically):

(I am writing a lot here, but again this is just an explanation to my personal opinionated stance and just what I do myself and why. Zero pressure to make the change and feel free to take it or leave it.)

  • Shelling out has some cost associated with it. Next to nothing 99% of the time, but depending on how much you need to do it, it can add up. In your case, nothing here of note or value where this is actually an issue, just an FYI.
  • When I use "Windows" as an example, bit is more about removing inconsistencies between platforms, and since we know Ruby is going to be used by who ever is running this script, we can assume it will be there as a more consistent constant (#redundantWordsAreRedundant). For a few examples in your case:
    • A user might have some weird version of /bin/bash (probably a bad example for this use case...)
    • which might not exist on the $PATH on some machines, or the user might be running with sudo
    • weird user aliases being loaded that mess with the calls in the script
  • Specifically to the rake suggestion, this is simply for dev/maintainer consistency. I know that you did put a README together for the speedscope stuff specifically, but from personal experience, most people don't RTFM. But assuming they would at least do a rake -T when they clone the project (which rake is pretty much accepted as the build pipeline tool for ruby projects), they can get at a glance what general dev tasks are available.
    • Further more, if it is written in ruby, we then are just doing Ruby -> npm for that one time we need to shell out, instead of Ruby -> shell -> npm/tar/.... Less moving parts and less places for failure.

</2cents>

@NickLaMuro

NickLaMuro Jul 26, 2018

Contributor

So there are a few non-"but what about WIN-DOSE" reasons I tend to always push for this myself, and it very much is pedantic in most cases... BUT YOU ASKED FOR IT! (he says sarcastically):

(I am writing a lot here, but again this is just an explanation to my personal opinionated stance and just what I do myself and why. Zero pressure to make the change and feel free to take it or leave it.)

  • Shelling out has some cost associated with it. Next to nothing 99% of the time, but depending on how much you need to do it, it can add up. In your case, nothing here of note or value where this is actually an issue, just an FYI.
  • When I use "Windows" as an example, bit is more about removing inconsistencies between platforms, and since we know Ruby is going to be used by who ever is running this script, we can assume it will be there as a more consistent constant (#redundantWordsAreRedundant). For a few examples in your case:
    • A user might have some weird version of /bin/bash (probably a bad example for this use case...)
    • which might not exist on the $PATH on some machines, or the user might be running with sudo
    • weird user aliases being loaded that mess with the calls in the script
  • Specifically to the rake suggestion, this is simply for dev/maintainer consistency. I know that you did put a README together for the speedscope stuff specifically, but from personal experience, most people don't RTFM. But assuming they would at least do a rake -T when they clone the project (which rake is pretty much accepted as the build pipeline tool for ruby projects), they can get at a glance what general dev tasks are available.
    • Further more, if it is written in ruby, we then are just doing Ruby -> npm for that one time we need to shell out, instead of Ruby -> shell -> npm/tar/.... Less moving parts and less places for failure.

</2cents>

This comment has been minimized.

@jlfwong

jlfwong Jul 27, 2018

Collaborator

@tenderlove Do you have opinions on this front? I don't spend too much time in the ruby ecosystem, so I'm not sure what the expectations are on this front. I'm happy to do this if you feel strongly, but would otherwise bias towards not doing it, since not doing it less work 😅

@jlfwong

jlfwong Jul 27, 2018

Collaborator

@tenderlove Do you have opinions on this front? I don't spend too much time in the ruby ecosystem, so I'm not sure what the expectations are on this front. I'm happy to do this if you feel strongly, but would otherwise bias towards not doing it, since not doing it less work 😅

This comment has been minimized.

@NickLaMuro

NickLaMuro Jul 27, 2018

Contributor

@jlfwong I think you meant to direct this at me, but I can put something together for you if it is decided this would be preferred.

@NickLaMuro

NickLaMuro Jul 27, 2018

Contributor

@jlfwong I think you meant to direct this at me, but I can put something together for you if it is decided this would be preferred.

This comment has been minimized.

@NickLaMuro

NickLaMuro Jul 27, 2018

Contributor

Sorry, I misread the past comment and just noticed your true intent. Again, apologize for the extra noise.

I did, however, put together some sample code to show how this could be done, but again, no pressure to actually implement this, and it was mostly a interesting exercise for myself.

# Rakefile  (I personally put this starting around line 11)

def untar(tarfile)
  # Little bit of a hack with the `.new` to get this to work
  Gem::Package.new("").open_tar_gz tarfile do |tar|
    tar.each { |file| yield file }
  end
end

desc "Update speedscope assets"
task :update_speedscope do
  rm_rf   "vendor/speedscope"
  mkdir_p "vendor/speedscope"

  cd "vendor/speedscope" do
    sh "npm pack speedscope"

    File.open Dir.glob("speedscope*.tgz").first do |tarfile|
      untar tarfile do |file|
        next unless file.full_name =~ /^package\/(LICENSE|dist\/release\/.*(html|css|js|png))$/

        File.write File.basename(file.full_name), file.read
      end

      rm tarfile.path
    end
  end
end

One note, this does change the scoping of the resulting directory structure from vendor/speedscope/speedscope to just vendor/speedscope, since the nested dir seemed redundant (but I could be missing something). I would assume that some changes in report.rb would be necessary if this is a reasonable change, otherwise a few tweaks to this task could be made to emulate what already exists.

@NickLaMuro

NickLaMuro Jul 27, 2018

Contributor

Sorry, I misread the past comment and just noticed your true intent. Again, apologize for the extra noise.

I did, however, put together some sample code to show how this could be done, but again, no pressure to actually implement this, and it was mostly a interesting exercise for myself.

# Rakefile  (I personally put this starting around line 11)

def untar(tarfile)
  # Little bit of a hack with the `.new` to get this to work
  Gem::Package.new("").open_tar_gz tarfile do |tar|
    tar.each { |file| yield file }
  end
end

desc "Update speedscope assets"
task :update_speedscope do
  rm_rf   "vendor/speedscope"
  mkdir_p "vendor/speedscope"

  cd "vendor/speedscope" do
    sh "npm pack speedscope"

    File.open Dir.glob("speedscope*.tgz").first do |tarfile|
      untar tarfile do |file|
        next unless file.full_name =~ /^package\/(LICENSE|dist\/release\/.*(html|css|js|png))$/

        File.write File.basename(file.full_name), file.read
      end

      rm tarfile.path
    end
  end
end

One note, this does change the scoping of the resulting directory structure from vendor/speedscope/speedscope to just vendor/speedscope, since the nested dir seemed redundant (but I could be missing something). I would assume that some changes in report.rb would be necessary if this is a reasonable change, otherwise a few tweaks to this task could be made to emulate what already exists.

This comment has been minimized.

@jlfwong

jlfwong Aug 2, 2018

Collaborator

Thanks for writing up sample code! I always appreciate it when people are willing to take the time to write code to support their ideas.

That said, I'm not planning on changing this to a rake task unless this is considered a blocker for merging.

My reasoning here is that I'm intending to make similar changes to several different profilers in many different languages (e.g. pyflame), and would prefer the update scripts to look as similar as possible. Changing them to be specific to the language of the profiler would make that more difficult.

@jlfwong

jlfwong Aug 2, 2018

Collaborator

Thanks for writing up sample code! I always appreciate it when people are willing to take the time to write code to support their ideas.

That said, I'm not planning on changing this to a rake task unless this is considered a blocker for merging.

My reasoning here is that I'm intending to make similar changes to several different profilers in many different languages (e.g. pyflame), and would prefer the update scripts to look as similar as possible. Changing them to be specific to the language of the profiler would make that more difficult.

Show outdated Hide outdated lib/stackprof/report.rb
@jlfwong

This comment has been minimized.

Show comment
Hide comment
@jlfwong

jlfwong Jul 25, 2018

Collaborator

@jlfwong - have you ran this against output from a Rails app? That's pretty common in the Ruby world.

I have not, but have this integrated into https://github.com/jlfwong/rack-mini-profiler, which we use to profile a sinatra app regularly at Figma.

We also use speedscope at Figma to open 100MB+ profiles generated by Chrome, which it handles relatively gracefully.

Collaborator

jlfwong commented Jul 25, 2018

@jlfwong - have you ran this against output from a Rails app? That's pretty common in the Ruby world.

I have not, but have this integrated into https://github.com/jlfwong/rack-mini-profiler, which we use to profile a sinatra app regularly at Figma.

We also use speedscope at Figma to open 100MB+ profiles generated by Chrome, which it handles relatively gracefully.

@tenderlove

We need to make sure the viewer and JSON are separate (it looks like this PR will do that), but also ensure we can print the flamegraph JSON data to an IO of our choosing. I think this PR removes the that ability (though I could be wrong because the diff is pretty large 😅).

As long as we have an API that can ensure those things, then I'm happy. 😊

Show outdated Hide outdated bin/stackprof
@@ -80,65 +83,36 @@ def print_stackcollapse
end
end
def print_flamegraph(f=STDOUT, skip_common=true)

This comment has been minimized.

@tenderlove

tenderlove Jul 27, 2018

Collaborator

We need to maintain this API. We're using flamegraphs in production by serving a static asset (the flamegraph viewer) and it makes a request to and endpoint that serves up the flamegraph JSON for a particular page. We use this method to print the flamegraph JSON to the socket

@tenderlove

tenderlove Jul 27, 2018

Collaborator

We need to maintain this API. We're using flamegraphs in production by serving a static asset (the flamegraph viewer) and it makes a request to and endpoint that serves up the flamegraph JSON for a particular page. We use this method to print the flamegraph JSON to the socket

This comment has been minimized.

@jlfwong

jlfwong Jul 27, 2018

Collaborator

Can you elaborate on the workflow you use? This function as it currently exists doesn't print JSON directly, but rather prints a JavaScript function invocation whose only argument happens to be valid JSON.

Do you take the output of this and strip the flamegraph( at the beginning and the trailing ) at the end then serve it as true JSON?

Or, when you say

it makes a request to and endpoint that serves up the flamegraph JSON for a particular page

do you mean that you set up the flamegraph viewer to include a <script> which references the exact output of print_flamegraph?

Both before and after this PR, the data and the viewer are separated, and before and after this PR the data is valid JavaScript but not valid JSON without modification.

So I think the request here is that "the step for extracting the JavaScript file and the step for opening the browser need to be separate steps so that consumers of stackprof as a library can serve the JavaScript file manually".

If that's the request, I'm going to open up a different code pathway for this.

Speedscope has multiple ways of loading data.

  1. Dropping local files in
  2. Browsing for local files
  3. Specifying files via (possibly CORS) XHR URLs, which would must not contain the JavaScript function invocation wrapper (documented here: https://github.com/jlfwong/speedscope#importing-via-url)
  4. Specifying files via file:// protocol script URLs which must contain the Javascript function invocation wrapper, which is what this PR does since you can't make XHR requests to file:// protocol URLs.

So if I understand you correctly, here's a proposed remediation:

  1. Add a method called get_speedscope_json which returns a JSON string (not a JavaScript string) which could then be served over HTTP
  2. Use that method in view_flamegraph_in_browser to retain the behavior in the PR as it stands

Then to upgrade your integration with stackprof, it would require changing the call-site to use get_speedscope_json instead of print_flamegraph_json, and also change the static assets that are being served right now to serve speedscope rather than serving the existing flamegraph viewer.

How does that path forward sound to you?

@jlfwong

jlfwong Jul 27, 2018

Collaborator

Can you elaborate on the workflow you use? This function as it currently exists doesn't print JSON directly, but rather prints a JavaScript function invocation whose only argument happens to be valid JSON.

Do you take the output of this and strip the flamegraph( at the beginning and the trailing ) at the end then serve it as true JSON?

Or, when you say

it makes a request to and endpoint that serves up the flamegraph JSON for a particular page

do you mean that you set up the flamegraph viewer to include a <script> which references the exact output of print_flamegraph?

Both before and after this PR, the data and the viewer are separated, and before and after this PR the data is valid JavaScript but not valid JSON without modification.

So I think the request here is that "the step for extracting the JavaScript file and the step for opening the browser need to be separate steps so that consumers of stackprof as a library can serve the JavaScript file manually".

If that's the request, I'm going to open up a different code pathway for this.

Speedscope has multiple ways of loading data.

  1. Dropping local files in
  2. Browsing for local files
  3. Specifying files via (possibly CORS) XHR URLs, which would must not contain the JavaScript function invocation wrapper (documented here: https://github.com/jlfwong/speedscope#importing-via-url)
  4. Specifying files via file:// protocol script URLs which must contain the Javascript function invocation wrapper, which is what this PR does since you can't make XHR requests to file:// protocol URLs.

So if I understand you correctly, here's a proposed remediation:

  1. Add a method called get_speedscope_json which returns a JSON string (not a JavaScript string) which could then be served over HTTP
  2. Use that method in view_flamegraph_in_browser to retain the behavior in the PR as it stands

Then to upgrade your integration with stackprof, it would require changing the call-site to use get_speedscope_json instead of print_flamegraph_json, and also change the static assets that are being served right now to serve speedscope rather than serving the existing flamegraph viewer.

How does that path forward sound to you?

This comment has been minimized.

@tenderlove

tenderlove Jul 27, 2018

Collaborator

Yes, this sounds perfect!

@tenderlove

tenderlove Jul 27, 2018

Collaborator

Yes, this sounds perfect!

This comment has been minimized.

@jlfwong

jlfwong Aug 2, 2018

Collaborator

This should be complete now

@jlfwong

jlfwong Aug 2, 2018

Collaborator

This should be complete now

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Jul 31, 2018

Collaborator

I've tried integrating this in to production, but it seems like speedscope.c6a476e8.js tries to dynamically download other stuff (and our content policy blocks it). Is there a way we can combine it in to one script?

Collaborator

tenderlove commented Jul 31, 2018

I've tried integrating this in to production, but it seems like speedscope.c6a476e8.js tries to dynamically download other stuff (and our content policy blocks it). Is there a way we can combine it in to one script?

@jlfwong

This comment has been minimized.

Show comment
Hide comment
@jlfwong

jlfwong Jul 31, 2018

Collaborator

I've tried integrating this in to production, but it seems like speedscope.c6a476e8.js tries to dynamically download other stuff (and our content policy blocks it). Is there a way we can combine it in to one script?

Hmm. It's going to be a bit tricky, but it's doable. The reason it downloads other things is to speed up loading so that it can present a UI and allow users to browse for a file without needing to wait for all the code that does the actual importing.

If I'm going down that path anyway, would you prefer that everything is inlined into the .html file instead?

I'll look into it when I change the codepaths to split get_speedscope_json and print_flamegraph_json.

Collaborator

jlfwong commented Jul 31, 2018

I've tried integrating this in to production, but it seems like speedscope.c6a476e8.js tries to dynamically download other stuff (and our content policy blocks it). Is there a way we can combine it in to one script?

Hmm. It's going to be a bit tricky, but it's doable. The reason it downloads other things is to speed up loading so that it can present a UI and allow users to browse for a file without needing to wait for all the code that does the actual importing.

If I'm going down that path anyway, would you prefer that everything is inlined into the .html file instead?

I'll look into it when I change the codepaths to split get_speedscope_json and print_flamegraph_json.

@jlfwong

This comment has been minimized.

Show comment
Hide comment
@jlfwong

jlfwong Aug 1, 2018

Collaborator

@tenderlove To be clear, are you talking about Content-Security-Policy? If so, can you provide what Content-Security-Policy header you're working with?

I'm curious how you have this set up with the current flamechart viewer, given that I thought it loads scripts dynamically too?

Collaborator

jlfwong commented Aug 1, 2018

@tenderlove To be clear, are you talking about Content-Security-Policy? If so, can you provide what Content-Security-Policy header you're working with?

I'm curious how you have this set up with the current flamechart viewer, given that I thought it loads scripts dynamically too?

@jlfwong

This comment has been minimized.

Show comment
Hide comment
@jlfwong

jlfwong Aug 2, 2018

Collaborator

@tenderlove My working assumption is that you have a Content-Security-Policy header which does not include strict-dynamic. Is that right? https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src#strict-dynamic

Collaborator

jlfwong commented Aug 2, 2018

@tenderlove My working assumption is that you have a Content-Security-Policy header which does not include strict-dynamic. Is that right? https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src#strict-dynamic

o.on('--flamegraph-viewer [f.js]', String, "open html viewer for flamegraph output\n\n"){ |file|
puts("open file://#{File.expand_path('../../lib/stackprof/flamegraph/viewer.html', __FILE__)}?data=#{File.expand_path(file)}")
o.on('--flamegraph', "output format for consumption by --flamegraph-viewer"){ options[:format] = :flamegraph }
o.on('--flamegraph-viewer [profile-path]', "open a viewer for the flamegraph of the given profile"){ |f|

This comment has been minimized.

@jlfwong

jlfwong Aug 2, 2018

Collaborator

Okay, I switched this back to preserve the original switches, with the added ability to use --flamegraph-viewer directly on a profile file without needing to do the compile step first, but also supporting doing the compile step first to preserve people's existing workflows

@jlfwong

jlfwong Aug 2, 2018

Collaborator

Okay, I switched this back to preserve the original switches, with the added ability to use --flamegraph-viewer directly on a profile file without needing to do the compile step first, but also supporting doing the compile step first to preserve people's existing workflows

@jlfwong

This comment has been minimized.

Show comment
Hide comment
@jlfwong

jlfwong Aug 2, 2018

Collaborator

Okay, @tenderlove I updated the PR using a build which shouldn't do any dynamic script loading. The work is based on an open PR on speedscope, so I pulled in the tarball for it manually rather than using npm pack speedscope. If does end up meeting your needs, I'll land the PR and push to npm.

Here's the relevant PR on speedscope if you're curious: jlfwong/speedscope#113

Collaborator

jlfwong commented Aug 2, 2018

Okay, @tenderlove I updated the PR using a build which shouldn't do any dynamic script loading. The work is based on an open PR on speedscope, so I pulled in the tarball for it manually rather than using npm pack speedscope. If does end up meeting your needs, I'll land the PR and push to npm.

Here's the relevant PR on speedscope if you're curious: jlfwong/speedscope#113

@jlfwong

This comment has been minimized.

Show comment
Hide comment
@jlfwong

jlfwong Aug 5, 2018

Collaborator

@tenderlove I'm also interested to know if the CSP policy you have set up prohibits the use of eval. That might end up being a bigger problem since one of the core libraries I depend upon uses eval: https://github.com/regl-project/regl

Collaborator

jlfwong commented Aug 5, 2018

@tenderlove I'm also interested to know if the CSP policy you have set up prohibits the use of eval. That might end up being a bigger problem since one of the core libraries I depend upon uses eval: https://github.com/regl-project/regl

@jlfwong

This comment has been minimized.

Show comment
Hide comment
@jlfwong

jlfwong Aug 9, 2018

Collaborator

@tenderlove ^ ping

Collaborator

jlfwong commented Aug 9, 2018

@tenderlove ^ ping

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Aug 15, 2018

Collaborator

@jlfwong hey, sorry it took so long to get back to you. Yes, our CSP won't allow eval. I'm not totally sure what to do here. I really like the viewer in this PR, but it sounds like we may not be able to use it in production.

My goal is that folks at work can just click a link and see flame graphs of the page.

I really want this in stackprof because it's hands down better than the existing one. Maybe we could keep an API that outputs data that will work with the old viewer?

Collaborator

tenderlove commented Aug 15, 2018

@jlfwong hey, sorry it took so long to get back to you. Yes, our CSP won't allow eval. I'm not totally sure what to do here. I really like the viewer in this PR, but it sounds like we may not be able to use it in production.

My goal is that folks at work can just click a link and see flame graphs of the page.

I really want this in stackprof because it's hands down better than the existing one. Maybe we could keep an API that outputs data that will work with the old viewer?

@jlfwong

This comment has been minimized.

Show comment
Hide comment
@jlfwong

jlfwong Aug 15, 2018

Collaborator

@tenderlove Got it. Yeah, that makes sense!

My goal is that folks at work can just click a link and see flame graphs of the page.

Definitely a noble goal :)

A couple of alternative options to potentially consider. You've probably thought of these already, but just to make sure that these are all show-stoppers:

  1. Have a different CSP specifically for the page loading speedscope
  2. Host speedscope on a totally different domain without the same CSP protection, and then allow access to the API endpoint which serves the profile file cross-domain via an Access-Control-Allow-Origin header. I have a friend which uses speedscope at another company in exactly this way: their profiles are uploaded to S3, and a header is set on the S3 bucket to allow them to be accessed cross domain. This opens a different variety of attack vector that might be unreasonable to open, even if done only for that specific endpoint, but speedscope being hosted on a different domain would guard against cookie jacking attacks via a theoretical XSS hole.

If those are both no-gos, then I'll look into either preserving the existing flamechart viewer or into switching the WebGL abstraction I'm using to one which doesn't use eval.

Collaborator

jlfwong commented Aug 15, 2018

@tenderlove Got it. Yeah, that makes sense!

My goal is that folks at work can just click a link and see flame graphs of the page.

Definitely a noble goal :)

A couple of alternative options to potentially consider. You've probably thought of these already, but just to make sure that these are all show-stoppers:

  1. Have a different CSP specifically for the page loading speedscope
  2. Host speedscope on a totally different domain without the same CSP protection, and then allow access to the API endpoint which serves the profile file cross-domain via an Access-Control-Allow-Origin header. I have a friend which uses speedscope at another company in exactly this way: their profiles are uploaded to S3, and a header is set on the S3 bucket to allow them to be accessed cross domain. This opens a different variety of attack vector that might be unreasonable to open, even if done only for that specific endpoint, but speedscope being hosted on a different domain would guard against cookie jacking attacks via a theoretical XSS hole.

If those are both no-gos, then I'll look into either preserving the existing flamechart viewer or into switching the WebGL abstraction I'm using to one which doesn't use eval.

@jlfwong

This comment has been minimized.

Show comment
Hide comment
@jlfwong

jlfwong Aug 19, 2018

Collaborator

@tenderlove Okay, I've updated the PR to use a version which does not use eval.

To validate that it was working, I opened it via a local server and specified the following header:

Content-Security-Policy: script-src 'self';

I was able to validate that the version before the removal of eval calls did not work with that policy, and that the version now in this PR does. It doesn't seem like the dynamic script loading was the problem, I think it was just the eval calls, which is great because it means that I don't have to maintain two different builds :)

Can you see if this now works for GitHub's content security policy?

If it turns out that I'm wrong and that both the eval calls and the dynamic file loading were causing issues, I can easily switch this to a build that has both removed.

Collaborator

jlfwong commented Aug 19, 2018

@tenderlove Okay, I've updated the PR to use a version which does not use eval.

To validate that it was working, I opened it via a local server and specified the following header:

Content-Security-Policy: script-src 'self';

I was able to validate that the version before the removal of eval calls did not work with that policy, and that the version now in this PR does. It doesn't seem like the dynamic script loading was the problem, I think it was just the eval calls, which is great because it means that I don't have to maintain two different builds :)

Can you see if this now works for GitHub's content security policy?

If it turns out that I'm wrong and that both the eval calls and the dynamic file loading were causing issues, I can easily switch this to a build that has both removed.

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