Skip to content

Support new failed request status codes API in Starlette#3563

Merged
szokeasaurusrex merged 1 commit into
masterfrom
szokeasaurusrex/support-new-failed_request_status_codes
Sep 25, 2024
Merged

Support new failed request status codes API in Starlette#3563
szokeasaurusrex merged 1 commit into
masterfrom
szokeasaurusrex/support-new-failed_request_status_codes

Conversation

@szokeasaurusrex

@szokeasaurusrex szokeasaurusrex commented Sep 24, 2024

Copy link
Copy Markdown
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

szokeasaurusrex commented Sep 24, 2024

Copy link
Copy Markdown
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 Bot commented Sep 24, 2024 •
edited
Loading

Copy link
Copy Markdown

❌ 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

@sentrivana sentrivana left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread tests/integrations/starlette/test_starlette.py
Comment thread tests/integrations/starlette/test_starlette.py
Comment on lines +1169 to +1170
([NonIterableContainer(range(500, 600))], 500, True),
([NonIterableContainer(range(500, 600))], 404, False),

@sentrivana sentrivana Sep 24, 2024

Copy link
Copy Markdown
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?

@szokeasaurusrex szokeasaurusrex Sep 24, 2024

Copy link
Copy Markdown
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
@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