jazzband / django-silk

Silky smooth profiling for Django
MIT License
4.32k stars 332 forks source link

Fix deprecation warning for get_storage_class #669

Closed albertyw closed 6 months ago

albertyw commented 11 months ago

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[bot] commented 11 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

albertyw commented 6 months ago

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 commented 6 months ago

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.