-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(perf-issues): Accept http.method as key for span data #48155
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
feat(perf-issues): Accept http.method as key for span data #48155
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #48155 +/- ##
==========================================
- Coverage 80.89% 80.89% -0.01%
==========================================
Files 4771 4776 +5
Lines 201728 201831 +103
Branches 11542 11542
==========================================
+ Hits 163192 163275 +83
- Misses 38281 38301 +20
Partials 255 255
|
src/sentry/utils/performance_issues/detectors/consecutive_db_detector.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Abhijeet Prasad <aprasad@sentry.io>
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.
I think this should be fine! The other detectors that rely on the HTTP method are ConsecutiveHTTPSpanDetector
and NPlusOneAPICallsDetector
, both of which use the span's description to get the method, instead of span data 👍🏻
@@ -216,7 +216,7 @@ def is_event_eligible(cls, event, project: Project = None) -> bool: | |||
|
|||
if request: | |||
url = request.get("url", "") or "" | |||
method = request.get("method", "") or "" | |||
method = request.get("http.method", "") or request.get("method", "") or "" |
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.
I'd leave a TODO w/ a cleanup note here that method
can be removed likely in 6 month+ when the sdk has been more adopted.
I couldn't find anywhere in the frontend where we parsed out `method` from spans, so I've only updated the span data in tests. Backend PR: #48155
Span data is renaming
method
->http.method
so this PR checks for both in the backend. I was only able to find one perf issue that relied onmethod
. Could there be other places I need to update to usehttp.method
?SDK PR: getsentry/sentry-javascript#7990