Allow to configure the ITransport to support custom serialization scenarios #944

pchinery · 2021-04-19T09:33:26Z

As discussed in #941, this is a proposal to make the ITransport configurable, which required to make the interface and some other classes along the path public.

The transport will be wrapped with caching, if configured.

codecov-commenter · 2021-04-19T10:04:28Z

Codecov Report

Merging #944 (c42362b) into main (cacbd07) will increase coverage by 0.68%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #944      +/-   ##
==========================================
+ Coverage   81.15%   81.83%   +0.68%     
==========================================
  Files         183      183              
  Lines        5878     5879       +1     
  Branches     1451     1451              
==========================================
+ Hits         4770     4811      +41     
+ Misses        692      666      -26     
+ Partials      416      402      -14

Impacted Files	Coverage Δ
src/Sentry/Envelopes/Envelope.cs	`94.11% <ø> (ø)`
src/Sentry/Envelopes/EnvelopeItem.cs	`89.47% <ø> (ø)`
src/Sentry/Internal/SdkComposer.cs	`65.90% <100.00%> (+28.69%)`	⬆️
src/Sentry/SentryOptions.cs	`90.90% <100.00%> (ø)`
src/Sentry/Protocol/Device.cs	`87.05% <0.00%> (+0.58%)`	⬆️
src/Sentry/Internal/Http/CachingTransport.cs	`79.35% <0.00%> (+0.64%)`	⬆️
src/Sentry/SentryClient.cs	`79.83% <0.00%> (+0.80%)`	⬆️
src/Sentry/Request.cs	`71.42% <0.00%> (+1.58%)`	⬆️
src/Sentry/Protocol/App.cs	`98.07% <0.00%> (+1.92%)`	⬆️
src/Sentry/User.cs	`84.78% <0.00%> (+2.17%)`	⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cacbd07...c42362b. Read the comment docs.

bruno-garcia

Code looks good but it's missing some tests to cover the new behavior.

One outstanding point is the fact we need to make ISerializable also public. @Tyrrrz won't be back for 3 weeks to chime in but I believe he wanted to refactor things before we make those public. So either we'll wait or we need to figure this out ourselves

src/Sentry/SentryOptions.cs

src/Sentry/Internal/SdkComposer.cs

bruno-garcia · 2021-04-19T17:50:23Z

src/Sentry/Envelopes/ISerializable.cs

    /// Represents a serializable entity.
    /// </summary>
-    internal interface ISerializable
+    public interface ISerializable


I think this was another reason @Tyrrrz wanted to make things internal.
He had some plans to refactor this so types don't need to implement this in order to be serialized.

That's most likely something that goes beyond of my understanding of the code vision of the library right now. I'm happy to help, but if there was an idea how to do this already, I might not think in the direction @Tyrrrz wanted to bring this, so I think it's better to wait for him.

As a temp workaround to avoid exposing ISerializable, let's make:

EnvelopeItem.Payload internal (or even private? can we do that?)

EnvelopeItem.ctor internal (we won't expose a public constructor for envelope)

It's ugly but I think exposing ISerializable right now will constrain us too much. I don't like exposing Envelope either but there's no way around it if we want to expose ITransport.

Co-authored-by: Bruno Garcia <[email protected]>

pchinery · 2021-04-20T07:09:07Z

I have now added a test to verify that custom transports a wrapped with a cache. The opposite (to show that no caching is used) would require either to make some properties public on the BackgroundWorker or would require some reflection to verify this. Do you have a preference for any of the two ways or is it good as it is?

Tyrrrz · 2021-05-12T19:15:15Z

src/Sentry/Internal/SdkComposer.cs

+            ITransport transport;
+
+            if (_options.Transport is not null)
+            {
+                transport = _options.Transport;
+            }
+            else
+            {
+                transport = CreateHttpTransport();
+            }


Suggested change

ITransport transport;

if (_options.Transport is not null)

{

transport = _options.Transport;

}

else

{

transport = CreateHttpTransport();

}

var transport = _options.Transport ?? CreateHttpTransport();

test/Sentry.Tests/SentrySdkTests.cs

Tyrrrz · 2021-05-12T19:19:58Z

src/Sentry/Envelopes/ISerializable.cs

    /// Represents a serializable entity.
    /// </summary>
-    internal interface ISerializable
+    public interface ISerializable


As a temp workaround to avoid exposing ISerializable, let's make:

EnvelopeItem.Payload internal (or even private? can we do that?)

EnvelopeItem.ctor internal (we won't expose a public constructor for envelope)

It's ugly but I think exposing ISerializable right now will constrain us too much. I don't like exposing Envelope either but there's no way around it if we want to expose ITransport.

Alexandr-Pletnev · 2021-05-20T15:44:42Z

As discussed in #941, this is a proposal to make the ITransport configurable, which required to make the interface and some other classes along the path public.

The transport will be wrapped with caching, if configured.

It's great feature! Thanks.

Could you help me in one use-case?

I have to develop a use-case: Send the Sentry Events from a private side throught RabbitMq to a public side and then send them to Sentry Server.

What I did:

I developed own ITransport that send an Envelope thought RabbitMq.
I developed the RabbitMq consumer that receive the Envelope. Everything ok.
I need to send the Envelope to the Sentry Server. But i can't realise how i can do it.

I will plesant if you help me.

Tyrrrz · 2021-05-20T17:25:17Z

As discussed in #941, this is a proposal to make the ITransport configurable, which required to make the interface and some other classes along the path public.
The transport will be wrapped with caching, if configured.

It's great feature! Thanks.

Could you help me in one use-case?

I have to develop a use-case: Send the Sentry Events from a private side throught RabbitMq to a public side and then send them to Sentry Server.

What I did:

I developed own ITransport that send an Envelope thought RabbitMq.

I developed the RabbitMq consumer that receive the Envelope. Everything ok.

I need to send the Envelope to the Sentry Server. But i can't realise how i can do it.

I will plesant if you help me.

I don't think there's a straightforward way to achieve that (@bruno-garcia can correct me if I'm wrong).

I think your best bet is to write the envelopes from RabbitMQ to a local directory, and pick them up via caching feature:

sentry-dotnet/src/Sentry/SentryOptions.cs

Lines 385 to 389 in 984b210

    
           /// <summary> 
        
           /// Path to the root directory used for storing events locally for resilience. 
        
           /// If set to <i>null</i>, caching will not be used. 
        
           /// </summary> 
        
           public string? CacheDirectoryPath { get; set; }

The files will need to be placed in a specific subdirectory as well. Assuming SentryOptions.CacheDirectoryPath = "path/to/some/directory/", the files would need to be seeded in:

path/to/some/directory/Sentry/{your DSN}/

The file name doesn't matter, but the extension has to be .envelope. You can inspect the implementation here for more info: https://github.com/getsentry/sentry-dotnet/blob/984b21063ca806df0129a89aeec011f95012eca5/src/Sentry/Internal/Http/CachingTransport.cs

Cached envelopes are flushed automatically during SentrySdk.Init(...) but can also be flushed manually through SentrySdk.FlushAsync(...) (I think).

That said, all of this is very hacky and I'm not sure how well this will work. We probably need a proper solution for this, but currently all of the relevant interfaces and implementations are internal.

Alexandr-Pletnev · 2021-05-21T07:05:06Z

Thank you for your reply.

I decided to achieve this via reflections like that:

`
//

FieldInfo fiHub = typeof(SentrySdk).GetField("_hub", BindingFlags.NonPublic | BindingFlags.Static);
object hub = fiHub.GetValue(null);

FieldInfo fiClient = hub.GetType().GetField("_ownedClient", BindingFlags.NonPublic | BindingFlags.Instance);
object client = fiClient.GetValue(hub);

var mi = client.GetType().GetMethod("CaptureEnvelope", BindingFlags.NonPublic | BindingFlags.Instance);
object result = mi.Invoke(client, new object[] { envelope });

`

mattjohnsonpint · 2022-04-26T22:03:38Z

Hi. Sorry this PR is open for so long. I'm taking a look now, and it seems like most of what it does has already been addressed by other PRs for other reasons, as follows:

Making Envelope, EnvelopeItem, and ISerializable public was done with BackgroundWorker: make types public #1450, released in 3.14.0.
Making ITransport and options.Transport public was not done, but rather we extracted most of the logic from HttpTransport into the public abstract HttpTransportBase class with Add HttpTransport extensibility and synchronous serialization support #1560, released in 3.16.0. This was done primarily to support Unity WebGL.
Ensuring CreateHttpTransport correctly uses options.Transport when set, and wraps with CachingTransport properly was done with Fix options internal reference to transport #1589 (unreleased as of today)

I think to achieve the stated goals, all that needs to be done is to make ITransport and options.Transport public. That would allow others to create their own transports (extending from HttpTransportBase if desired). Unity didn't need that, because they also needed to create their own implementation of IBackgroundWorker - which is public. But we shouldn't require others to do that just to attach a transport.

I'll close this PR and open a new one with that.

mattjohnsonpint · 2022-04-26T22:47:19Z

See #1602

pchinery requested review from Tyrrrz and bruno-garcia as code owners April 19, 2021 09:33

pchinery mentioned this pull request Apr 19, 2021

Allow to serialize events #941

Closed

pchinery force-pushed the main branch from 12bcbf6 to 065ec58 Compare April 19, 2021 09:41

Allow to configure the to support custom serialization scenarios

21582f4

pchinery force-pushed the main branch from 065ec58 to 21582f4 Compare April 19, 2021 09:43

bruno-garcia reviewed Apr 19, 2021

View reviewed changes

pchinery and others added 3 commits April 20, 2021 08:17

Update src/Sentry/SentryOptions.cs

fe56c1f

Co-authored-by: Bruno Garcia <[email protected]>

Update src/Sentry/Internal/SdkComposer.cs

c7430fb

Co-authored-by: Bruno Garcia <[email protected]>

Add test to verify that the custom transport is cached

fd4f23a

Tyrrrz reviewed May 12, 2021

View reviewed changes

Tyrrrz added 2 commits May 12, 2021 12:21

Merge branch 'main' into main

00161a6

Merge branch 'main' into main

c42362b

Silvenga mentioned this pull request May 28, 2021

Expose ITransport and Friends Publicly #1018

Closed

mattjohnsonpint closed this Apr 26, 2022

Uh oh!

Allow to configure the ITransport to support custom serialization scenarios #944

Allow to configure the ITransport to support custom serialization scenarios #944

Uh oh!

Conversation

pchinery commented Apr 19, 2021

Uh oh!

codecov-commenter commented Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bruno-garcia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bruno-garcia Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

pchinery Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

Tyrrrz May 12, 2021

Choose a reason for hiding this comment

Uh oh!

pchinery commented Apr 20, 2021

Uh oh!

Tyrrrz May 12, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Tyrrrz May 12, 2021

Choose a reason for hiding this comment

Uh oh!

Alexandr-Pletnev commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tyrrrz commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alexandr-Pletnev commented May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattjohnsonpint commented Apr 26, 2022

Uh oh!

mattjohnsonpint commented Apr 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-commenter commented Apr 19, 2021 •

edited

Loading

Alexandr-Pletnev commented May 20, 2021 •

edited

Loading

Tyrrrz commented May 20, 2021 •

edited

Loading

Alexandr-Pletnev commented May 21, 2021 •

edited

Loading