-
Notifications
You must be signed in to change notification settings - Fork 569
feat(django): Instrument database commits #5100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5100 +/- ##
==========================================
- Coverage 84.01% 83.98% -0.03%
==========================================
Files 180 180
Lines 18004 18179 +175
Branches 3200 3229 +29
==========================================
+ Hits 15126 15268 +142
- Misses 1906 1923 +17
- Partials 972 988 +16
|
| user = User.objects.db_manager("postgres").create_user( | ||
| username="user1", | ||
| ) | ||
| return HttpResponse("ok {}".format(user)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Duplicate username causes IntegrityError on repeated calls
Both postgres_insert_orm_no_autocommit and postgres_insert_orm_atomic views create users with the hardcoded username "user1". When these endpoints are called more than once (either the same endpoint twice or both endpoints once), Django raises an IntegrityError because usernames must be unique. This prevents manual testing as described in the PR description and makes the endpoints non-idempotent. The views need to either generate unique usernames, delete existing users first, or handle the integrity constraint violation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests should be as deterministic as possible. The sample is intended to be very minimal.
Description
Create spans for SQL commits issued when Django calls
commit()on a PEP-249 database connection.Patch the
BaseDatabaseWrapper._commitmethod, the same method in which debug statements were added in django/django@798e38c.Commit spans are generated for
transaction.atomicblocks and for manualtransction.commit()calls when auto-commit is disabled. Tests cover both cases, for SQLite and PostgreSQL, respectively.Trying for yourself
The easiest way to try the new spans out for yourself is to bootstrap off the Django sample in our tests. To get the sample running independently, modify the initialization in
tests/integrations/django/myapp/settings.pyand make the PostgreSQL database name deterministic:
DATABASES["postgres"] = { "ENGINE": db_engine, "HOST": os.environ.get("SENTRY_PYTHON_TEST_POSTGRES_HOST", "localhost"), "PORT": int(os.environ.get("SENTRY_PYTHON_TEST_POSTGRES_PORT", "5432")), "USER": os.environ.get("SENTRY_PYTHON_TEST_POSTGRES_USER", "postgres"), "PASSWORD": os.environ.get("SENTRY_PYTHON_TEST_POSTGRES_PASSWORD", "sentry"), "NAME": os.environ.get( - "SENTRY_PYTHON_TEST_POSTGRES_NAME", f"myapp_db_{os.getpid()}" + "SENTRY_PYTHON_TEST_POSTGRES_NAME", f"myapp_db" ), }To start a PostgreSQL instance, you can create the server in a container as follows:
Next, populate the PostgreSQL database and run the Django server with the series of commands:
You can trigger an SQL transaction by hitting one of the
postgres_insert*endpoints. For instance, accesshttp://localhost:8000/postgres-insert-no-autocommit.Issues
Reminders
tox -e linters.feat:,fix:,ref:,meta:)