Added a distribution tab to visualize request time distributions over time #281

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

requirements.txt

@@ -11,3 +11,4 @@ Django>=1.11
 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

silk/views/filterable_requests_view.py

@@ -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

silk/static/silk/lib/url.js

@@ -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

silk/templates/silk/profiling.html

@@ -94,13 +94,13 @@ <h4>Profile</h4>
                   value="OverallTimeFilter"
                   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

silk/config.py

+        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

silk/migrations/0008_request_revision.py

@@ -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

silk/request_filters.py

-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

silk/templates/silk/inclusion/request_filter.html

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


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

silk/urls.py

@@ -89,4 +91,19 @@
        view=CProfileView.as_view(),
        name='cprofile',
    ),
+    url(


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

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

silk/views/distribution.py

@@ -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

auvipy self-assigned this Nov 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a distribution tab to visualize request time distributions over time #281

Added a distribution tab to visualize request time distributions over time #281

danielbradburn commented May 4, 2018 •

edited

Loading

albertyw left a comment

albertyw May 6, 2018

danielbradburn May 6, 2018

danielbradburn May 6, 2018

albertyw May 6, 2018

danielbradburn May 6, 2018

albertyw May 6, 2018

danielbradburn May 6, 2018

moagstar Oct 17, 2020

albertyw May 6, 2018

danielbradburn May 6, 2018

albertyw May 6, 2018

moagstar Oct 17, 2020

moagstar commented Oct 17, 2020

codecov bot commented Apr 13, 2021 •

edited

Loading

auvipy Apr 13, 2021

auvipy Apr 13, 2021

auvipy Apr 13, 2021

auvipy Apr 13, 2021

auvipy Apr 13, 2021

Archmonger commented Sep 3, 2021

moagstar commented Sep 14, 2021

auvipy commented Sep 28, 2021

eduzen commented Feb 9, 2022

Added a distribution tab to visualize request time distributions over time #281

Are you sure you want to change the base?

Added a distribution tab to visualize request time distributions over time #281

Conversation

danielbradburn commented May 4, 2018 • edited Loading

albertyw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moagstar commented Oct 17, 2020

codecov bot commented Apr 13, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Archmonger commented Sep 3, 2021

moagstar commented Sep 14, 2021

auvipy commented Sep 28, 2021

eduzen commented Feb 9, 2022

danielbradburn commented May 4, 2018 •

edited

Loading

codecov bot commented Apr 13, 2021 •

edited

Loading