django 4.0 support #3067

mabdinur · 2021-12-14T22:37:26Z

Add support for django 4.0

Remove references to django.urls.url, this was deprecated in favor of re_path in django 3 and removed in django 4
- django 4 release notes
djangorestframework version >= 3.13 has support for django 4 compatibility release nodes
django_host version >= 5.0 has support for django 4 compatibility release nodes

Checklist

Added to the correct milestone.
Tests provided or description of manual testing performed is included in the code or PR.
Library documentation is updated.
Corp site documentation is updated (link to the PR).

mabdinur · 2021-12-15T00:18:24Z

tests/snapshots/tests.contrib.django.test_django_snapshots.test_safe_string_encoding_4x.json

Diff between snapshot generated by django 4 and django 2.2 for test_safe_string_endocing

Django 4 instrumentation appends the name of the ListView class to the resource name of a django.view span.

{ "name": "django.view", "service": "django", - "resource": "tests.contrib.django.views.view", + "resource": "tests.contrib.django.views.SafeTemplateUserList", "trace_id": 0, "span_id": 26, "parent_id": 23,

can you add this to the PR description as well as a change

This is the other way around right? We lose SafeTemplateUserList for some reason in Django 4 and get tests.contrib.django.views.view instead?

Seems odd... maybe something changed in .as_view()? We should probably figure out why this changed since it could be breaking if a user has a facet on the resource of django view spans.

brettlangdon

Remove references to dgango.urls.url

in PR description, should be django.urls.url

also, this will need a release note

riotfile.py

brettlangdon · 2021-12-15T19:21:11Z

tests/contrib/django/django1_app/settings.py

+if django.VERSION >= (4, 0, 0):
+    CACHES["redis"]["BACKEND"] = "django.core.cache.backends.redis.RedisCache"


it might be cleaner to organize as... before CACHES is defined:

redis_backend = "django_redis.cache.RedisCache" if django.VERSION >= (4, 0, 0): redis_backend = "django.core.cache.backends.redis.RedisCache" CACHES = { ... "redis": { "BACKEND": redis_backend, "LOCATION": "..." }, ... }

a little cleaner than reaching into the nested dict later

brettlangdon · 2021-12-15T19:24:03Z

tests/contrib/django/django1_app/urls.py

+if django.VERSION < (2, 0, 0):
+    from django.conf.urls import url
+else:
+    from django.urls import re_path as url


we probably shouldn't do this, don't want to confuse anyone looking at all of the urlpatterns and expecting django.conf.urls.url instead of re_path.

this is also a django 1.x app, why do we need this change?

I think I'd prefer to just import re_path and use that instead rather than mapping it to url

brettlangdon · 2021-12-15T19:24:53Z

tests/contrib/django/django_app/settings.py

 }

+if django.VERSION >= (4, 0, 0):
+    CACHES["redis"]["BACKEND"] = "django.core.cache.backends.redis.RedisCache"


brettlangdon · 2021-12-15T19:29:24Z

tests/contrib/django/django_app/urls.py

 from .. import views


+if django.VERSION < (2, 0, 0):


this shouldn't be used with django 1 right, so is this needed?

Nope but this was a mistake that line should be django.VERSION < (4, 0, 0). I'd like to keep test coverage for django.conf.urls.url for django 2

brettlangdon · 2021-12-15T19:29:32Z

tests/contrib/django/django_app/urls.py

+if django.VERSION < (2, 0, 0):
+    from django.conf.urls import url
+else:
+    from django.urls import re_path as url


same here as above

brettlangdon · 2021-12-15T19:30:44Z

tests/contrib/django_hosts/django_app/app_urls.py

+if django.VERSION < (2, 0, 0):
+    from django.conf.urls import url
+else:
+    from django.urls import re_path as url


same here, do we even test django_hosts with django 1?

Nope min version is 2.2. Good catch

brettlangdon · 2021-12-15T19:30:59Z

tests/contrib/django_hosts/django_app/extra_urls.py



+if django.VERSION < (2, 0, 0):
+    from django.conf.urls import url


brettlangdon · 2021-12-15T19:31:36Z

tests/contrib/djangorestframework/app/views.py

+if django.VERSION < (2, 0, 0):
+    from django.conf.urls import url
+else:
+    from django.urls import re_path as url


same as above, I'd prefer not to remap re_path to url

I mapped it to "handler". The reason why I made this change was to keep this pr small. In hindsight I should've prioritized readability

brettlangdon · 2021-12-15T19:32:01Z

tests/snapshots/tests.contrib.django.test_django_snapshots.test_safe_string_encoding_4x.json

can you add this to the PR description as well as a change

Co-authored-by: Brett Langdon <brett.langdon@datadoghq.com>

…o munir/django-4.0

ddtrace/tracer.py

tests/contrib/django/django_app/urls.py

tests/contrib/django_hosts/django_app/app_urls.py

tests/contrib/django_hosts/django_app/extra_urls.py

Kyle-Verhoog · 2021-12-16T15:33:14Z

tests/contrib/djangorestframework/app/views.py



+# django version < 2 does not support django.urls.re_path
+# django version > 3 does not support django.conf.urls.url


You mean >=4.0?

Also, we should be testing both when possible

Yup, it's deprecated in django 3 but it's still supported

…o munir/django-4.0

codecov-commenter · 2021-12-16T21:26:23Z

Codecov Report

Merging #3067 (4e2e41c) into master (d6d1705) will decrease coverage by 0.01%.
The diff coverage is 83.16%.

@@            Coverage Diff             @@
##           master    #3067      +/-   ##
==========================================
- Coverage   84.56%   84.54%   -0.02%     
==========================================
  Files         638      638              
  Lines       46069    46262     +193     
==========================================
+ Hits        38957    39114     +157     
- Misses       7112     7148      +36

Impacted Files	Coverage Δ
ddtrace/propagation/utils.py	`0.00% <0.00%> (-100.00%)`	⬇️
ddtrace/tracer.py	`86.41% <0.00%> (ø)`
...ests/contrib/django_hosts/django_app/extra_urls.py	`0.00% <0.00%> (ø)`
tests/profiling/collector/test_stack.py	`0.00% <0.00%> (ø)`
tests/profiling/collector/test_task.py	`0.00% <0.00%> (ø)`
ddtrace/sampler.py	`94.66% <84.37%> (-2.04%)`	⬇️
ddtrace/contrib/django/patch.py	`83.66% <100.00%> (+0.05%)`	⬆️
ddtrace/contrib/django/utils.py	`89.26% <100.00%> (ø)`
ddtrace/contrib/wsgi/wsgi.py	`89.15% <100.00%> (ø)`
ddtrace/internal/compat.py	`95.37% <100.00%> (+0.97%)`	⬆️
... and 19 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 921640e...4e2e41c. Read the comment docs.

Kyle-Verhoog

Looks good! Just a question about the subtle change to the view resource name and the docs. Otherwise lgtm

Kyle-Verhoog · 2021-12-16T22:43:57Z

releasenotes/notes/django-4-support-eadd627959b9344e.yaml

+---
+features:
+  - |
+    Add django 4.0 support.


Also have to update integrations.rst

Kyle-Verhoog · 2021-12-16T22:55:38Z

tests/snapshots/tests.contrib.django.test_django_snapshots.test_safe_string_encoding_4x.json

This is the other way around right? We lose SafeTemplateUserList for some reason in Django 4 and get tests.contrib.django.views.view instead?

Seems odd... maybe something changed in .as_view()? We should probably figure out why this changed since it could be breaking if a user has a facet on the resource of django view spans.

tests/contrib/django/django_app/extra_urls.py

Co-authored-by: Brett Langdon <brett.langdon@datadoghq.com>

…iew(), in django 4 View.__name__ returns 'view' and not the name of the sub class of django.views.generic.View

…o munir/django-4.0

mabdinur · 2021-12-20T22:04:10Z

ddtrace/contrib/django/patch.py

        log.debug("Failed to instrument Django view %r", instance, exc_info=True)
    view = func(*args, **kwargs)
-    return wrapt.FunctionWrapper(view, traced_func(django, "django.view", resource=func_name(view)))
+    return wrapt.FunctionWrapper(view, traced_func(django, "django.view", resource=func_name(instance)))


In the func_name method view.__class__.__name__ returns "view" in django 4.0 instead of the name of the sub class (ex. tests.contrib.django.views.SomeViewClass). According to this comment, django 4.0 supports accessing the name of a View using theview_class attribute. Instead of this approach this change access the class name using an instance of a view and not view method. This approach is backwards compatible and can be used with the existing func_name utility.

Comments have been addressed

django 4.0 support

293127f

mabdinur requested a review from a team as a code owner December 14, 2021 22:37

mabdinur added the changelog/no-changelog A changelog entry is not required for this PR. label Dec 14, 2021

mabdinur added 2 commits December 14, 2021 17:42

isort

fce8c32

erun django4 tests for djangorestframework and django_hosts

1fe7ed0

mabdinur force-pushed the munir/django-4.0 branch from e684028 to 543ca95 Compare December 15, 2021 00:08

mabdinur commented Dec 15, 2021

View reviewed changes

mabdinur force-pushed the munir/django-4.0 branch from 543ca95 to 62e013b Compare December 15, 2021 00:27

add back support for django.version < 2

29f9cf6

mabdinur force-pushed the munir/django-4.0 branch from 62e013b to 29f9cf6 Compare December 15, 2021 00:52

mabdinur added 3 commits December 15, 2021 09:11

fix for django_app and djangorestframework

43812cd

djangorestframwork version > 3.13 supports django 4

9e8fe45

Merge branch 'master' into munir/django-4.0

7765f79

brettlangdon removed the changelog/no-changelog A changelog entry is not required for this PR. label Dec 15, 2021

brettlangdon requested changes Dec 15, 2021

View reviewed changes

mabdinur and others added 7 commits December 16, 2021 09:28

Update riotfile.py

e03d3f0

Co-authored-by: Brett Langdon <brett.langdon@datadoghq.com>

addresses pr comments

3c7f9fb

Merge branch 'munir/django-4.0' of github.com:DataDog/dd-trace-py int…

3eac137

…o munir/django-4.0

Update urls.py

ed8f934

Update settings.py

3a8944f

mypy fix

c7089b1

Merge branch 'munir/django-4.0' of github.com:DataDog/dd-trace-py int…

6766ddf

…o munir/django-4.0

mabdinur commented Dec 16, 2021

View reviewed changes

ddtrace/tracer.py Outdated Show resolved Hide resolved

Kyle-Verhoog requested changes Dec 16, 2021

View reviewed changes

keep django.conf.urls.url() for django 3

14244f4

mabdinur force-pushed the munir/django-4.0 branch 4 times, most recently from e90d4b5 to f39707e Compare December 16, 2021 18:13

add release note

dab050f

mabdinur requested review from Kyle-Verhoog and brettlangdon December 16, 2021 19:04

mabdinur force-pushed the munir/django-4.0 branch 2 times, most recently from efb7fc7 to d3d2d09 Compare December 16, 2021 19:08

Merge branch 'munir/django-4.0' of github.com:DataDog/dd-trace-py int…

76ebe12

…o munir/django-4.0

mabdinur force-pushed the munir/django-4.0 branch from d3d2d09 to 76ebe12 Compare December 16, 2021 19:11

mabdinur added 2 commits December 16, 2021 14:12

Update extra_urls.py

f3bf356

Merge branch 'master' into munir/django-4.0

74a4f9d

Merge branch 'master' into munir/django-4.0

586b300

Kyle-Verhoog previously requested changes Dec 16, 2021

View reviewed changes

brettlangdon reviewed Dec 16, 2021

View reviewed changes

tests/contrib/django/django_app/extra_urls.py Show resolved Hide resolved

mabdinur and others added 8 commits December 17, 2021 09:12

Update tests/contrib/django/django_app/extra_urls.py

6417a72

Co-authored-by: Brett Langdon <brett.langdon@datadoghq.com>

fix snapshot and update index.rst to show django 4.0 support

71bc98f

get class name from instance instead of the return value of View.as_v…

ef84de0

…iew(), in django 4 View.__name__ returns 'view' and not the name of the sub class of django.views.generic.View

Merge branch 'master' into munir/django-4.0

90ac1b3

reset test_safe_string_encoding snapshot

f1b91ea

Merge branch 'munir/django-4.0' of github.com:DataDog/dd-trace-py int…

2118f11

…o munir/django-4.0

Merge branch 'munir/django-4.0' of github.com:DataDog/dd-trace-py int…

ec3143d

…o munir/django-4.0

Merge branch 'munir/django-4.0' of github.com:DataDog/dd-trace-py int…

4e2e41c

…o munir/django-4.0

mabdinur commented Dec 20, 2021

View reviewed changes

mabdinur requested review from Kyle-Verhoog and brettlangdon December 20, 2021 22:33

brettlangdon approved these changes Dec 21, 2021

View reviewed changes

brettlangdon merged commit 23827ed into master Dec 21, 2021

brettlangdon deleted the munir/django-4.0 branch December 21, 2021 15:58

		if django.VERSION >= (4, 0, 0):
		CACHES["redis"]["BACKEND"] = "django.core.cache.backends.redis.RedisCache"



		if django.VERSION < (2, 0, 0):
		from django.conf.urls import url



		# django version < 2 does not support django.urls.re_path
		# django version > 3 does not support django.conf.urls.url

django 4.0 support #3067

django 4.0 support #3067

Uh oh!

Conversation

mabdinur commented Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

mabdinur Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brettlangdon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mabdinur Dec 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Dec 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Kyle-Verhoog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mabdinur Dec 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

mabdinur commented Dec 14, 2021 •

edited

Loading

mabdinur Dec 15, 2021 •

edited

Loading

mabdinur Dec 16, 2021 •

edited

Loading

codecov-commenter commented Dec 16, 2021 •

edited

Loading

mabdinur Dec 20, 2021 •

edited

Loading