Envelopes by Tyrrrz · Pull Request #544 · getsentry/sentry-dotnet

Tyrrrz · 2020-10-06T16:28:26Z

No description provided.

src/Sentry/Protocol/EnvelopeItem.cs

src/Sentry/Internal/Http/HttpTransport.cs

src/Sentry/Protocol/Builders/EnvelopeBuilder.cs

src/Sentry/Protocol/Envelope.cs

codecov-io · 2020-10-07T17:21:58Z

Codecov Report

Merging #544 into main will increase coverage by 0.22%.
The diff coverage is 91.39%.

@@            Coverage Diff             @@
##             main     #544      +/-   ##
==========================================
+ Coverage   85.33%   85.55%   +0.22%     
==========================================
  Files         128      135       +7     
  Lines        3170     3274     +104     
  Branches      730      732       +2     
==========================================
+ Hits         2705     2801      +96     
- Misses        268      276       +8     
  Partials      197      197

Impacted Files	Coverage Δ
...entry/Internal/Extensions/CollectionsExtensions.cs	`100.00% <ø> (ø)`
src/Sentry/Protocol/BaseScopeExtensions.cs	`86.66% <ø> (ø)`
src/Sentry/Protocol/Context/Contexts.cs	`90.90% <ø> (ø)`
src/Sentry/Protocol/Request.cs	`92.00% <ø> (ø)`
src/Sentry/Protocol/EnvelopeHeaderCollection.cs	`71.42% <71.42%> (ø)`
src/Sentry/SentryClient.cs	`80.89% <80.00%> (-1.66%)`	⬇️
src/Sentry/Protocol/EnvelopePayload.cs	`83.33% <83.33%> (ø)`
src/Sentry/Protocol/EnvelopeItemCollection.cs	`85.71% <85.71%> (ø)`
src/Sentry/Internal/Http/HttpTransport.cs	`89.47% <87.50%> (+2.80%)`	⬆️
src/Sentry/Protocol/Envelope.cs	`89.47% <89.47%> (ø)`
... and 13 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 42b0f92...2d9c6aa. Read the comment docs.

Tyrrrz · 2020-10-09T14:16:10Z

Took your approach @bruno-garcia and made a very hacky implementation.
It works:

I first started out with the concept of returning a Stream from Envelope but later realized that implementing a custom HttpContent is way easier.

bruno-garcia

Nice! This is going on a great direction!

src/Sentry/Internal/Http/EnvelopeHttpContent.cs

src/Sentry/Internal/Polyfills.cs

src/Sentry/Protocol/Builders/EnvelopeBuilder.cs

src/Sentry/Protocol/Builders/EnvelopeItemBuilder.cs

bruno-garcia · 2020-10-09T14:59:26Z

Where's is the hacky code? !

Tyrrrz · 2020-10-09T15:22:29Z

Where's is the hacky code? !

For example where I do stream.WriteByte((byte) '\n') :D

Tyrrrz · 2020-10-12T14:05:45Z

@bruno-garcia went through a couple of iterations, I think this one is okay in regards to streaming events.
I also replaced builders with factory methods as I concluded that it's very unlikely that we will be building "free-form" envelopes (as I assumed originally), but rather generate them from existing structures.

Tyrrrz · 2020-10-12T14:15:42Z

If this is okay feature-wise, maybe we can make a cut off, fix tests, refactor & merge this? Looking at the plan, it seems that this makes up a rather significant and self-contained portion of the feature set we're aiming for.

(green is done)

This way we can keep PRs smaller and break fewer things in between.

bruno-garcia

Nice! we're almost ready to remove the Draft flag it seems?
I left a few notes, and I used the LOGAF scale. Basically L, M, H as in low medium and high.

benchmarks/Sentry.Benchmarks/BackgroundWorkerFlushBenchmarks.cs

src/Sentry/Internal/Http/SerializableHttpContent.cs

src/Sentry/Protocol/EnvelopeHeaderCollection.cs

src/Sentry/Protocol/EnvelopeItem.cs

src/Sentry/Protocol/ISerializable.cs

src/Sentry/Protocol/SentryEvent.cs

test/Sentry.AspNetCore.Tests/IntegrationMockedBackgroundWorker.cs

test/Sentry.Tests/Protocol/EnvelopeTests.cs

test/xunit.runner.json

codecov-commenter · 2020-10-13T16:56:49Z

Codecov Report

Merging #544 into main will increase coverage by 0.22%.
The diff coverage is 91.39%.

@@            Coverage Diff             @@
##             main     #544      +/-   ##
==========================================
+ Coverage   85.33%   85.55%   +0.22%     
==========================================
  Files         128      135       +7     
  Lines        3170     3274     +104     
  Branches      730      732       +2     
==========================================
+ Hits         2705     2801      +96     
- Misses        268      276       +8     
  Partials      197      197

Impacted Files	Coverage Δ
...entry/Internal/Extensions/CollectionsExtensions.cs	`100.00% <ø> (ø)`
src/Sentry/Protocol/BaseScopeExtensions.cs	`86.66% <ø> (ø)`
src/Sentry/Protocol/Context/Contexts.cs	`90.90% <ø> (ø)`
src/Sentry/Protocol/Request.cs	`92.00% <ø> (ø)`
src/Sentry/Protocol/EnvelopeHeaderCollection.cs	`71.42% <71.42%> (ø)`
src/Sentry/SentryClient.cs	`80.89% <80.00%> (-1.66%)`	⬇️
src/Sentry/Protocol/EnvelopePayload.cs	`83.33% <83.33%> (ø)`
src/Sentry/Protocol/EnvelopeItemCollection.cs	`85.71% <85.71%> (ø)`
src/Sentry/Internal/Http/HttpTransport.cs	`89.47% <87.50%> (+2.80%)`	⬆️
src/Sentry/Protocol/Envelope.cs	`89.47% <89.47%> (ø)`
... and 13 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 42b0f92...87b0f2a. Read the comment docs.

Tyrrrz · 2020-10-13T16:56:59Z

@bruno-garcia implemented dynamic length calculation, moving on to tests now probably.

Tyrrrz · 2020-10-14T13:47:41Z

One more question I realized: should it be Envelope/EnvelopeItem or SentryEnvelope/SentryEnvelopeItem for consistency?

Tyrrrz · 2020-10-14T13:57:47Z

Interestingly enough, Arg<T>.Do doesn't seem to actually trigger a callback so tests that rely on it seems to pass regardless of the result.

Tyrrrz · 2020-10-14T14:54:03Z

Newtonsoft.Json doesn't actually support async serialization. Our best bet is:

public static async Task SerializeToStreamAsync(
            object obj,
            Stream stream,
            CancellationToken cancellationToken = default)
        {
            using var writer = CreateWriter(stream);
            Serializer.Serialize(writer, obj);
            await writer.FlushAsync(cancellationToken).ConfigureAwait(false);
        }

Notably, it's probably going to be the same with System.Text.Json, unless we serialize manually.

Tyrrrz · 2020-10-14T14:59:44Z

@bruno-garcia some mocking magic is failing, I'm guessing it's because Arg.Is<T>(...) uses an expression for predicate, so it can't handle more involved scenarios. In any case, the test errors don't give any clues:

Error: NotSentry.Tests.HubTests.CaptureMessage_AttachStacktraceTrue_IncludesStackTrace: NSubstitute.Exceptions.ReceivedCallsException : Expected to receive exactly 1 call matching:
  Failed NotSentry.Tests.HubTests.CaptureMessage_AttachStacktraceTrue_IncludesStackTrace [20 ms]
  Error Message:
   NSubstitute.Exceptions.ReceivedCallsException : Expected to receive exactly 1 call matching:
	EnqueueEnvelope(e => (e.Items.Select(i => i.Payload).OfType().Single().Exception.StackTrace != null))
Actually received no matching calls.
Received 1 non-matching call (non-matching arguments indicated with '*' characters):
	EnqueueEnvelope(*Envelope*)

  Stack Trace:
     at NSubstitute.Core.ReceivedCallsExceptionThrower.Throw(ICallSpecification callSpecification, IEnumerable`1 matchingCalls, IEnumerable`1 nonMatchingCalls, Quantity requiredQuantity)
   at NSubstitute.Routing.Handlers.CheckReceivedCallsHandler.Handle(ICall call)
   at System.Linq.Enumerable.SelectArrayIterator`2.MoveNext()
   at System.Linq.Enumerable.TryGetFirst[TSource](IEnumerable`1 source, Func`2 predicate, Boolean& found)
   at NSubstitute.Routing.Route.Handle(ICall call)
   at NSubstitute.Proxies.CastleDynamicProxy.CastleForwardingInterceptor.Intercept(IInvocation invocation)
   at Castle.DynamicProxy.AbstractInvocation.Proceed()
   at Castle.Proxies.IBackgroundWorkerProxy_1.EnqueueEnvelope(Envelope envelope)
   at NotSentry.Tests.HubTests.CaptureMessage_AttachStacktraceTrue_IncludesStackTrace() in /home/runner/work/sentry-dotnet/sentry-dotnet/test/Sentry.Tests/HubTests.cs:line 77

codecov-io · 2020-10-15T15:26:09Z

Codecov Report

Merging #544 into main will decrease coverage by 0.16%.
The diff coverage is 84.79%.

@@            Coverage Diff             @@
##             main     #544      +/-   ##
==========================================
- Coverage   85.33%   85.16%   -0.17%     
==========================================
  Files         128      137       +9     
  Lines        3170     3317     +147     
  Branches      730      749      +19     
==========================================
+ Hits         2705     2825     +120     
- Misses        268      294      +26     
- Partials      197      198       +1

Impacted Files	Coverage Δ
src/Sentry/Protocol/BaseScopeExtensions.cs	`86.66% <ø> (ø)`
src/Sentry/Protocol/Context/Contexts.cs	`90.90% <ø> (ø)`
src/Sentry/Protocol/Request.cs	`92.00% <ø> (ø)`
src/Sentry/Internal/Polyfills.cs	`50.00% <50.00%> (ø)`
src/Sentry/Protocol/EnvelopeItem.cs	`60.00% <60.00%> (ø)`
src/Sentry/Protocol/StreamSerializable.cs	`66.66% <66.66%> (ø)`
src/Sentry/SentryClient.cs	`80.45% <66.66%> (-2.10%)`	⬇️
src/Sentry/Internal/Http/HttpTransport.cs	`89.47% <88.88%> (+2.80%)`	⬆️
src/Sentry/Internal/BackgroundWorker.cs	`84.28% <93.18%> (+0.82%)`	⬆️
src/Sentry/Protocol/Envelope.cs	`96.15% <96.15%> (ø)`
... and 22 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 42b0f92...88c46e0. Read the comment docs.

Tyrrrz · 2020-10-15T15:53:06Z

This test seems to never have actually worked:

sentry-dotnet/test/Sentry.Tests/HubTests.cs

Lines 58 to 71 in 7166cc2

    
           [Fact] 
        
           public void CaptureMessage_AttachStacktraceTrue_IncludesStackTrace() 
        
           { 
        
               _fixture.SentryOptions.AttachStacktrace = true; 
        
               var sut = _fixture.GetSut(); 
        
               _ = sut.CaptureMessage("test"); 
        
               _ = _fixture.Worker.Received(1) 
        
                       .EnqueueEvent(Arg.Do<SentryEvent>( 
        
                           e => Assert.DoesNotContain(e.SentryExceptions.Single().Stacktrace.Frames, 
        
                               p => p.Function == nameof(CaptureMessage_AttachStacktraceTrue_IncludesStackTrace)))); 
        
           }

The Arg.Do<> part was just ignored. I couldn't find any logic in code that actually does anything with AttachStackTrace. On top of that, the name of the test seems to mismatch the assertion. So I decided to remove it because it doesn't make sense. Will probably need to add another, proper test.

bruno-garcia

Elegant! Thanks for this @Tyrrrz , great stuff!

test/Sentry.Tests/SentrySdkTests.cs

Tyrrrz added 2 commits October 6, 2020 18:26

Envelope data model & builders

52c915f

Tests & fix formatting

df49ead

Tyrrrz added Feature New feature or request Sentry labels Oct 6, 2020

bruno-garcia reviewed Oct 6, 2020

View reviewed changes

src/Sentry/Protocol/EnvelopeItem.cs Outdated Show resolved Hide resolved

src/Sentry/Protocol/EnvelopeItem.cs Outdated Show resolved Hide resolved

Tyrrrz added 2 commits October 6, 2020 23:26

wip

825ecd3

wip

20e2929

bruno-garcia reviewed Oct 7, 2020

View reviewed changes

src/Sentry/Internal/Http/HttpTransport.cs Outdated Show resolved Hide resolved

bruno-garcia reviewed Oct 7, 2020

View reviewed changes

src/Sentry/Protocol/Builders/EnvelopeBuilder.cs Outdated Show resolved Hide resolved

bruno-garcia reviewed Oct 7, 2020

View reviewed changes

src/Sentry/Protocol/Envelope.cs Outdated Show resolved Hide resolved

Tyrrrz added 4 commits October 7, 2020 19:41

unit test shitfuckery

8384cd4

docs

36cc66e

docs

25e0a40

more tests fixing

c0e83e9

Tyrrrz added 2 commits October 7, 2020 20:23

wip

54cea18

Streaming serialization

be6748c

bruno-garcia reviewed Oct 9, 2020

View reviewed changes

Tyrrrz added 2 commits October 9, 2020 17:19

wip

6559355

wip

5ba7405

wip

341233b

bruno-garcia reviewed Oct 12, 2020

View reviewed changes

Tyrrrz added 3 commits October 13, 2020 16:24

wip

5a1a22a

wip

2d9c6aa

wip

cd9a69e

Tyrrrz added 4 commits October 13, 2020 19:29

wip

83eaef6

wip

25741ec

wip

cbcc630

wip

87b0f2a

Tyrrrz added 3 commits October 14, 2020 16:30

wip

2b6992e

wip

b17ba63

wip

64d06ce

Tyrrrz marked this pull request as ready for review October 14, 2020 13:43

wip

67745bb

Tyrrrz added 2 commits October 14, 2020 17:38

wip

00ea03e

wip

d6ba9f3

Tyrrrz added 5 commits October 14, 2020 18:25

wip

32c3b27

wip

e88429c

wip

f013523

Rework HubTests

4769395

Rework HttpTransport tests

54b4a2b

Remove broken test

88c46e0

bruno-garcia approved these changes Oct 15, 2020

View reviewed changes

test/Sentry.Tests/SentrySdkTests.cs Show resolved Hide resolved

bruno-garcia merged commit 5dada5b into main Oct 15, 2020

bruno-garcia deleted the envelopes branch October 15, 2020 23:50

This was referenced Oct 20, 2020

User Feedback #153

Closed

Cache Envelopes #557

Closed

Uh oh!

Conversation

Tyrrrz commented Oct 6, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-io commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Tyrrrz commented Oct 9, 2020

Uh oh!

bruno-garcia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bruno-garcia commented Oct 9, 2020

Uh oh!

Tyrrrz commented Oct 9, 2020

Uh oh!

Tyrrrz commented Oct 12, 2020

Uh oh!

Tyrrrz commented Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bruno-garcia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Oct 13, 2020

Codecov Report

Uh oh!

Tyrrrz commented Oct 13, 2020

Uh oh!

Tyrrrz commented Oct 14, 2020

Uh oh!

Tyrrrz commented Oct 14, 2020

Uh oh!

Tyrrrz commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tyrrrz commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Tyrrrz commented Oct 15, 2020

Uh oh!

bruno-garcia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Oct 7, 2020 •

edited

Loading

Tyrrrz commented Oct 12, 2020 •

edited

Loading

Tyrrrz commented Oct 14, 2020 •

edited

Loading

Tyrrrz commented Oct 14, 2020 •

edited

Loading

codecov-io commented Oct 15, 2020 •

edited

Loading