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

Add query source to DB spans #2521

Merged
merged 26 commits into from
Nov 24, 2023
Merged

Add query source to DB spans #2521

merged 26 commits into from
Nov 24, 2023

Conversation

antonpirker
Copy link
Member

@antonpirker antonpirker commented Nov 21, 2023

Adding OTel compatible information to database spans that show the code location of the query.
Refs getsentry/team-sdks#40

This has to be

  • "opt in" and
  • only add the location where the query takes longer than 100ms to run
  • working with SQLAlchemy
  • working with Django
  • working with asyncpg

Documentation PR: getsentry/sentry-docs#8558

In the product this will then be displayed with something like this:

query-source

@antonpirker antonpirker changed the title Antonpirker/query source Add query source to DB spans Nov 21, 2023
@antonpirker antonpirker self-assigned this Nov 21, 2023
@AbhiPrasad
Copy link
Member

Is it possible to tell what the overhead of this is going to be?

@antonpirker
Copy link
Member Author

Is it possible to tell what the overhead of this is going to be?

Hard to tell. I am "only" walking the stack until i find the right frame and then add the data to the span. But maybe we should do some load testing.

(it is opt-in, so we will not break anything out of the box)

@antonpirker
Copy link
Member Author

@AbhiPrasad Did some load testing on an FastAPI example app running 50 concurrent users doing around 85 requests per second:

Without attaching the query source:

without-query-source-50users-fastapi

With attaching the query source:

with-query-source-50users-fastapi

The only difference I can see is the 99percentile is a bit slower with attaching query sources (but those are probably some outliers, the the max column)

So I think we are good here.

@antonpirker
Copy link
Member Author

And here the event as JSON: python-with-query-source.json

@antonpirker
Copy link
Member Author

One thing to mention: When finding the right frame to attach as query source I am omitting external sources (stuff that is in site-packages and dist-packages)

This works well if someone has for example Django and does queries using Djangos ORM because the queries are done in the users code. But if they have Django and DjangoRestFramework the place in the users code that defines the query does not show up in the stack trace and I can not add the query source.

Which is a bummer, but I think kind of ok, because if they have DjangoRestFramework the transaction name will lead them to their serializer and then they know where to look for the query/queries.

@antonpirker antonpirker marked this pull request as ready for review November 22, 2023 13:29
Copy link
Contributor

@sentrivana sentrivana left a comment

Choose a reason for hiding this comment

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

LGTM, left some suggestions

sentry_sdk/tracing_utils.py Show resolved Hide resolved
sentry_sdk/consts.py Outdated Show resolved Hide resolved
@antonpirker antonpirker enabled auto-merge (squash) November 24, 2023 08:32
auto-merge was automatically disabled November 24, 2023 09:13

Pull Request is not mergeable

@antonpirker antonpirker enabled auto-merge (squash) November 24, 2023 09:42
@antonpirker antonpirker merged commit f6325f7 into master Nov 24, 2023
467 of 468 checks passed
@antonpirker antonpirker deleted the antonpirker/query-source branch November 24, 2023 09:43
gggritso added a commit to getsentry/sentry that referenced this pull request Nov 29, 2023
Dogfooding the query source feature. Conservatively sets the threshold
to 500ms for now. If you're wondering about performance overhead for
this, check out info in
getsentry/sentry-python#2521

---------

Co-authored-by: Anton Pirker <[email protected]>
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.

4 participants