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

Configure HTTP methods to capture in ASGI middleware and frameworks #3533

Conversation

antonpirker
Copy link
Member

@antonpirker antonpirker commented Sep 13, 2024

  • Do not capture transactions for OPTIONS and HEAD HTTP methods by default.
  • Make it possible with an http_methods_to_capture config option for Starlette and FastAPI, to specify what HTTP methods to capture.

Fixes #3529

Docs PR:
getsentry/sentry-docs#11349

@antonpirker antonpirker changed the title Configure http methods to capture in asgi middleware Configure HTTP methods to capture in ASGI middleware and frameworks Sep 13, 2024
Copy link

codecov bot commented Sep 13, 2024

Codecov Report

Attention: Patch coverage is 86.36364% with 3 lines in your changes missing coverage. Please review.

Project coverage is 84.29%. Comparing base (205591e) to head (8e63cc7).
Report is 10 commits behind head on antonpirker/http_methods_to_capture.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
sentry_sdk/integrations/asgi.py 84.21% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@                           Coverage Diff                           @@
##           antonpirker/http_methods_to_capture    #3533      +/-   ##
=======================================================================
- Coverage                                84.30%   84.29%   -0.01%     
=======================================================================
  Files                                      133      133              
  Lines                                    13902    13914      +12     
  Branches                                  2933     2936       +3     
=======================================================================
+ Hits                                     11720    11729       +9     
  Misses                                    1444     1444              
- Partials                                   738      741       +3     
Files with missing lines Coverage Δ
sentry_sdk/integrations/_asgi_common.py 94.54% <100.00%> (+0.10%) ⬆️
sentry_sdk/integrations/starlette.py 84.88% <100.00%> (+0.08%) ⬆️
sentry_sdk/integrations/asgi.py 90.37% <84.21%> (+0.68%) ⬆️

... and 3 files with indirect coverage changes

@antonpirker antonpirker marked this pull request as ready for review September 13, 2024 08:54
Comment on lines +247 to +253
if transaction is not None:
is_http_response = (
event.get("type") == "http.response.start"
and "status" in event
)
if is_http_response:
transaction.set_http_status(event["status"])
Copy link
Member Author

Choose a reason for hiding this comment

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

I did it this way to make mypy happy

Copy link
Contributor

Choose a reason for hiding this comment

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

mypy also made me do things I'm not proud of. We need mypy anonymous

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.

See comments, we can dedup some things but looks very nice

@@ -15,6 +15,19 @@
from sentry_sdk.utils import AnnotatedValue


DEFAULT_HTTP_METHODS_TO_CAPTURE = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we maybe just import this from _wsgi_common here? Or directly use the _wsgi_common constant in the integrations?

Comment on lines +61 to +65
# This noop context manager can be replaced with "from contextlib import nullcontext" when we drop Python 3.6 support
@contextmanager
def nullcontext():
# type: () -> Iterator[None]
yield
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe move this to utils.py and use it from there? Also for wsgi.py

Comment on lines +247 to +253
if transaction is not None:
is_http_response = (
event.get("type") == "http.response.start"
and "status" in event
)
if is_http_response:
transaction.set_http_status(event["status"])
Copy link
Contributor

Choose a reason for hiding this comment

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

mypy also made me do things I'm not proud of. We need mypy anonymous

@antonpirker antonpirker changed the base branch from master to antonpirker/http_methods_to_capture October 1, 2024 10:11
@antonpirker antonpirker merged commit 8944047 into antonpirker/http_methods_to_capture Oct 1, 2024
136 checks passed
@antonpirker antonpirker deleted the antonpirker/http_methods_to_capture_asgi branch October 1, 2024 10:12
@antonpirker
Copy link
Member Author

I have merge this PR into #3531 and did the de-duping in the other PR.

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.

Don't trace OPTIONS and HEAD in ASGI frameworks by default
2 participants