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

Add HttpTransport extensibility and synchronous serialization support #1560

Merged
merged 5 commits into from
Mar 31, 2022

Conversation

mattjohnsonpint
Copy link
Contributor

@mattjohnsonpint mattjohnsonpint commented Mar 30, 2022

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, reducing SendEnvelopeAsync to a small bit of orchestration code. Higher-level SDKs (such as UnityWebRequestTransport proposed in getsentry/sentry-unity#657) can implement their own version of that method without having to take a dependency on HttpClient or async/await.

Note that ITransport is still internal and implemented only on HttpTransport - not HttpTransportBase.

A fully synchronous path for handling response messages is available as HandleResponse instead of HandleResponseAsync (either can be used by the parent class). In order to fully implement this correctly, Sentry.Protocol.Envelopes.ISerializable has been given a Serialize method which mirrors the existing SerializeAsync 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 like Stream.Write instead of Stream.WriteAsync - which are implemented in .NET using the correct primitives (no sync over async or vice versa).

@mattjohnsonpint
Copy link
Contributor Author

mattjohnsonpint commented Mar 30, 2022

Just because it's a bit hard to read in the diff, here's the new version of HttpTransport.SendEnvelopeAsync in its entirety. This is now the only method in the class.

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);
    }
}

Copy link
Collaborator

@vaind vaind left a 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)

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

LGTM

src/Sentry/Envelopes/Envelope.cs Show resolved Hide resolved
@mattjohnsonpint
Copy link
Contributor Author

I removed the HttpContentReader, and just added a synchronous serialization path for EnvelopeHttpContent.

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.

3 participants