Fix deprecation warning for get_storage_class

[albertyw]

This PR should fix the deprecation warning for get_storage_class by trying to import and use the storages class instead. However, the latter is not an exact replacement for the former, requiring a different configuration option to be provided under the STORAGE instead of the SILKY_STORAGE_CLASS setting. This makes this PR a forwards incompatible change requiring a minor semver bump.

Fixes #656

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f24f055) 86.58% compared to head (b24978d) 86.65%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #669      +/-   ##
==========================================
+ Coverage   86.58%   86.65%   +0.06%     
==========================================
  Files          52       52              
  Lines        2102     2113      +11     
==========================================
+ Hits         1820     1831      +11     
  Misses        282      282              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[50-Course]

👍🏼

[50-Course]

Thanks for this PR! It's great to see that the deprecation warning for get_storage_class being addressed in earnest.

I noticed that the PR introduces a forwards incompatibility by requiring a different configuration option (STORAGE) to be used instead of SILKY_STORAGE_CLASS - a major change that will require users to update their configuration files.

I'm also curious about how the storages class compares to the old get_storage_class approach in terms of functionality and performance.

Overall, I think this is a good PR that addresses an important issue.

[albertyw]

The STORAGES config is defined by the Django framework itself and its predecessor get_storage_class is removed starting Django 5.1 so there's not much of an alternative to replacing get_storage_class. All users, whether using django-silk or not will be eventually forced to abandon get_storage_class and therefore SILKY_STORAGE_CLASS and set up a STORAGES config. Nevertheless, this change is a no-op for users using the default silk.storage.ProfilerResultStorage due to the try/except falling back to that storage if storages['SILKY_STORAGE'] is not specified.

In terms of performance, comparing the new storages object against the get_storage_class implementation, it seems that the former would be more performant due to it caching storages. Since django-silk is initializing silk_storage once on initialization rather than in the request-serving path, I don't think performance is a significant consideration though. Either storages configuration gives equivalent functionality; the change only affects configuration format.

[50-Course]

The STORAGES config is defined by the Django framework itself and its predecessor get_storage_class is removed starting Django 5.1 so there's not much of an alternative to replacing get_storage_class. All users, whether using django-silk or not will be eventually forced to abandon get_storage_class and therefore SILKY_STORAGE_CLASS and set up a STORAGES config. Nevertheless, this change is a no-op for users using the default silk.storage.ProfilerResultStorage due to the try/except falling back to that storage if storages['SILKY_STORAGE'] is not specified.

In terms of performance, comparing the new storages object against the get_storage_class implementation, it seems that the former would be more performant due to it caching storages. Since django-silk is initializing silk_storage once on initialization rather than in the request-serving path, I don't think performance is a significant consideration though. Either storages configuration gives equivalent functionality; the change only affects configuration format.

Thanks for the explanation. Coming back to this in a few.