Skip to content
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

record span and breadcrumb when Django opens db connection #1250

Merged

Conversation

chdsbd
Copy link
Contributor

@chdsbd chdsbd commented Nov 15, 2021

Here's a screenshot of a trace where Django opens a new connection

Screen Shot 2021-11-14 at 9 04 39 PM

fixes #1249

Comment on lines +522 to +526
with capture_internal_exceptions():
hub.add_breadcrumb(message="connect", category="query")

with hub.start_span(op="db", description="connect"):
return real_connect(self)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what the message or description should be

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think these are fine, we can iterate on them in the future if people give feedback


from tests.integrations.django.myapp.wsgi import application

# Hack to prevent from experimental feature introduced in version `4.3.0` in `pytest-django` that
# requires explicit database allow from failing the test
pytest_mark_django_db_decorator = pytest.mark.django_db
pytest_mark_django_db_decorator = partial(pytest.mark.django_db)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated this so I could pass transaction=False

@sl0thentr0py sl0thentr0py requested review from a team, rhcarvalho and AbhiPrasad and removed request for a team November 15, 2021 13:43
@AbhiPrasad
Copy link
Member

Will wait for tests match against the django matrix to see if it works with all versions, but LGTM from an initial glance. Thank you for your contribution!

@AbhiPrasad
Copy link
Member

The test failures are due to sanic, which we fixed. Mind rebasing on master?

@chdsbd
Copy link
Contributor Author

chdsbd commented Nov 16, 2021

@AbhiPrasad I updated the branch

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seem like there some test failures, mind taking a look?

platform linux -- Python 3.8.12, pytest-6.2.5, py-1.11.0, pluggy-0.13.1
cachedir: .tox/py3.8-django-3.2/.pytest_cache
django: settings: tests.integrations.django.myapp.settings (from ini)
rootdir: /home/runner/work/sentry-python/sentry-python, configfile: pytest.ini
plugins: asyncio-0.16.0, hypothesis-6.24.5, localserver-0.5.0, forked-1.1.3, django-4.4.0, cov-2.8.1
collected 51 items
........F.
tests/integrations/django/test_basic.py ............Xx................   [ 54%]
tests/integrations/django/test_transactions.py .....                     [ 64%]
tests/integrations/django/asgi/test_asgi.py .......

=================================== FAILURES ===================================
__________________________ test_django_connect_trace ___________________________
tests/integrations/django/test_basic.py:428: in test_django_connect_trace
    assert res.status_code == 200
E   AttributeError: 'tuple' object has no attribute 'status_code'

@chdsbd
Copy link
Contributor Author

chdsbd commented Nov 16, 2021

@AbhiPrasad I think I finally fixed it. I was incorrectly using the test client like the Django Rest client.

@AbhiPrasad
Copy link
Member

AbhiPrasad commented Nov 16, 2021

Awesome, thanks for sticking with this! Will merge after I double check one thing.

@AbhiPrasad AbhiPrasad merged commit df542a2 into getsentry:master Nov 17, 2021
@chdsbd chdsbd deleted the chris/trace-opening-db-connections branch November 17, 2021 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Instrument new database connections in Django
2 participants