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

Profiling panel improvements #1669

Merged
merged 10 commits into from
Sep 16, 2022

Conversation

tim-schilling
Copy link
Member

  • Added Profiling panel setting PROFILER_THRESHOLD_RATIO to give users
    better control over how many function calls are included. A higher value
    will include more data, but increase render time.
  • Update Profiling panel to include try to always include user code. This
    code is more important to developers than dependency code.
  • Highlight the project function calls in the profiling panel.

Copy link
Member

@matthiask matthiask left a comment

Choose a reason for hiding this comment

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

Looks good. (I think.)

docs/panels.rst Outdated Show resolved Hide resolved
docs/configuration.rst Outdated Show resolved Hide resolved
docs/configuration.rst Outdated Show resolved Hide resolved
docs/panels.rst Outdated
The panel will include all function calls made by your project if your using
the setting ``settings.BASE_DIR`` to point to your project's root directory.
If a function is in a file within that directory and does not include
``"/site-packages/"`` in the path, it will be included.
Copy link
Member

Choose a reason for hiding this comment

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

I had some strange occurrences of broken Python installations when packages ended up in dist-packages as well. I couldn't really explain the difference -- all I know is that site-packages signify that everything is working well, dist-packages signify that many things will end up broken.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could check for that path here too.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be prudent to do so, both dist-packages and site-packages are very improbable to exist outside virtualenvs.

@tim-schilling
Copy link
Member Author

tim-schilling commented Sep 10, 2022

So a little context here @matthiask, as I was writing up my tutorial I wrote this view that iterated over 50k posts to calculate how many were created in the last 30, 90 and 180 days. I had a function that was called for each iteration, but this was wasn't immediately present in the profiling panel's information. So my initial thought was to change the / 8 calculation. Then I realized that we really should show any project code regardless of what it does. I ended up not needing the threshold control, but I left the change in case it's helpful to future developers. I'm happy to remove it if you feel we should.

Additionally, do you think we should have a killswitch preventing the or func.is_project_code() bypass? I'm thinking maybe we get something wrong with site-packages or dist-packages and there is too much to the profiling stats so it doesn't return.

@tim-schilling
Copy link
Member Author

@matthiask is it worth adding another setting to prevent this inclusion?

tim-schilling and others added 9 commits September 12, 2022 19:34
By checking that the code lives in the settings.BASE_DIR directory, we know
that the code was likely written by the user and thus more important
to a developer when debugging code.
Co-authored-by: Matthias Kestenholz <mk@feinheit.ch>
Co-authored-by: Matthias Kestenholz <mk@feinheit.ch>
Co-authored-by: Matthias Kestenholz <mk@feinheit.ch>
This can be used to disable the attempt to include all project code. This is
useful if dependencies are installed within the project.
@tim-schilling
Copy link
Member Author

@matthiask can you review the latest changes? If it's still good, we can squash merge it.

Copy link
Member

@matthiask matthiask left a comment

Choose a reason for hiding this comment

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

Thank you!

Additionally, do you think we should have a killswitch preventing the or func.is_project_code() bypass? I'm thinking maybe we get something wrong with site-packages or dist-packages and there is too much to the profiling stats so it doesn't return.

I'm a bit confused by the setting. The default was to skip some frames and now you want to add the option to include the projects' code no matter what? PROFILER_CAPTURE_PROJECT_CODE = False sounds to me as if the profiler wouldn't capture the projects' code at all. Naming is always borderline bikeshedding, but maybe we could avoid finding a good name for the setting by not making this configurable at all for now? Or maybe it's just me. I definitely don't have any requested changes and the code looks sound to me.

@tim-schilling
Copy link
Member Author

The default was to skip some frames and now you want to add the option to include the projects' code no matter what?

Correct. The system currently guesses at what frames it should include. My pitch is that we should include all of the project's code in the profiler. The killswitch I was mentioning ended up being PROFILER_CAPTURE_PROJECT_CODE. If that's set to False, it reverts back to how the profiling panel works today which includes frames depending on the timing ratio.

Naming is always borderline bikeshedding, but maybe we could avoid finding a good name for the setting by not making this configurable at all for now?

That's a fair comment. I feel strongly enough about having it be configurable (I can explain if you want me to) that I'd push for us to come up with a better name or merge it as is.

@matthiask
Copy link
Member

That's a fair comment. I feel strongly enough about having it be configurable (I can explain if you want me to) that I'd push for us to come up with a better name or merge it as is.

PROFILER_SKIP_PROJECT_CODE_FILTERING or something, maybe? If this doesn't sound better to you feel free to merge as-is :) Thank you!

@matthiask matthiask merged commit be0a433 into django-commons:main Sep 16, 2022
@matthiask
Copy link
Member

Let's move ahead. I want to try this out with a current project :)

@tim-schilling tim-schilling deleted the profiler-improvements branch September 20, 2022 00:58
@tim-schilling
Copy link
Member Author

Sorry, it was a long weekend for me. I think we can do another version release after #1673 is merged. Probably a minor version bump?

@matthiask
Copy link
Member

Nice! And yes, let's make a release after that.

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

Successfully merging this pull request may close these issues.

2 participants