chore(deps): Upgrade to Django 4.2 #18653

webjunkie · 2023-11-15T15:24:06Z

Problem

Upgrade to latest Django version.

Changes

upgrading to Django 4.1 seems pretty straightforward, no major issues encountered, done in chore(deps): Upgrade Django 4.1 #20006
Django 4.2: Here Django started supporting psycopg==3.x which was recently added to the dependencies coincidentally. Most of the problems come from this. Otherwise also no blockers it seems.

Issues left:

~~pg_sleep(1) to make tests simulate slow DB is somehow not applied~~ this issue vanished 🤷🏻
~~new duplicate key value violates unique constraint "posthog_organization_slug_key" errors come up~~ this as well
SQL commenting has stopped working -> Postgres only, decided to remove it for now

How did you test this code?

Among https://docs.djangoproject.com/en/4.2/releases/4.0/#features-removed-in-4-0

We need to be on >= 3.1.8 Locally there is an additional problem that somehow psycopg2 seemingly overshadows psycopg, making it appear that 3.1 works. Had to install pip install "psycopg[binary,pool]==3.1.2" to recreate the problem.

We use custom SQL that somehow doesn't get formatted in the right way using server or client side cursors.

github-actions · 2023-11-16T12:01:50Z

Size Change: -2.05 kB (0%)

Total Size: 2.01 MB

Filename	Size	Change
`frontend/dist/toolbar.js`	2.01 MB	-2.05 kB (0%)

_{compressed-size-action}

Because full_clean validates since Django 4.1, see https://docs.djangoproject.com/en/4.2/releases/4.1/#validation-of-constraints

Come up as error: Unused "type: ignore" comment

webjunkie · 2023-11-20T06:44:35Z

@pauldambra We are green on Django 4.1 🚀
With Django 4.2 I have two issues left that so far don't make sense to me.

pauldambra

very quick scan, all looks good apart from the sql commenting.

I'll check it out and run it later on

pauldambra · 2023-11-20T08:48:26Z

ee/api/test/__snapshots__/test_organization_resource_access.ambr

@@ -48,7 +48,7 @@
         "posthog_organization"."available_features"
  FROM "posthog_organization"
  WHERE "posthog_organization"."id" = '00000000-0000-0000-0000-000000000000'::uuid
-  LIMIT 21 /*controller='organization_resource_access-list',route='api/organizations/%28%3FP%3Cparent_lookup_organization_id%3E%5B%5E/.%5D%2B%29/resource_access/%3F%24'*/


I don't remember where they're set but it looks like the useful SQL commenting has stopped working

reasonably sure it's the sql commenter: https://github.com/PostHog/posthog/blob/master/posthog/settings/web.py#L90

It seems Google gave this sqlcommenter to opentelemetry and it was not updated in a long time.

This might be where it ended up:
https://opentelemetry-python-contrib.readthedocs.io/en/latest/instrumentation/django/django.html#sqlcommenter

However, then there's this about supporting psycopg3, which I think might be the problem we have:
open-telemetry/opentelemetry-python-contrib#1751

How much do we need it anyways?

oh dang it!

It's pretty useful for knowing where the request is coming from, specially when looking in, say, pganalyze for most popular sources of sql requests.

But, we do this manually for clickhouse ourselves, so wonder if there's an easy / small-scoped way to just do it ourselves

I've investigated further and also tried to use the middleware from opentelemetry that has some modifications as it seems.
Unfortunately, then immediately the following issue comes up 🥴
open-telemetry/opentelemetry-python-contrib#1554

It seems there are some differences between how the queries are handled and how the wrappers are executed. This would also explain why the pg_sleep wrapper has no effect anymore.

I'd say for now I'll make another PR just to upgrade to Django 4.1, where I had all CI green. Or does anyone have any suggestions?

Yep... a step on the road that works 🚀

webjunkie · 2023-11-22T20:02:30Z

I've created #18830 for Django 4.1 and would propose to leave this one here open to track issues for 4.2

posthog-bot · 2023-11-30T07:31:11Z

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

posthog-bot · 2023-12-08T07:31:05Z

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

Because not maintained anymore

This reverts commit c2a7ef8.

webjunkie · 2024-04-03T14:28:06Z

I merged master into this PR to update it and fixed some more issues, others meanwhile went away.
It's now looking ready.

One issue is that mypy shows errors in CI that I don't see locally. @pauldambra or @neilkakkar can someone check this locally as well and possibly just update the baseline?

neilkakkar · 2024-04-04T10:53:24Z

posthog/api/test/test_decide.py

@@ -3962,7 +3962,7 @@ def test_feature_flags_v3_consistent_flags(self, mock_is_connected):
        # make sure caches are populated
        response = self._post_decide()

-        with self.assertNumQueries(5, using="replica"), self.assertNumQueries(0, using="default"):
+        with self.assertNumQueries(9, using="replica"), self.assertNumQueries(0, using="default"):


weird, how are there so many extra queries for flags, would you mind sharing the output here? I can look into this soon-ish as well

oh its this

I'll do a once-over anyway to confirm they're legit

Yeah the increases should only be from this, the count changes do match that.

neilkakkar · 2024-04-04T11:20:05Z

posthog/models/feature_flag/flag_matching.py

@@ -790,7 +790,8 @@ def get_all_feature_flags(
                    distinct_ids = [distinct_id, str(hash_key_override)]
                    query = """
                        WITH target_person_ids AS (
-                            SELECT team_id, person_id FROM posthog_persondistinctid WHERE team_id = %(team_id)s AND distinct_id IN %(distinct_ids)s
+                            SELECT team_id, person_id FROM posthog_persondistinctid WHERE team_id = %(team_id)s AND
+                            distinct_id = ANY(%(distinct_ids)s)


this is in preparation for psycopg3 update?

This is the psycopg3 update as with Django 4.2 it's used actively.

hmm, oh I see, I was looking at docs for psycopg3.2.0, https://www.psycopg.org/psycopg3/docs/basic/from_pg2.html#server-side-binding - this is what I expect will cause the pg_sleep() tests to fail because it sends a SET command with parameter binding

but we can leave this be for when its actually released :P

Also had a look at the psycopg2 usage still in the app, we can remove it in a follow up indeed, no need to do it right now. 👍

neilkakkar

Great job!

When we roll-out we should pause deploys and test this in dev / test in a canary deploy first!

webjunkie and others added 8 commits November 15, 2023 16:10

Upgrade dependencies

78d795a

Fix middleware error

60c4351

Among https://docs.djangoproject.com/en/4.2/releases/4.0/#features-removed-in-4-0

Upgrade psycopg

cb6b389

We need to be on >= 3.1.8 Locally there is an additional problem that somehow psycopg2 seemingly overshadows psycopg, making it appear that 3.1 works. Had to install pip install "psycopg[binary,pool]==3.1.2" to recreate the problem.

Go to Django 4.1 because of problems with psycopg3

f764d13

We use custom SQL that somehow doesn't get formatted in the right way using server or client side cursors.

Update query snapshots

197cdc8

Update query snapshots

1513680

Update query snapshots

eabba3b

Update query snapshots

d0d6ee6

webjunkie and others added 12 commits November 16, 2023 14:13

Switch TaggedItem tests to assert ValidationError

fd8212d

Because full_clean validates since Django 4.1, see https://docs.djangoproject.com/en/4.2/releases/4.1/#validation-of-constraints

Remove type: ignore comments

bb8d7a5

Come up as error: Unused "type: ignore" comment

Update query snapshots

faad0ca

Figure out psycopg problem and try Django 4.2 again

c474c03

Update query snapshots

a1731eb

Fix other IN errors

15fe236

Fix getting status

9c9bcb4

Fix psycopg3 issues

677d38a

Fix psycopg issues

9cc26d4

Update query snapshots

1dfef28

Update query snapshots

3170b03

Update query snapshots

ccba439

pauldambra reviewed Nov 20, 2023

View reviewed changes

webjunkie mentioned this pull request Nov 22, 2023

chore(deps): Upgrade to Django 4.1 #18830

Merged

webjunkie changed the title ~~chore(deps): Upgrade Django~~ chore(deps): Upgrade to Django 4.2 Nov 22, 2023

posthog-bot added the stale label Nov 30, 2023

webjunkie removed the stale label Nov 30, 2023

webjunkie added 6 commits April 2, 2024 15:50

Fix queries

e9db3ac

Fix query

8e95b93

Revert

4fc7873

Update requirements.in

2724b91

Remove restore-virtualenv

c2a7ef8

Because not maintained anymore

Revert "Remove restore-virtualenv"

d22a44e

This reverts commit c2a7ef8.

webjunkie marked this pull request as ready for review April 3, 2024 14:28

neilkakkar and others added 6 commits April 3, 2024 15:44

Merge branch 'master' of github.com:PostHog/posthog into chore/Django-4

53b965b

mypy

8c0d9c4

Adjust num queries

70e54d3

Adjust num queries

b91b5e5

Adjust num queries

e1b37a5

Update query snapshots

f0d9fdc

neilkakkar reviewed Apr 4, 2024

View reviewed changes

neilkakkar approved these changes Apr 5, 2024

View reviewed changes

webjunkie added the deploy label Apr 8, 2024

Merge branch 'master' into chore/Django-4

007d96b

github-actions bot temporarily deployed to pr-chore/Django-4 April 8, 2024 08:58 Destroyed

webjunkie requested a review from benjackwhite April 8, 2024 09:17

Add to updated_fields

3f2bf63

github-actions bot temporarily deployed to pr-chore/Django-4 April 8, 2024 09:38 Destroyed

frankh added canary and removed waiting Prevents stale-bot from marking the PR as stale. labels Apr 16, 2024

Merge branch 'master' into chore/Django-4

f0cfd29

github-actions bot temporarily deployed to pr-chore/Django-4 April 16, 2024 09:07 Destroyed

frankh merged commit dc0cf33 into master Apr 16, 2024
97 checks passed

frankh deleted the chore/Django-4 branch April 16, 2024 09:53

webjunkie mentioned this pull request Apr 16, 2024

fix: Make sure event names are string #21568

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(deps): Upgrade to Django 4.2 #18653

chore(deps): Upgrade to Django 4.2 #18653

webjunkie commented Nov 15, 2023 •

edited

Loading

github-actions bot commented Nov 16, 2023 •

edited

Loading

webjunkie commented Nov 20, 2023

pauldambra left a comment

pauldambra Nov 20, 2023

neilkakkar Nov 20, 2023

webjunkie Nov 20, 2023

neilkakkar Nov 20, 2023

webjunkie Nov 21, 2023

pauldambra Nov 21, 2023

webjunkie commented Nov 22, 2023

posthog-bot commented Nov 30, 2023

posthog-bot commented Dec 8, 2023

webjunkie commented Apr 3, 2024

neilkakkar Apr 4, 2024

neilkakkar Apr 4, 2024

webjunkie Apr 4, 2024

neilkakkar Apr 4, 2024

webjunkie Apr 4, 2024

neilkakkar Apr 5, 2024

neilkakkar left a comment

chore(deps): Upgrade to Django 4.2 #18653

chore(deps): Upgrade to Django 4.2 #18653

Conversation

webjunkie commented Nov 15, 2023 • edited Loading

Problem

Changes

How did you test this code?

github-actions bot commented Nov 16, 2023 • edited Loading

webjunkie commented Nov 20, 2023

pauldambra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webjunkie commented Nov 22, 2023

posthog-bot commented Nov 30, 2023

posthog-bot commented Dec 8, 2023

webjunkie commented Apr 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neilkakkar left a comment

Choose a reason for hiding this comment

webjunkie commented Nov 15, 2023 •

edited

Loading

github-actions bot commented Nov 16, 2023 •

edited

Loading