-
Notifications
You must be signed in to change notification settings - Fork 554
ref(transport): Add shared sync/async transport superclass and create a sync http subclass #4572
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
base: potel-base
Are you sure you want to change the base?
Conversation
❌ 54 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
53b92f2
to
3607d44
Compare
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.
Other than my little nitpicking, this looks great!
sentry_sdk/transport.py
Outdated
@@ -162,7 +162,7 @@ def _parse_rate_limits( | |||
continue | |||
|
|||
|
|||
class BaseHttpTransport(Transport): | |||
class HttpTransportCore(Transport): | |||
"""The base HTTP transport.""" |
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.
Please also update the docstring
# remove all items from the envelope which are over quota | ||
def _prepare_envelope( | ||
self: Self, envelope: Envelope | ||
) -> Optional[Tuple[Envelope, io.BytesIO, Dict[str, str]]]: | ||
new_items = [] |
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.
please keep the # remove all items from the envelope which are over quota
comment
sentry_sdk/transport.py
Outdated
_prepared_envelope = self._prepare_envelope(envelope) | ||
if _prepared_envelope is None: | ||
return None | ||
envelope, body, headers = _prepared_envelope | ||
self._send_request( | ||
body.getvalue(), | ||
headers=headers, | ||
endpoint_type=EndpointType.ENVELOPE, | ||
envelope=envelope, | ||
) | ||
return None |
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.
Just a personal taste but I would change this to only have one return None
so it is easier to read.
_prepared_envelope = self._prepare_envelope(envelope) | |
if _prepared_envelope is None: | |
return None | |
envelope, body, headers = _prepared_envelope | |
self._send_request( | |
body.getvalue(), | |
headers=headers, | |
endpoint_type=EndpointType.ENVELOPE, | |
envelope=envelope, | |
) | |
return None | |
_prepared_envelope = self._prepare_envelope(envelope) | |
if _prepared_envelope is not None: | |
envelope, body, headers = _prepared_envelope | |
self._send_request( | |
body.getvalue(), | |
headers=headers, | |
endpoint_type=EndpointType.ENVELOPE, | |
envelope=envelope, | |
) | |
return None |
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.
Agree, this looks nicer. Thanks!
@@ -641,3 +641,38 @@ def test_record_lost_event_transaction_item(capturing_server, make_client, span_ | |||
"reason": "test", | |||
"quantity": span_count + 1, | |||
} in discarded_events | |||
|
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.
Please add a test that tests those untested lines here: https://app.codecov.io/gh/getsentry/sentry-python/pull/4572/blob/sentry_sdk/transport.py#L292
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.
Done!
…ted a sync transport HTTP subclass Moved shared sync/async logic into a new superclass (HttpTransportCore), and moved sync transport specific code into a new subclass(BaseSyncHttpTransport), from which the current transport implementations inherit Fixes GH-4568
Removed an unnecessary TODO message and reverted a class name change for BaseHTTPTransport. GH-4568
Adds test coverage for the error handling path when HTTP requests return error status codes. GH-4568
Restore comments accidentally removed during a previous commit.
Refactored class names such that BaseHttpTransport now has the same functionality as before the hierarchy refactor GH-4568
Add a new flush_async method in the Transport ABC. This is needed for the async transport, as calling it from the client while preserving execution order in close will require flush to be a coroutine, not a function. GH-4568
Move flush_async down to the specific async transport subclass. This makes more sense anyway, as this will only be required by the async transport. If more async transports are expected, another shared superclass can be created. GH-4568
Add necessary type annotations to the core HttpTransport to accomodate for async transport. GH-4568
…nd cleaner return paths GH-4568
062ab5b
to
4e56e5c
Compare
Moved shared sync/async logic into superclass, and moved sync transport specific code into a new subclass(BaseSyncHttpTransport), from which the current transport implementations inherit. Note that currently the threaded worker is still being created in the superclass. In a next step, I want to add an abstract worker class for both the threaded and async task worker, however I wanted to keep this PR atomic and only re-implement the sync transport with the new hierarchy.
Fixes GH-4568