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.

Sign up for GitHub

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

Jump to bottom

Support new failed request status codes API in Starlette #3563

Merged
merged 1 commit into from
Sep 25, 2024
Merged

Support new failed request status codes API in Starlette #3563

merged 1 commit into from
Sep 25, 2024

Conversation

szokeasaurusrex
Copy link
Member

Add support to Starlette and FastApi for the new Set[int] API for failed_request_status_codes that we are using in the AIOHttp integration

@szokeasaurusrex szokeasaurusrex marked this pull request as draft September 24, 2024 11:52
@szokeasaurusrex
Copy link
Member Author

This PR depends on some other PRs being merged first. Going to mark this as a draft until those are reviewed, approved, and merged

@codecov Codecov
Copy link

codecov bot commented Sep 24, 2024 •
edited
Loading

❌ 6 Tests Failed:

Tests completed Failed Passed Skipped
11835 6 11829 1643
View the full list of 3 ❄️ flaky tests 
tests.integrations.aiohttp.test_aiohttp test_invalid_response[invalid]

Flake rate in main: 100.00% (Passed 0 times, Failed 58 times)

Stack Traces | 0.058s run time 
.../integrations/aiohttp/test_aiohttp.py:626: in test_invalid_response
    await client.get("/")
E   Failed: DID NOT RAISE <class 'aiohttp.client_exceptions.ServerDisconnectedError'>

tests.integrations.aiohttp.test_aiohttp test_invalid_response[None]

Flake rate in main: 100.00% (Passed 0 times, Failed 58 times)

Stack Traces | 0.058s run time 
.../integrations/aiohttp/test_aiohttp.py:626: in test_invalid_response
    await client.get("/")
E   Failed: DID NOT RAISE <class 'aiohttp.client_exceptions.ServerDisconnectedError'>

tests.integrations.aiohttp.test_aiohttp test_invalid_response[invalid]

Flake rate in main: 100.00% (Passed 0 times, Failed 59 times)

Stack Traces | 0.068s run time 
.../integrations/aiohttp/test_aiohttp.py:625: in test_invalid_response
    with pytest.raises(ServerDisconnectedError):
E   Failed: DID NOT RAISE <class 'aiohttp.client_exceptions.ServerDisconnectedError'>

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/support-new-failed_request_status_codes branch from e65d313 to c25852c Compare September 24, 2024 12:24
Base automatically changed from szokeasaurusrex/fix-failed_request_status_codes to master September 24, 2024 12:24
@szokeasaurusrex szokeasaurusrex marked this pull request as ready for review September 24, 2024 12:26
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/support-new-failed_request_status_codes branch from c25852c to 52993a0 Compare September 24, 2024 12:31
sentrivana
sentrivana approved these changes Sep 24, 2024
View reviewed changes
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.

Looking good in general, but please see 2 comments

tests/integrations/starlette/test_starlette.py Show resolved Hide resolved
tests/integrations/starlette/test_starlette.py
Comment on lines +1169 to +1170
([NonIterableContainer(range(500, 600))], 500, True),
([NonIterableContainer(range(500, 600))], 404, False),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need the NonIterableContainer cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

See this part of the NonIterableContainer docstring (lines 1140-1141):

Used to test backwards compatibility with our old way of defining failed_request_status_codes, which allowed passing in a list of (possibly non-iterable) containers.

The background for why I added this test case is originally I wanted to implement backwards compatibility by just converting any user-provided list[HttpStatusCodeRange] to the new Set[int] by iterating over the codes and adding all of them to the set. However, this would have been a breaking change from the old API, since an HttpStatusCodeRange is defined as an int or Container[int], and a Container[int] is not necessarily iterable. The NonIterableContainer test cases would have failed against my original implementation; I added them to ensure we don't accidentally break this behavior.

@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/support-new-failed_request_status_codes branch from 52993a0 to 04aac93 Compare September 25, 2024 07:02
@szokeasaurusrex
feat(starlette): Support new failed_request_status_codes
dbece9f 
Add support for passing `failed_request_status_codes` to the `StarletteIntegration` and `FastApiIntegration` constructors as a `Set[int]`, while maintaining backwards-compatibility with the old format.
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/support-new-failed_request_status_codes branch from 04aac93 to dbece9f Compare September 25, 2024 07:36
@szokeasaurusrex szokeasaurusrex merged commit 6489fa0 into master Sep 25, 2024
121 of 125 checks passed
@szokeasaurusrex szokeasaurusrex deleted the szokeasaurusrex/support-new-failed_request_status_codes branch September 25, 2024 09:33
@szokeasaurusrex szokeasaurusrex mentioned this pull request Sep 25, 2024
Closed
szokeasaurusrex added a commit to getsentry/sentry-docs that referenced this pull request Sep 25, 2024
@szokeasaurusrex
docs: Update FastAPI/Starlette failed_request_status_codes docs
41ac2f4 
We have slightly changed the `failed_request_status_codes` API for FastAPI and Starlette in getsentry/sentry-python#3563. The old API is still supported, but it is deprecated, so the docs should only document the new API.
@szokeasaurusrex szokeasaurusrex mentioned this pull request Sep 25, 2024
Merged
3 tasks
szokeasaurusrex added a commit to getsentry/sentry-docs that referenced this pull request Sep 26, 2024
@szokeasaurusrex
docs: Update FastAPI/Starlette failed_request_status_codes docs
596a706 
We have slightly changed the `failed_request_status_codes` API for FastAPI and Starlette in getsentry/sentry-python#3563. The old API is still supported, but it is deprecated, so the docs should only document the new API.
szokeasaurusrex added a commit to getsentry/sentry-docs that referenced this pull request Oct 4, 2024
@szokeasaurusrex
docs: Update FastAPI/Starlette failed_request_status_codes docs
a7710e3 
We have slightly changed the `failed_request_status_codes` API for FastAPI and Starlette in getsentry/sentry-python#3563. The old API is still supported, but it is deprecated, so the docs should only document the new API.
szokeasaurusrex added a commit to getsentry/sentry-docs that referenced this pull request Oct 4, 2024
@szokeasaurusrex
docs: Update FastAPI/Starlette failed_request_status_codes docs (#1…
02ba09c 
…1435)

We have slightly changed the `failed_request_status_codes` API for FastAPI and Starlette in getsentry/sentry-python#3563. The old API is still supported, but it is deprecated, so the docs should only document the new API.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sentrivana sentrivana sentrivana approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@szokeasaurusrex @sentrivana