Added a distribution tab to visualize request time distributions over time by danielbradburn · Pull Request #281 · jazzband/django-silk

danielbradburn · 2018-05-04T20:35:23Z

This PR adds a distribution tab (at the moment opt in using the SILK_DISTRIBUTION_TAB configuration setting) with a visualisation of the request time distributions grouped by date or revision. We are using this at crunchr for continous benchmarking to ensure we don't introduce changes to our codebase that degrade the performance of our backend. This might be particularly useful for benchmarking django apps over time or versions.

…tion of the request timings, grouped either by date or revision. The default revision is taken from source control, however can be customized using the silk config for example to use a version number, or any other identifier.

… show the requests on the clicked group

…lter unit was in milliseconds

…adme

albertyw

I'm not sure if dealer is the best package to use here. It's not very popular and there are more tested and up-to-date python libraries to introspect version control information. A lot of django production deployments also won't contain git repository data.
Tests currently don't pass and should be fixed.

albertyw · 2018-05-06T06:34:50Z

 freezegun>=0.3
 factory-boy>=2.8.1
 gprof2dot>=2016.10.13,<2017.09.19
+dealer>=2.0.5


Checking out dealer, it seems dealer may be out of date. Have you tried testing with django >= 1.10 (klen/dealer#24)?

I will take a look for an alternative to dealer. I think the revision fuctionality is not that useful in production, at least we don't use it, we set SILKY_REVISION to a version number in settings.py. But it is quite useful for benchmarking locally, for example for comparing a number of commits. If I can't find a better alternative I guess another option is to just remove this dependency and let users set to the revision number using whichever method they prefer (probably nice to have an example in the readme)

I will check out the failing tests.

albertyw · 2018-05-06T06:44:18Z

@@ -0,0 +1,102 @@
+try:
+    # Django>=1.8


As of fa1f542, django-silk does not support django<1.11

Ah good, improves this code!

albertyw · 2018-05-06T06:47:59Z

@@ -0,0 +1,199 @@
+(function() {


Is this an unminified copy of url.min.js? Do we need both?

Yes and no, I will remove it.

@albertyw I removed this unnecessary file

albertyw · 2018-05-06T06:48:44Z

                   name="filter-overalltime-typ"/>
            <input type="text"
-                   placeholder="seconds"
+                   placeholder="milliseconds"


Is this a bugfix?

I guess this is a typo fix, since the placeholder says 'seconds' but the filter actually accepts 'milliseconds', do you think it makes sense to make a separate PR for this?

albertyw · 2018-05-06T06:52:19Z

+        timestamp = auto._repo.git('show -s --format=%at ' + revision)
+        revision = ' # '.join([timestamp.decode(), revision])
+    except:
+        pass


Does this make more sense to be revision = ''?

And does the rest of the app work correctly with an empty string or None value? There seem to be several areas of the app that filter or group by revision value.

@albertyw I removed the dealer dependency now - I think it's simpler just to let users to set the revision themselves using whichever method they prefer. This is what we have ended up doing when running this branch in production.

…equestsView

moagstar · 2020-10-17T15:21:17Z

@albertyw @avelis Apologies for letting this PR sit for so long. I have fixed the tests, and am curious if there is any further feedback or if this can be merged. Let me know if you have any questions.

codecov · 2021-04-13T03:44:57Z

Codecov Report

Merging #281 (ee6d6cb) into master (30eccfe) will decrease coverage by 52.03%.
The diff coverage is 25.24%.

@@             Coverage Diff             @@
##           master     #281       +/-   ##
===========================================
- Coverage   82.83%   30.80%   -52.04%     
===========================================
  Files          50       52        +2     
  Lines        2051     2136       +85     
===========================================
- Hits         1699      658     -1041     
- Misses        352     1478     +1126

Impacted Files	Coverage Δ
silk/config.py	`80.95% <ø> (-9.53%)`	⬇️
silk/templatetags/silk_inclusion.py	`0.00% <0.00%> (-96.56%)`	⬇️
silk/urls.py	`0.00% <0.00%> (-100.00%)`	⬇️
silk/views/clear_db.py	`0.00% <0.00%> (-96.00%)`	⬇️
silk/views/distribution.py	`0.00% <0.00%> (ø)`
silk/views/requests.py	`44.04% <16.66%> (-48.73%)`	⬇️
silk/request_filters.py	`37.93% <21.05%> (-27.48%)`	⬇️
silk/views/profiling.py	`27.08% <50.00%> (-60.42%)`	⬇️
silk/views/summary.py	`37.09% <50.00%> (-54.84%)`	⬇️
silk/models.py	`51.79% <59.09%> (-35.11%)`	⬇️
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30eccfe...ee6d6cb. Read the comment docs.

auvipy · 2021-04-13T03:42:55Z

@@ -0,0 +1,21 @@
+# -*- coding: utf-8 -*-


please remove this migrations and regen again with django 2.2+ only.

auvipy · 2021-04-13T03:44:21Z

-def filters_from_request(request):
+class RevisionFilter(BaseFilter):
+    def __init__(self, value):
+        super(RevisionFilter, self).__init__(value, revision=value)


no need of expliclit RevisionFilter, self) in super()

auvipy · 2021-04-13T03:47:50Z

+        </div>
+
+
+    </div>


auvipy · 2021-04-13T03:49:36Z

        view=CProfileView.as_view(),
        name='cprofile',
    ),
+    url(


using path() would be better choice as url() is deprected

auvipy · 2021-04-13T03:50:05Z

@@ -0,0 +1,53 @@
+import csv
+import contextlib
+from six import StringIO


six is not needed

Archmonger · 2021-09-03T23:38:05Z

@moagstar do you have time to address @auvipy 's change requests?

moagstar · 2021-09-14T18:56:47Z

@moagstar do you have time to address @auvipy 's change requests?

I am no longer working at crunchr, so I don't think have permissions to push to this branch any longer (in fact I made this PR with my crunchr account). I would probably have to create a new PR from my own fork. I am not sure when I would have time to look at this though, since we don't use silk at my new job and personal time for programming is quite limited at the moment.

auvipy · 2021-09-28T11:06:49Z

@moagstar do you have time to address @auvipy 's change requests?

I am no longer working at crunchr, so I don't think have permissions to push to this branch any longer (in fact I made this PR with my crunchr account). I would probably have to create a new PR from my own fork. I am not sure when I would have time to look at this though, since we don't use silk at my new job and personal time for programming is quite limited at the moment.

thanks for your work, i might end up opening a cleaner version of your work

eduzen · 2022-02-09T20:10:42Z

Hey @auvipy did you have time to check it? If not, I would like to help!

moagstar added 9 commits July 18, 2017 08:26

add .venv* to .gitignore

1475408

Merge branch 'master' of https://github.com/jazzband/silk

7dddd0e

Merge branch 'master' of https://github.com/jazzband/silk

15bdb19

Merge branch 'master' of https://github.com/jazzband/silk

8584a5f

Merge branch 'master' of https://github.com/jazzband/silk

65de365

Added link on last level of distribution view to requests filtered to…

cd8fef3

… show the requests on the clicked group

fixed typo in time filters where seconds were referred to, but the fi…

b7c903d

…lter unit was in milliseconds

added feature flag to opt in to distribution tab and added info to re…

3bfb00e

…adme

avelis requested a review from albertyw May 4, 2018 21:03

albertyw reviewed May 6, 2018

View reviewed changes

albertyw requested a review from avelis May 6, 2018 07:04

danielbradburn added 7 commits October 16, 2020 22:24

merge upstream master

6905851

use box plot to improve performance with lots of samples

b2a73d8

fix bug in clear_db where context was not passed

1ae7775

fix migration conflict

c9719ae

fix filtering of requests

4465479

fixed filtering on requests page

2d9ac63

Remove FilterableRequestsView - merged that functionality back into R…

4fa0574

…equestsView

nasirhjafri self-requested a review October 17, 2020 15:25

Merge branch 'master' into add-distribution-tab

ee6d6cb

auvipy suggested changes Apr 13, 2021

View reviewed changes

Uh oh!

Conversation

danielbradburn commented May 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

albertyw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moagstar commented Oct 17, 2020

Uh oh!

codecov bot commented Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Archmonger commented Sep 3, 2021

Uh oh!

moagstar commented Sep 14, 2021

Uh oh!

auvipy commented Sep 28, 2021

Uh oh!

eduzen commented Feb 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

danielbradburn commented May 4, 2018 •

edited

Loading

codecov bot commented Apr 13, 2021 •

edited

Loading