Skip to content
This repository has been archived by the owner on Dec 9, 2021. It is now read-only.

Update djdt-flamegraph to work with newest django debug toolbar #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aleksanb
Copy link

@aleksanb aleksanb commented Jun 4, 2020

Lifecycle methods changed on djdt 2.0 release, and scripts stopped
executing as well. This change requires you to add djdt_flamegraph to
INSTALLED_APPS.

Lifecycle methods changed on djdt 2.0 release, and scripts stopped
executing as well. This change requires you to add djdt_flamegraph to
INSTALLED_APPS.
@blopker
Copy link
Owner

blopker commented Jun 4, 2020

Hey @aleksanb, we don't really use this tool internally any more so I don't have a good way to test the changes. Happy to merge this though. However, why was the script extracted to its own file? Also, if extra steps are required to make this work, could you update the readme? Cheers.

@ngearhart
Copy link

ngearhart commented Jun 7, 2020

I installed this fork in my Django 2.2 project with django-debug-toolbar 2.2 and although the flamegraph tab now shows up in the toolbar, it is grayed out. My DEBUG_TOOLBAR_CONFIG.DISABLE_PANELS = {}, so I think it should be enabled. Is this a problem with this fork, or the base project?

Copy link

@jayvdb jayvdb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for me, but appear to have more changes than necessary

'flamegraph': flamegraph.stats_to_svg(self.sampler.get_stats())
}
return Template(template).render(Context(ctx))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesnt appear to have changed at all? Why modify it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a leftover from other attempts to fix the scripts not executing that i ended up removing.
I can remove it?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restoring this chunk would be good.

Also I think the minimum supported version of djdt should be increased to 2.2 - that is what works for me.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @aleksanb , can you restore this, so that the changes needed are more clear and not distorted by unneeded changes.

@aleksanb
Copy link
Author

aleksanb commented Jun 9, 2020

I installed this fork in my Django 2.2 project with django-debug-toolbar 2.2 and although the flamegraph tab now shows up in the toolbar, it is grayed out. My DEBUG_TOOLBAR_CONFIG.DISABLE_PANELS = {}, so I think it should be enabled. Is this a problem with this fork, or the base project?

Hi.

A thing i see i forgot to mention here, is that we're running django debug toolbar from master, as when debugging why the script moving content to the iframe wasn't executing in the frontend, i found this notice in the changelogs for unreleased djdt 3.0: https://django-debug-toolbar.readthedocs.io/en/latest/changes.html#a1-2020-06-05

Loading panel content no longer executes the scripts elements embedded in the HTML. Third party panels that require JavaScript resources should now use the Panel.scripts property.

Assuming that perhaps this already had broken on earlier versions, i just as well wrote this patch with support for 3.0 instead.

Conclusion: It might not actually work against djdt < 3.0.0

@ngearhart
Copy link

ngearhart commented Jun 9, 2020

@aleksanb I think it may work with djdt 2.2 - I upgraded to 3.0a1 and am seeing the same grayed out behavior

EDIT: Actually, turns out the styling of my site destroyed the checkbox I had to click to enable it. However, it appears blank - is this correct?
image

@aleksanb
Copy link
Author

EDIT: Actually, turns out the styling of my site destroyed the checkbox I had to click to enable it. However, it appears blank - is this correct?

the iframe should be filled with an enormous svg, if the script loaded correctly. I'm afraid that means you'll have to bump to 3.0.

@pktiuk
Copy link

pktiuk commented Dec 8, 2021

@blopker
Is there any chance of merging it? (this PR made flamegraph usable with django 3.2)

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

Successfully merging this pull request may close these issues.

5 participants