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

Added support of Oracle #619

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions project/tests/test_models.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import datetime
import uuid

from django.core.management import call_command
from django.test import TestCase, override_settings
Expand Down Expand Up @@ -37,10 +36,6 @@ def tearDown(self):
SilkyConfig().SILKY_MAX_RECORDED_REQUESTS_CHECK_PERCENT = self.max_percent
SilkyConfig().SILKY_MAX_RECORDED_REQUESTS = self.max_requests

def test_uuid_is_primary_key(self):

self.assertIsInstance(self.obj.id, uuid.UUID)

@freeze_time('2016-01-01 12:00:00')
def test_start_time_field_default(self):

Expand Down Expand Up @@ -202,10 +197,6 @@ def setUp(self):

self.obj = ResponseFactory.create()

def test_uuid_is_primary_key(self):

self.assertIsInstance(self.obj.id, uuid.UUID)

def test_is_1to1_related_to_request(self):

request = RequestMinFactory.create()
Expand Down
101 changes: 101 additions & 0 deletions silk/migrations/0009_add_oracle_support.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
# Generated by Django 1.9.7 on 2018-01-10 14:14
Copy link
Member

Choose a reason for hiding this comment

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

This is a very old version of Django. Should this file be regenerated using a supported version of django?

Copy link
Member

Choose a reason for hiding this comment

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

Also, a migration here may require the parent django application to run the migration when upgrading/downgrading django-silk which we should avoid, or if we really need to, should be very clearly messaged in the changelog.

As this migration is currently written, I think upgrading django-silk would require wiping the silk database and downgrading django-silk would not be supported (unless several manual steps are taken to reset the django-silk database which would also involve wiping the database). This PR would need be a breaking change requiring a major version bump.


from django.db import migrations, models
from django.db.backends.utils import truncate_name

import silk.storage


def create_autoinc(table_name, column_name):
def autoinc(apps, schema_editor):
autoinc_sql = schema_editor.connection.ops.autoinc_sql(table_name, column_name)
if autoinc_sql:
schema_editor.deferred_sql.extend(autoinc_sql)

return autoinc


def remove_autoinc(table_name, column_name):
def autoinc(apps, schema_editor):
def _get_trigger_name(table):
name_length = schema_editor.connection.ops.max_name_length() - 3
return schema_editor.connection.ops.quote_name('%s_TR' % truncate_name(table, name_length).upper())

seq_sql = schema_editor.connection.ops.drop_sequence_sql(table_name)
if seq_sql:
schema_editor.execute(seq_sql)
schema_editor.execute('DROP TRIGGER %s;' % _get_trigger_name(table_name))

return autoinc


class Migration(migrations.Migration):

dependencies = [
('silk', '0008_sqlquery_analysis'),
]

operations = [
migrations.RemoveField(
model_name='response',
name='request'
),
migrations.RemoveField(
model_name='sqlquery',
name='request'
),
migrations.RemoveField(
model_name='profile',
name='request'
),
Comment on lines +39 to +50
Copy link
Member

Choose a reason for hiding this comment

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

Is this valid? I don't see this appearing in silk/models.py.


migrations.RemoveField(
model_name='request',
name='id'
),
migrations.RemoveField(
model_name='response',
name='id'
),

migrations.AddField(
model_name='request',
name='id',
field=models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID'),
),
migrations.AddField(
model_name='response',
name='id',
field=models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID'),
),

migrations.AddField(
model_name='response',
name='request',
field=models.OneToOneField(to='silk.Request', related_name='response', on_delete=models.CASCADE),
),
migrations.AddField(
model_name='sqlquery',
name='request',
field=models.ForeignKey(to='silk.Request', blank=True, null=True, related_name='queries', on_delete=models.CASCADE),
),
migrations.AddField(
model_name='profile',
name='request',
field=models.ForeignKey(to='silk.Request', blank=True, null=True, on_delete=models.CASCADE),
),

migrations.RunPython(create_autoinc('silk_request', 'id'), remove_autoinc('silk_request', 'id')),
migrations.RunPython(create_autoinc('silk_response', 'id'), remove_autoinc('silk_response', 'id')),
migrations.AlterField(
model_name='request',
name='prof_file',
field=models.FileField(
default='',
max_length=300,
null=True,
blank=True,
storage=silk.storage.ProfilerResultStorage(),
),
),
]
2 changes: 1 addition & 1 deletion silk/model_factory.py
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ def construct_response_model(self):
finally:
headers[header] = val
silky_response = models.Response(
request_id=self.request.id,
request=self.request,
status_code=self.response.status_code,
encoded_headers=json.dumps(headers, ensure_ascii=SilkyConfig().SILKY_JSON_ENSURE_ASCII),
body=b
Expand Down
5 changes: 1 addition & 4 deletions silk/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
import json
import random
import re
from uuid import uuid4

import sqlparse
from django.core.files.storage import get_storage_class
Expand Down Expand Up @@ -58,7 +57,6 @@ def __init__(self, d):


class Request(models.Model):
id = CharField(max_length=36, default=uuid4, primary_key=True)
Copy link
Member

Choose a reason for hiding this comment

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

The default id field for a django model is going to be a BigAutoField which is an integer field. Removing this line would change a Request model from an integer field to a charfield, which would probably break a lot of dependencies or require wiping a lot of data.

path = CharField(max_length=190, db_index=True)
query_params = TextField(blank=True, default='')
raw_body = TextField(blank=True, default='')
Expand All @@ -76,7 +74,7 @@ class Request(models.Model):
meta_num_queries = IntegerField(null=True, blank=True)
meta_time_spent_queries = FloatField(null=True, blank=True)
pyprofile = TextField(blank=True, default='')
prof_file = FileField(max_length=300, blank=True, storage=silk_storage)
prof_file = FileField(max_length=300, default='', null=True, blank=True, storage=silk_storage)

# Useful method to create shortened copies of strings without losing start and end context
# Used to ensure path and view_name don't exceed 190 characters
Expand Down Expand Up @@ -192,7 +190,6 @@ def save(self, *args, **kwargs):


class Response(models.Model):
id = CharField(max_length=36, default=uuid4, primary_key=True)
request = OneToOneField(
Request, related_name='response', db_index=True,
on_delete=models.CASCADE,
Expand Down
23 changes: 20 additions & 3 deletions silk/request_filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@
import logging
from datetime import datetime, timedelta

from django.db.models import Count, Q, Sum
from django.db.models import Count, OuterRef, Q, Subquery, Sum
from django.utils import timezone

from silk.models import SQLQuery
from silk.profiling.dynamic import _get_module
from silk.templatetags.silk_filters import _silk_date_time

Expand Down Expand Up @@ -163,7 +164,15 @@ def __str__(self):
return '#queries >= %s' % self.value

def contribute_to_query_set(self, query_set):
return query_set.annotate(num_queries=Count('queries'))
return query_set.annotate(
# This is overly complex due to Oracle not accepting group by on TextField
Copy link
Member

Choose a reason for hiding this comment

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

Is contribute_to_query_set used anywhere in the hot path (for silk to ingest data)? If it is, this change may incur a significant performance degradation to host django apps.

num_queries=Subquery(
SQLQuery.objects.filter(request_id=OuterRef('id'))
.values('request_id')
.annotate(count=Count('id'))
.values('count')[:1]
),
)


class TimeSpentOnQueriesFilter(BaseFilter):
Expand All @@ -178,7 +187,15 @@ def __str__(self):
return 'DB Time >= %s' % self.value

def contribute_to_query_set(self, query_set):
return query_set.annotate(db_time=Sum('queries__time_taken'))
return query_set.annotate(
# This is overly complex due to Oracle not accepting group by on TextField
db_time=Subquery(
SQLQuery.objects.filter(request_id=OuterRef('id'))
.values('request_id')
.annotate(sum_time_taken=Sum('time_taken'))
.values('sum_time_taken')[:1]
),
)


class OverallTimeFilter(BaseFilter):
Expand Down
22 changes: 11 additions & 11 deletions silk/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,52 +18,52 @@
path(route='', view=SummaryView.as_view(), name='summary'),
path(route='requests/', view=RequestsView.as_view(), name='requests'),
path(
route='request/<uuid:request_id>/',
route='request/<int:request_id>/',
view=RequestView.as_view(),
name='request_detail',
),
path(
route='request/<uuid:request_id>/sql/',
route='request/<int:request_id>/sql/',
view=SQLView.as_view(),
name='request_sql',
),
path(
route='request/<uuid:request_id>/sql/<int:sql_id>/',
route='request/<int:request_id>/sql/<int:sql_id>/',
view=SQLDetailView.as_view(),
name='request_sql_detail',
),
path(
route='request/<uuid:request_id>/raw/',
route='request/<int:request_id>/raw/',
view=Raw.as_view(),
name='raw',
),
path(
route='request/<uuid:request_id>/pyprofile/',
route='request/<int:request_id>/pyprofile/',
view=ProfileDownloadView.as_view(),
name='request_profile_download',
),
path(
route='request/<uuid:request_id>/json/',
route='request/<int:request_id>/json/',
view=ProfileDotView.as_view(),
name='request_profile_dot',
),
path(
route='request/<uuid:request_id>/profiling/',
route='request/<int:request_id>/profiling/',
view=ProfilingView.as_view(),
name='request_profiling',
),
path(
route='request/<uuid:request_id>/profile/<int:profile_id>/',
route='request/<int:request_id>/profile/<int:profile_id>/',
view=ProfilingDetailView.as_view(),
name='request_profile_detail',
),
path(
route='request/<uuid:request_id>/profile/<int:profile_id>/sql/',
route='request/<int:request_id>/profile/<int:profile_id>/sql/',
view=SQLView.as_view(),
name='request_and_profile_sql',
),
path(
route='request/<uuid:request_id>/profile/<int:profile_id>/sql/<int:sql_id>/',
route='request/<int:request_id>/profile/<int:profile_id>/sql/<int:sql_id>/',
view=SQLDetailView.as_view(),
name='request_and_profile_sql_detail',
),
Expand All @@ -85,7 +85,7 @@
path(route='profiling/', view=ProfilingView.as_view(), name='profiling'),
path(route='cleardb/', view=ClearDBView.as_view(), name='cleardb'),
path(
route='request/<uuid:request_id>/cprofile/',
route='request/<int:request_id>/cprofile/',
view=CProfileView.as_view(),
name='cprofile',
),
Expand Down
2 changes: 1 addition & 1 deletion silk/views/requests.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class RequestsView(View):
},
'db_time': {
'label': 'Time on queries',
'additional_query_filter': lambda x: x.annotate(db_time=Sum('queries__time_taken'))
'additional_query_filter': lambda x: x.only('pk').annotate(db_time=Sum('queries__time_taken'))
.filter(db_time__gte=0)
},
}
Expand Down
30 changes: 17 additions & 13 deletions silk/views/summary.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,44 +13,48 @@ class SummaryView(View):
filters_key = 'summary_filters'

def _avg_num_queries(self, filters):
queries__aggregate = models.Request.objects.filter(*filters).annotate(num_queries=Count('queries')).aggregate(num=Avg('num_queries'))
queries__aggregate = models.Request.objects.filter(*filters).values('view_name').annotate(num_queries=Count('queries')).aggregate(num=Avg('num_queries'))
return queries__aggregate['num']

def _avg_time_spent_on_queries(self, filters):
taken__aggregate = models.Request.objects.filter(*filters).annotate(time_spent=Sum('queries__time_taken')).aggregate(num=Avg('time_spent'))
taken__aggregate = models.Request.objects.filter(*filters).values('view_name').annotate(time_spent=Sum('queries__time_taken')).aggregate(num=Avg('time_spent'))
return taken__aggregate['num']

def _avg_overall_time(self, filters):
taken__aggregate = models.Request.objects.filter(*filters).annotate(time_spent=Sum('time_taken')).aggregate(num=Avg('time_spent'))
taken__aggregate = models.Request.objects.filter(*filters).values('view_name').annotate(time_spent=Sum('time_taken')).aggregate(num=Avg('time_spent'))
return taken__aggregate['num']

# TODO: Find a more efficient way to do this. Currently has to go to DB num. views + 1 times and is prob quite expensive
def _longest_query_by_view(self, filters):
values_list = models.Request.objects.filter(*filters).values_list("view_name").annotate(max=Max('time_taken')).filter(max__isnull=False).order_by('-max')[:5]
requests = []
for view_name, _ in values_list:
request = models.Request.objects.filter(view_name=view_name, *filters).filter(time_taken__isnull=False).order_by('-time_taken')[0]
requests.append(request)
return sorted(requests, key=lambda item: item.time_taken, reverse=True)
request = models.Request.objects.filter(view_name=view_name, time_taken__isnull=False, *filters).order_by('-time_taken').first()
if request:
requests.append(request)
return requests

def _time_spent_in_db_by_view(self, filters):
values_list = models.Request.objects.filter(*filters).values_list('view_name').annotate(t=Sum('queries__time_taken')).filter(t__gte=0).order_by('-t')[:5]
requests = []
for view, _ in values_list:
r = models.Request.objects.filter(view_name=view, *filters).annotate(t=Sum('queries__time_taken')).filter(t__isnull=False).order_by('-t')[0]
requests.append(r)
return sorted(requests, key=lambda item: item.t, reverse=True)
request = models.Request.objects.filter(view_name=view, time_taken__isnull=False, *filters)
r = request.first()
if request:
r.t = request.values('view_name').annotate(t=Sum('queries__time_taken')).filter(t__isnull=False).order_by('-t')[0]['t']
requests.append(r)
return requests

def _num_queries_by_view(self, filters):
queryset = models.Request.objects.filter(*filters).values_list('view_name').annotate(t=Count('queries')).order_by('-t')[:5]
views = [r[0] for r in queryset[:6]]
requests = []
for view in views:
try:
r = models.Request.objects.filter(view_name=view, *filters).annotate(t=Count('queries')).order_by('-t')[0]
request = models.Request.objects.filter(view_name=view, time_taken__isnull=False, *filters)
r = request.first()
if r:
r.t = request.values('view_name').annotate(t=Count('queries')).order_by('-t')[0]['t']
requests.append(r)
except IndexError:
pass
return sorted(requests, key=lambda item: item.t, reverse=True)

def _create_context(self, request):
Expand Down