Conversation
for more information, see https://pre-commit.ci
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #619 +/- ##
==========================================
- Coverage 86.31% 86.18% -0.13%
==========================================
Files 52 53 +1
Lines 2090 2114 +24
==========================================
+ Hits 1804 1822 +18
- Misses 286 292 +6 ☔ View full report in Codecov by Sentry. |
| migrations.RemoveField( | ||
| model_name='response', | ||
| name='request' | ||
| ), | ||
| migrations.RemoveField( | ||
| model_name='sqlquery', | ||
| name='request' | ||
| ), | ||
| migrations.RemoveField( | ||
| model_name='profile', | ||
| name='request' | ||
| ), |
There was a problem hiding this comment.
Is this valid? I don't see this appearing in silk/models.py.
| @@ -0,0 +1,101 @@ | |||
| # Generated by Django 1.9.7 on 2018-01-10 14:14 | |||
There was a problem hiding this comment.
This is a very old version of Django. Should this file be regenerated using a supported version of django?
There was a problem hiding this comment.
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.
|
|
||
|
|
||
| class Request(models.Model): | ||
| id = CharField(max_length=36, default=uuid4, primary_key=True) |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
3 participants
Now I would like to discuss dropping the uuid on Request mode, is everybody okay with that?