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

Add sampler profiling #623

Merged
merged 7 commits into from
May 4, 2023
Merged

Conversation

ivergara
Copy link
Collaborator

This PR leverages pyinstrument to provide sampling profiling to all endpoints exposed by quetz.

For this to happen, the enable_sampling configuration of the section profiling must be set to True and then addingprofiling=true (or any value that evaluates to True) as a query parameter. Once this is done, the response will not be the expected one, but an HTML view of the profiling done by pyinstrument.

@janjagusch janjagusch added the enhancement New feature or request label Apr 20, 2023
@janjagusch janjagusch self-requested a review April 20, 2023 09:13
@AndreasAlbertQC
Copy link
Collaborator

@ivergara could you add some documentation on the new config setting here?

@janjagusch janjagusch changed the title adding sampler profiling Add sampler profiling Apr 20, 2023
Copy link
Collaborator

@janjagusch janjagusch left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!

Something about your changes seems to break CI (it was running successfully on the default branch this morning). Could you please take a look?

quetz/main.py Outdated Show resolved Hide resolved
@ivergara
Copy link
Collaborator Author

@janjagusch @AndreasAlbertQC a few options

  1. should I add the option to activate profiling in the dev configuration?
  2. How do you recommend testing this? Or is enough manual QA since this is basically for dev purposes?

@janjagusch
Copy link
Collaborator

  1. should I add the option to activate profiling in the dev configuration?

You mean in this file? I don't have a strong preference. What about adding the section but setting it to false?

  1. How do you recommend testing this? Or is enough manual QA since this is basically for dev purposes?

I'm unsure how easily we could integrate this in the current testing suite. If it's too much work, then I suggest we leave it untested for now. Let's do a manual demonstration together, so I can see that it's working.

@codecov-commenter
Copy link

codecov-commenter commented Apr 22, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.84 ⚠️

Comparison is base (1f29a9c) 82.28% compared to head (025ccd7) 81.45%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #623      +/-   ##
==========================================
- Coverage   82.28%   81.45%   -0.84%     
==========================================
  Files          78       78              
  Lines        6124     6135      +11     
==========================================
- Hits         5039     4997      -42     
- Misses       1085     1138      +53     
Impacted Files Coverage Δ
quetz/config.py 90.20% <ø> (ø)
quetz/main.py 86.46% <100.00%> (+0.20%) ⬆️
quetz/testing/fixtures.py 95.67% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ivergara
Copy link
Collaborator Author

  1. should I add the option to activate profiling in the dev configuration?

You mean in this file? I don't have a strong preference. What about adding the section but setting it to false?

Indeed that file. Sure, I can change it to false.

  1. How do you recommend testing this? Or is enough manual QA since this is basically for dev purposes?

I'm unsure how easily we could integrate this in the current testing suite. If it's too much work, then I suggest we leave it untested for now. Let's do a manual demonstration together, so I can see that it's working.

That's why I suggested just going for manual QA only. I could see how to write a test that if profiling is enabled and the query parameter is given, the response is an HTML page instead of a JSON answer (probably via the content-type of the response). But this sounds like a very weak test.

@AndreasAlbertQC
Copy link
Collaborator

But this sounds like a very weak test.

True, but still better than no test, though. At least it will break if something in the setup changes (e.g. some unexpected breaking change in pyinstrument).

@janjagusch
Copy link
Collaborator

But this sounds like a very weak test.

True, but still better than no test, though. At least it will break if something in the setup changes (e.g. some unexpected breaking change in pyinstrument).

I'm also in favor of this smoke test. After that, we should be good to merge.

@janjagusch janjagusch merged commit f4c30fc into mamba-org:main May 4, 2023
@ivergara ivergara deleted the profiling_endpoints branch May 4, 2023 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants