fix: Keep events in case of nonserializable context (#1401)

Co-authored-by: Bruno Garcia <bruno@brunogarcia.com> Co-authored-by: Sentry Github Bot <bot+github-bot@sentry.io>
getsentry · Dec 19, 2021 · 9f3770f · 9f3770f
1 parent 878bba7
commit 9f3770f
Show file tree

Hide file tree

Showing 9 changed files with 142 additions and 45 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,7 @@
 
 ### Fixes
 
+- Events no longer get dropped because of non-serializable contexts or attachments ([#1401](https://github.com/getsentry/sentry-dotnet/pull/1401))
 - Add MemoryInfo to sentry event ([#1337](https://github.com/getsentry/sentry-dotnet/pull/1337))
 
 ## 3.12.2

diff --git a/src/Sentry/Envelopes/EnvelopeItem.cs b/src/Sentry/Envelopes/EnvelopeItem.cs
@@ -119,7 +119,6 @@ public async Task SerializeAsync(Stream stream, IDiagnosticLogger? logger, Cance
                     // Header
                     var headerWithLength = Header.ToDictionary();
                     headerWithLength[LengthKey] = payloadBuffer.Length;
-
                     await SerializeHeaderAsync(stream, headerWithLength, logger, cancellationToken).ConfigureAwait(false);
                     await stream.WriteByteAsync((byte)'\n', cancellationToken).ConfigureAwait(false);
 

diff --git a/src/Sentry/Internal/Extensions/JsonExtensions.cs b/src/Sentry/Internal/Extensions/JsonExtensions.cs
@@ -296,17 +296,31 @@ public static void WriteDynamic(
             object? value,
             IDiagnosticLogger? logger)
         {
+            writer.WritePropertyName(propertyName);
+            var originalPropertyDepth = writer.CurrentDepth;
             try
             {
-                writer.WritePropertyName(propertyName);
                 writer.WriteDynamicValue(value, logger);
             }
-            catch (Exception e) when (logger != null)
+            catch (Exception e)
             {
-                // The only location in the protocol we allow dynamic objects are Extra and Contexts
                 // In the event of an instance that can't be serialized, we don't want to throw away a whole event
                 // so we'll suppress issues here.
-                logger.LogError("Failed to serialize object for property {0}", e, propertyName);
+                logger?.LogError(e, "Failed to serialize object for property '{0}'. Original depth: {1}, current depth: {2}",
+                    propertyName, originalPropertyDepth, writer.CurrentDepth);
+
+                // The only location in the protocol we allow dynamic objects are Extra and Contexts.
+                // Render an empty JSON object instead of null. This allows a round trip where this property name is the
+                // key to a map which would otherwise not be set and result in a different object.
+                // This affects envelope size which isn't recomputed after a roundtrip.
+                if (originalPropertyDepth == writer.CurrentDepth)
+                {
+                    writer.WriteStartObject();
+                }
+                while (originalPropertyDepth < writer.CurrentDepth)
+                {
+                    writer.WriteEndObject();
+                }
             }
         }
 

diff --git a/src/Sentry/Internal/Http/EnvelopeHttpContent.cs b/src/Sentry/Internal/Http/EnvelopeHttpContent.cs
@@ -1,3 +1,4 @@
+using System;
 using System.IO;
 using System.Net;
 using System.Net.Http;
@@ -18,8 +19,18 @@ public EnvelopeHttpContent(Envelope envelope, IDiagnosticLogger? logger)
             _logger = logger;
         }
 
-        protected override async Task SerializeToStreamAsync(Stream stream, TransportContext? context) =>
-            await _envelope.SerializeAsync(stream, _logger).ConfigureAwait(false);
+        protected override async Task SerializeToStreamAsync(Stream stream, TransportContext? context)
+        {
+            try
+            {
+                await _envelope.SerializeAsync(stream, _logger).ConfigureAwait(false);
+            }
+            catch (Exception e)
+            {
+                _logger?.LogError("Failed to serialize Envelope into the network stream", e);
+                throw;
+            }
+        }
 
         protected override bool TryComputeLength(out long length)
         {

diff --git a/test/Sentry.Google.Cloud.Functions.Tests/IntegrationTests.cs b/test/Sentry.Google.Cloud.Functions.Tests/IntegrationTests.cs
@@ -9,6 +9,7 @@
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Hosting;
 using Sentry.AspNetCore;
+using Sentry.Testing;
 
 namespace Sentry.Google.Cloud.Functions.Tests;
 
@@ -36,7 +37,7 @@ void Verify(HttpRequestMessage message)
                     {
                         // So we can assert on the payload without the need to Gzip decompress
                         o.RequestBodyCompressionLevel = CompressionLevel.NoCompression;
-                        o.CreateHttpClientHandler = () => new TestHandler(Verify);
+                        o.CreateHttpClientHandler = () => new CallbackHttpClientHandler(Verify);
                     });
                     services.AddFunctionTarget<FailingFunction>();
                 })
@@ -75,17 +76,4 @@ public class FailingFunction : IHttpFunction
     {
         public Task HandleAsync(HttpContext context) => throw new Exception(ExpectedMessage);
     }
-
-    private class TestHandler : HttpClientHandler
-    {
-        private readonly Action<HttpRequestMessage> _messageCallback;
-
-        public TestHandler(Action<HttpRequestMessage> messageCallback) => _messageCallback = messageCallback;
-
-        protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
-        {
-            _messageCallback(request);
-            return Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK));
-        }
-    }
 }
diff --git a/test/Sentry.Testing/CallbackHttpClientHandler.cs b/test/Sentry.Testing/CallbackHttpClientHandler.cs
@@ -0,0 +1,17 @@
+using System.Net;
+using System.Net.Http;
+
+namespace Sentry.Testing;
+
+public class CallbackHttpClientHandler : HttpClientHandler
+{
+    private readonly Action<HttpRequestMessage> _messageCallback;
+
+    public CallbackHttpClientHandler(Action<HttpRequestMessage> messageCallback) => _messageCallback = messageCallback;
+
+    protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
+    {
+        _messageCallback(request);
+        return Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK));
+    }
+}
diff --git a/test/Sentry.Testing/FuncHandler.cs b/test/Sentry.Testing/FuncHandler.cs
@@ -8,6 +8,9 @@ public class FuncHandler : HttpMessageHandler
 
     public bool SendAsyncCalled { get; set; }
 
+    public FuncHandler(Func<HttpRequestMessage, CancellationToken, HttpResponseMessage> sendAsyncFunc = null)
+        => SendAsyncFunc = sendAsyncFunc;
+
     protected override Task<HttpResponseMessage> SendAsync(
         HttpRequestMessage request,
         CancellationToken cancellationToken)

diff --git a/test/Sentry.Tests/HubTests.cs b/test/Sentry.Tests/HubTests.cs
@@ -1,5 +1,13 @@
 // ReSharper disable once CheckNamespace
 // Tests code path which excludes frames with namespace Sentry
+
+using System.IO.Compression;
+using System.Net.Http;
+#if NETCOREAPP2_1
+using System.Reflection;
+#endif
+using Sentry.Testing;
+
 namespace NotSentry.Tests;
 
 public class HubTests
@@ -287,6 +295,71 @@ public void CaptureEvent_ExceptionWithOpenSpan_SpanLinkedToEventContext()
         Assert.Equal(child.ParentSpanId, evt.Contexts.Trace.ParentSpanId);
     }
 
+    class EvilContext
+    {
+        public string Thrower => throw new InvalidDataException();
+    }
+
+    [Theory]
+    [InlineData(true)]
+    [InlineData(false)]
+    public void CaptureEvent_NonSerializableContextAndOfflineCaching_CapturesEventWithContextKey(bool offlineCaching)
+    {
+        var resetEvent = new ManualResetEventSlim();
+        var expectedMessage = Guid.NewGuid().ToString();
+
+        var requests = new List<string>();
+        void Verify(HttpRequestMessage message)
+        {
+            var payload = message.Content.ReadAsStringAsync().Result;
+            requests.Add(payload);
+            if (payload.Contains(expectedMessage))
+            {
+                resetEvent.Set();
+            }
+        }
+
+        var cachePath = offlineCaching ? Path.GetTempPath() : null;
+
+        var logger = Substitute.For<IDiagnosticLogger>();
+        var expectedLevel = SentryLevel.Error;
+        logger.IsEnabled(expectedLevel).Returns(true);
+
+        var hub = new Hub(new SentryOptions
+        {
+            Dsn = DsnSamples.ValidDsnWithSecret,
+            CacheDirectoryPath = cachePath, // To go through a round trip serialization of cached envelope
+            RequestBodyCompressionLevel = CompressionLevel.NoCompression, //  So we don't need to deal with gzip'ed payload
+            CreateHttpClientHandler = () => new CallbackHttpClientHandler(Verify),
+            AutoSessionTracking = false, // Not to send some session envelope
+            Debug = true,
+            DiagnosticLevel = expectedLevel,
+            DiagnosticLogger = logger
+        });
+
+        var expectedContextKey = Guid.NewGuid().ToString();
+        var evt = new SentryEvent
+        {
+            Contexts = { [expectedContextKey] = new EvilContext() },
+            Message = new SentryMessage { Formatted = expectedMessage }
+        };
+
+        hub.CaptureEvent(evt);
+
+        // Synchronizing in the tests to go through the caching and http transports and flushing guarantees persistence only
+        Assert.True(resetEvent.Wait(TimeSpan.FromSeconds(3)), "Event not captured");
+        Assert.True(requests.All(p => p.Contains(expectedContextKey)),
+            "Un-serializable context key should exist");
+
+        logger.Received().Log(expectedLevel, "Failed to serialize object for property '{0}'. Original depth: {1}, current depth: {2}",
+#if NETCOREAPP2_1
+            Arg.Is<TargetInvocationException>(e => e.InnerException.GetType() == typeof(InvalidDataException)),
+#else
+            Arg.Any<InvalidDataException>(),
+#endif
+            Arg.Any<object[]>());
+    }
+
     [Fact]
     public void CaptureEvent_SessionActive_ExceptionReportsError()
     {

diff --git a/test/Sentry.Tests/Internals/JsonTests.cs b/test/Sentry.Tests/Internals/JsonTests.cs
@@ -215,32 +215,13 @@ public void WriteDynamicValue_ClassWithTimeZone_SerializedClassWithTimeZoneInfo(
             Assert.All(expectedSerializedData, expectedData => Assert.Contains(expectedData, serializedString));
         }
 
-        [Fact]
-        public void WriteDynamic_NoLoggerAndError_ThrowsException()
+        class NonSerializableValue
         {
-            //Assert
-            ArgumentNullException expectedException = null;
-
-            using var stream = new MemoryStream();
-            using (var writer = new Utf8JsonWriter(stream))
-            {
-                try
-                {
-                    // Act
-                    writer.WriteDynamic(null, null, null);
-                }
-                catch (ArgumentNullException ex)
-                {
-                    expectedException = ex;
-                }
-            }
-
-            // Assert
-            Assert.NotNull(expectedException);
+            public string Thrower => throw new InvalidDataException();
         }
 
         [Fact]
-        public void WriteDynamic_WithLoggerAndError_LogException()
+        public void WriteDynamic_NonSerializableValue_LogException()
         {
             //Assert
             var logger = Substitute.For<IDiagnosticLogger>();
@@ -250,12 +231,22 @@ public void WriteDynamic_WithLoggerAndError_LogException()
             using var stream = new MemoryStream();
             using (var writer = new Utf8JsonWriter(stream))
             {
+                writer.WriteStartObject();
+
                 // Act
-                writer.WriteDynamic(null, null, logger);
+                writer.WriteDynamic("property_name", new NonSerializableValue(), logger);
+
+                writer.WriteEndObject();
             }
 
             // Assert
-            logger.Received(1).Log(Arg.Is(SentryLevel.Error), Arg.Any<string>(), Arg.Any<ArgumentNullException>(), Arg.Any<object>());
+            logger.Received(1).Log(Arg.Is(SentryLevel.Error), "Failed to serialize object for property '{0}'. Original depth: {1}, current depth: {2}",
+#if NETCOREAPP2_1
+                Arg.Is<TargetInvocationException>(e => e.InnerException.GetType() == typeof(InvalidDataException)),
+#else
+        Arg.Any<InvalidDataException>(),
+#endif
+                Arg.Any<object[]>());
         }
     }
 }