-
-
Notifications
You must be signed in to change notification settings - Fork 206
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
Add HttpTransport extensibility and synchronous serialization support #1560
Conversation
Just because it's a bit hard to read in the diff, here's the new version of public async Task SendEnvelopeAsync(Envelope envelope, CancellationToken cancellationToken = default)
{
using var processedEnvelope = ProcessEnvelope(envelope);
if (processedEnvelope.Items.Count > 0)
{
using var request = CreateRequest(processedEnvelope);
using var response = await _httpClient.SendAsync(request, cancellationToken).ConfigureAwait(false);
await HandleResponseAsync(response, processedEnvelope, cancellationToken).ConfigureAwait(false);
}
} |
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.
Works fine for the unity WebGL implementation. There's just one open point that could maybe be improved. See getsentry/sentry-unity#657 (review)
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.
LGTM
I removed the |
As mentioned in #1553 (comment), we need to have a clean way for higher-level SDKs (like sentry-unity) to implement their own HTTP transports. In doing so, we'll need to support both asynchronous and synchronous handling of response messages, including synchronous serialization support for things like logging of payloads.
This PR takes most of the functionality of
HttpTransport
and pulls it into an abstract base class,HttpTransportBase
. That allows protected access to the core functionality of the transport, reducingSendEnvelopeAsync
to a small bit of orchestration code. Higher-level SDKs (such asUnityWebRequestTransport
proposed in getsentry/sentry-unity#657) can implement their own version of that method without having to take a dependency onHttpClient
or async/await.Note that
ITransport
is still internal and implemented only onHttpTransport
- notHttpTransportBase
.A fully synchronous path for handling response messages is available as
HandleResponse
instead ofHandleResponseAsync
(either can be used by the parent class). In order to fully implement this correctly,Sentry.Protocol.Envelopes.ISerializable
has been given aSerialize
method which mirrors the existingSerializeAsync
method. All implementations and tests are identical, except as to whether async methods are employed or not. At the lowest level, these now use things likeStream.Write
instead ofStream.WriteAsync
- which are implemented in .NET using the correct primitives (no sync over async or vice versa).