Skip to content

No flame chart when report loaded on (htmlpreview) GitHub #34

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

Closed
twilsonco opened this issue Sep 13, 2023 · 16 comments
Closed

No flame chart when report loaded on (htmlpreview) GitHub #34

twilsonco opened this issue Sep 13, 2023 · 16 comments

Comments

@twilsonco
Copy link

When I load a StatProfileHTML.jl report that's hosted on GitHub, the flame graph doesn't appear, though it appears if I open the "same" report locally in the same browser, it works fine:

local on GitHub
Screenshot 2023-09-13 at 9 14 42 AM Screenshot 2023-09-13 at 9 14 38 AM

I've disabled any blockers from running on GitHub.io, so that's not the problem.

How can I get the flame chart to render correctly? I feel like there must be some simple thing I'm missing here.

Background:
When a StatProfileHTML.jl report is saved in a GitHub repository, you can prepend https://htmlpreview.github.io/? to to full url to the index.html file in the repository and it will render the report in your browser.

@tkluck
Copy link
Owner

tkluck commented Sep 14, 2023

Thanks for the bug report! From your screenshots I was able to find this link here for reproducing it.

The github html previewer instructs the browser not to load frames and objects by using X-Frame-Options: deny. That's a security feature.

We might be able to work around it by inlining the flamegraph rather than putting it in a separate file. Is that something you want to contribute?

@twilsonco
Copy link
Author

Certainly! I'll need to review some background as I have very little experience with web anything, but it doesn't sound too bad. I'll see if I can inline the graph and open a PR.

Thanks

@tkluck
Copy link
Owner

tkluck commented Sep 23, 2023

@twilsonco how is it going? Feel free to post here if you run into any roadblocks.

@twilsonco
Copy link
Author

Hi there,

Before changing the HAML code, I wanted to get a MWE working. Simply copying the contents of flamegraph.svg into index.html correctly renders the flamegraph when prepending with https://htmlpreview.github.io/?, i.e. here.

However, when the user clicks on a cell in the flamegraph, the target url needs to have https://htmlpreview.github.io/? prepended, otherwise the user is shown the raw html for the target url instead.
Screenshot 2023-09-24 at 10 02 48 AM png collage
Interesting that it's only the links in the flamegraph that seem to have this problem. Other links in the report seem to redirect correctly. However the links in the "Methods sorted by exclusive sample count" table are non-functional, while they work when I load index.html locally in the browser. The "All methods", "All files", and the link in the second table work correctly.
image

My web-noob thoughts:

  • The more elegant solution is to use javascript to define a target URL prefix, initialized as a blank string, and set to the correct prefix if htmlpreview is in the current url. Then all links in the flamegraph get the prefix. Something like this or this, but I can only speculate with my lack of experience here.
  • Or perhaps the easier thing to do is just add an optional argument for a regex find-replace in StatProfileHTML.jl so that the resulting HTML can be fixed, though that comes with some potential issues (would like to have a dry-run option, for example).

I'm not certain either of these are valid approaches to begin with. Would love some feedback/direction.

@twilsonco
Copy link
Author

I haven't looked into why the other links in index.html aren't working.

@tkluck
Copy link
Owner

tkluck commented Sep 24, 2023

I just realized html preview is not an official github feature but it's a third party's. The good news is, it's open source: https://github.com/htmlpreview/htmlpreview.github.com .

I think what's going wrong is that it processes href attributes, but it doesn't know about the xlink:href attributes as you find them in svg. It looks like something you could easily fix in the //Links part in https://github.com/htmlpreview/htmlpreview.github.com/blob/master/htmlpreview.js .

So you'd want to fix that there.

In fact, I think this means you could fix the original issue there, too! Just give object tags the same treatment as iframe tags.

Want to ask there if they'd accept a contribution? It benefits many more people if we fix the limitation there, rather than working around it here.

@twilsonco
Copy link
Author

Cool! Thanks for noticing that. I agree that’s where the fix should go. Then I just need to modify the index.hamljl to inline the svg and it should work.

I’d be happy to go PR that.

@tkluck
Copy link
Owner

tkluck commented Sep 25, 2023 via email

@twilsonco
Copy link
Author

OK looking through their code and comparing to the output html from StatProfileHTML, I have questions. If this were to process the page, it would be easy to find the object field and redefine the data to point to the svg file. but would it able to load the SVG file so that links can be processed correctly? If so, then we could keep everything the same here and only fix stuff in the html preview tool.

If not, then we would still want to inline the flamegraph, and then as you suggest, we only need to modify the processing of links within the SVG code. It seems that links within the SVG can be processes the same as those without.

However, looking at it, it's not clear to me what needs to change.

//Links
a = document.querySelectorAll('a[href]');
for (i = 0; i < a.length; ++i) {
	href = a[i].href; //Get absolute URL
	if (href.indexOf('#') > 0) { //Check if it's an anchor
		a[i].href = '//' + location.hostname + location.pathname + location.search + '#' + a[i].hash.substring(1); //Then rewrite URL with support for empty anchor
	} else if ((href.indexOf('//raw.githubusercontent.com') > 0 || href.indexOf('//bitbucket.org') > 0) && (href.indexOf('.html') > 0 || href.indexOf('.htm') > 0)) { //Check if it's from raw.github.com or bitbucket.org and to HTML files
		a[i].href = '//' + location.hostname + location.pathname + '?' + href; //Then rewrite URL so it can be loaded using CORS proxy
	}
}

Let's look at three links on this page, one within the flamegraph that redirects incorrectly, one outside that redirects incorrectly, and one outside the flamegraph that correctly redirects to another htmlpreview URL.

First the link in the flamegraph for "helper function":

<a target='_top' xlink:href='fake-source.jl-4c8b0079d11b2925585882f699dd2fdccff100a6.html#L8'>
  <rect x='0.0' y='0' width='400.0' height='15.0' fill='rgb&#40;220, 161, 47&#41;' rx='2' ry='2' />
  <text text-anchor='' x='3.0' y='10.5' font-size='12' font-family='Verdana' fill='rgb&#40;0,0,0&#41;'>helper_function</text>
</a>

contains a #, so it triggers the first if condition and gets turned into
https://raw.githubusercontent.com/MolecularTheoryGroup/test_results/main/golden.diff-2023-09-24T09%3A21%3A44.937%20copy/fake-source.jl-4c8b0079d11b2925585882f699dd2fdccff100a6.html#L8
which is incorrect and loads the html source of the target URL, and so needs to be prepended with the htmlpreview url. This is what my link previewer in Safari shows for the link Open "file:///Users/haiiro/NoSync/StatProfilerHTML.jl/test/golden.diff-2023-09-23T19:44:55.452/fake-source.jl-4c8b0079d11b2925585882f699dd2fdccff100a6.html#L8" in another frame


Then the "main_function" link in the table of method counts,

<a href='fake-source.jl-4c8b0079d11b2925585882f699dd2fdccff100a6.html#L15'>main_function</a>

also contains a # and should trigger the first condition, which shows in my link previewer as Open #L15 on this page and doesn't load anything when pressed. When the profile results are loaded locally, the link previewer reads Open "file:///Users/haiiro/NoSync/StatProfilerHTML.j/test/golden.diff-2023-09-23T19:44:55.452/fake-source.jl-4c8b0079d11b2925585882699dd2fdccff100a6.html#L15"

So locally, it shows the correct link and loads it. Oh the htmlpreview it shows the incorrect link and does nothing when you click it.


Last, the "fake-source.jl" link in the second table, in the inlined html eventually has

<a href='fake-source.jl-4c8b0079d11b2925585882f699dd2fdccff100a6.html'>fake-source.jl</a>

which triggers the second part of the if instead of the first, and correctly redirects to https://htmlpreview.github.io/?https://raw.githubusercontent.com/MolecularTheoryGroup/test_results/main/golden.diff-2023-09-24T09%3A21%3A44.937%20copy/fake-source.jl-4c8b0079d11b2925585882f699dd2fdccff100a6.html


So, it appears that with the inlined SVG, links not containing # work properly, and links containing # can fail in one of two ways, either by failing to prepend the htmlpreview url, or by breaking the link altogether.

This would be fixed by altering line 25 of the htmlpreview script, but it's not clear to me what needs to change to correct for these two different issues.

@twilsonco
Copy link
Author

twilsonco commented Sep 25, 2023

Yes, I think that's how simple it might be. An easy way to try it out is by putting a fork on your own moleculartheorygroup.github.io . If it works, you can also keep using that until your pull request gets accepted.

Interesting. I erased this comment thinking it was clearly wrong lol.

How would this work though? All the links in the SVG would be processed if the SVG were inlined, but wouldn't there need to be additional js code that opened the contents of the object's data and processes the code within? Or would the htmlpreview inline the contents of the object?

@tkluck
Copy link
Owner

tkluck commented Sep 25, 2023

wouldn't there need to be additional js code that opened the contents of the object's data and processes the code within?

I think the browser will do another request to retrieve the object's data. However, you re-write the object's url so it points to your htmlpreview's index page instead. That will then grab the content, process it, and include it.

What's a bit harder is that it's svg code that you need to include rather than html code, but I think it can be done.

(By the way, you could also just use Github Pages for your reports directly -- if just means that you need to push them to a different repository. But I'm assuming you have a reason why you want to do it this way.)

@twilsonco
Copy link
Author

I'm doing it this way because my unit tests also produce a large number of plots etc. I was just after more immediate visibility of CI results. Eventually I want to take profiling, benchmarking, and graphics that result from testing and have everything compiled into a nice site. For example, my "results" repo would run its own action that produces a nice read me linking to other results and with tables of images.

Pages is probably the best way to go about this. At first I thought this way would be easier but it's turning out to have some baggage. And since I'm wanting to get a nice looking product already, pages is probably where it should go.

Guess this can be closed since it's really more of a bug with the html preview code.

@twilsonco
Copy link
Author

twilsonco commented Sep 25, 2023

Looks like for my purposes, it would either be multiple repositories' GitHub pages sites, linked from a common site, or, more conveniently, I use multiple branches in my "test results" repository, each appearing as its own pages site in a larger conglomerate site, by using GitHub pages and https://antora.org/ to assemble the branches' docs into a single site. (Found here)

This should allow me to have one site where I can easily view many profiling/benchmarking/test/coverage results.

Edit: Antora's no general solution, and requires use of the AsciiDoc markup language. Instead, some other method of combining the different static sites I'm wanting to host, such as linking from a TOC start page to each index.html.

@tkluck
Copy link
Owner

tkluck commented Sep 26, 2023

I'll leave it up to you what's the best way forward, and I'll close this issue for now.

I did find it possible to modify htmlpreview to work with svg objects, and I created a fork with the necessary changes here: htmlpreview/htmlpreview.github.com@master...tkluck:htmlpreview.github.com:master . Feel free to either

  • deploy that somewhere
  • or ask the maintainers to upstream the changes

so you can keep your current workflow.

@tkluck tkluck closed this as completed Sep 26, 2023
@twilsonco
Copy link
Author

I tried to PR your changes but it says I'm no collaborator. I need to become a collaborator in their project first then?

Untitled1 png collage

@twilsonco
Copy link
Author

Nevermind I just needed to PR from my own fork

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants