Skip to content

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

Merged

Conversation

narsaynorath
Copy link
Member

@narsaynorath narsaynorath commented Apr 28, 2023

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 on method. Could there be other places I need to update to use http.method?

SDK PR: getsentry/sentry-javascript#7990

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Apr 28, 2023
@codecov
Copy link

codecov bot commented Apr 28, 2023

Codecov Report

Merging #48155 (6c7e23b) into master (9db2a33) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

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              
Impacted Files Coverage Δ
...ormance_issues/detectors/large_payload_detector.py 98.07% <ø> (ø)
...s/detectors/render_blocking_asset_span_detector.py 98.46% <ø> (ø)
...ce_issues/detectors/uncompressed_asset_detector.py 93.44% <ø> (ø)
...rmance_issues/detectors/consecutive_db_detector.py 99.13% <100.00%> (ø)

... and 141 files with indirect coverage changes

@narsaynorath narsaynorath marked this pull request as ready for review April 28, 2023 16:54
@narsaynorath narsaynorath requested a review from a team April 28, 2023 16:55
Co-authored-by: Abhijeet Prasad <aprasad@sentry.io>
Copy link
Member

@gggritso gggritso left a 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 ""
Copy link
Member

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.

narsaynorath added a commit that referenced this pull request May 2, 2023
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
@narsaynorath narsaynorath merged commit c02eb6d into master May 3, 2023
@narsaynorath narsaynorath deleted the nar/feat/perf-issues-change-method-to-http-method branch May 3, 2023 13:27
@github-actions github-actions bot locked and limited conversation to collaborators May 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants