search

getsentry / sentry-dotnet

Sentry SDK for .NET
https://docs.sentry.io/platforms/dotnet
MIT License
579 stars 206 forks source link

invalid event envelope. Error causes: unexpected end of file. If Context is null #1332

Closed lucas-zimerman closed 1 year ago

lucas-zimerman commented 2 years ago

Environment

How do you use Sentry? SaaS and Self Hosted.

Which SDK and version? latest.

Steps to Reproduce

Only happens with the caching feature.

run the snippet

using System;
using System.Globalization;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
using Sentry;
using Sentry.Infrastructure;

SentrySdk.Init(o =>
{
    o.Dsn = "https://eb18e953812b41c3aeb042e666fd3b5c@o447951.ingest.sentry.io/5428537";
    o.Debug = true;
    o.DiagnosticLogger = new TraceDiagnosticLogger(SentryLevel.Debug); //only used for revealing the data on the IDE.
    o.CacheDirectoryPath = "D:/bob";
});

SentrySdk.ConfigureScope(scope =>
{
    scope.Contexts["MyCoolContext"] = null;
    scope.Contexts["MyCoolContext2"] = null;
});
SentrySdk.CaptureMessage("Hello Wôrld");
await Task.Delay(1000);

Expected Result

An event sent to Sentry.

Actual Result

  Error: Sentry rejected the envelope 3e2862cff51d43b3860e4513ef522764. Status code: BadRequest. Error detail: invalid event envelope. Error causes: unexpected end of file.
  Debug: Failed envelope '3e2862cff51d43b3860e4513ef522764' has payload:
{"sdk":{"name":"Sentry","version":"3.12.0-alpha.1"},"event_id":"3e2862cff51d43b3860e4513ef522764"}
{"type":"event","length":1965}
{"modules":{"System.Private.CoreLib":"5.0.0.0","Sentry.Samples.Console.Basic":"9.8.7.0","System.Runtime":"5.0.0.0","Sentry":"3.12.0.0","System.Net.Primitives":"5.0.0.0","System.IO.Compression":"5.0.0.0","System.Private.Uri":"5.0.0.0","System.Diagnostics.Process":"5.0.0.0","System.ComponentModel.Primitives":"5.0.0.0","Microsoft.Win32.Primitives":"5.0.0.0","System.Runtime.InteropServices":"5.0.0.0","System.Threading":"5.0.0.0","System.Diagnostics.TraceSource":"5.0.0.0","System.Net.Http":"5.0.0.0","System.Diagnostics.Tracing":"5.0.0.0","System.Diagnostics.DiagnosticSource":"5.0.0.0","System.Net.Security":"5.0.0.0","System.Security.Cryptography.X509Certificates":"5.0.0.0","System.Collections":"5.0.0.0","System.Collections.Concurrent":"5.0.0.0","System.Security.Cryptography.Algorithms":"5.0.0.0","System.Security.Cryptography.Primitives":"5.0.0.0","System.Memory":"5.0.0.0","System.IO.FileSystem":"5.0.0.0","System.Linq":"5.0.0.0","System.Threading.Thread":"5.0.0.0","System.Diagnostics.StackTrace":"5.0.0.0"},"event_id":"3e2862cff51d43b3860e4513ef522764","timestamp":"2021-11-19T21:00:13.8001629+00:00","logentry":{"message":"Hello W\u00F4rld"},"platform":"csharp","release":"Sentry.Samples.Console.Basic@9.8.7","level":"info","request":{},"contexts":{"app":{"type":"app","app_start_time":"2021-11-19T21:00:12.4422617+00:00"},"device":{"type":"device","timezone":"E. South America Standard Time","timezone_display_name":"(UTC-03:00) Brasilia","boot_time":"2021-11-17T17:50:47.9937672+00:00"},"runtime":{"type":"runtime","name":".NET","version":"5.0.10","raw_description":".NET 5.0.10"},"Current Culture":{"Name":"en-US","DisplayName":"English (United States)","Calendar":"GregorianCalendar"},"os":{"type":"os","raw_description":"Microsoft Windows 10.0.22000"}},"user":{},"environment":"debug","sdk":{"packages":[{"name":"nuget:Sentry","version":"3.12.0-alpha.1"}],"name":"sentry.dotnet","version":"3.12.0-alpha.1"}}
SimonCropp commented 2 years ago

@lucas-zimerman i cant seem to reproduce this. does it still happen for you?

mattjohnsonpint commented 2 years ago

I can reproduce it with latest 3.20.1, though with some minor changes:

    o.Dsn = "https://eb18e953812b41c3aeb042e666fd3b5c@o447951.ingest.sentry.io/5428537";
    o.Debug = true;
    o.CacheDirectoryPath = "bin/temp";
  Error: Sentry rejected the envelope 825ebfa763dd420a99243523a0165d74. Status code: BadRequest. Error detail: invalid event envelope. Error causes: unexpected end of file.
  Debug: Failed envelope '825ebfa763dd420a99243523a0165d74' has payload: {"sdk":{"name":"sentry.dotnet","version":"3.20.1"},"event_id":"825ebfa763dd420a99243523a0165d74","sent_at":"2022-08-04T15:05:38.072424+00:00"} {"type":"event","length":3239} {"modules":{"System.Private.CoreLib":"6.0.0.0","ConsoleApp16":"1.0.0.0","System.Runtime":"6.0.0.0","JetBrains.Microsoft.Extensions.DotNetDeltaApplier":"1.0.0.0","System.Linq":"6.0.0.0","System.Collections":"6.0.0.0","System.Collections.Concurrent":"6.0.0.0","Sentry":"3.20.1.0","System.Net.Primitives":"6.0.0.0","System.IO.Compression":"6.0.0.0","System.IO.Pipes":"6.0.0.0","System.Console":"6.0.0.0","System.Threading":"6.0.0.0","Microsoft.Win32.Primitives":"6.0.0.0","System.Net.Sockets":"6.0.0.0","System.Diagnostics.Tracing":"6.0.0.0","System.Threading.ThreadPool":"6.0.0.0","System.Runtime.InteropServices.RuntimeInformation":"6.0.0.0","System.Threading.Thread":"6.0.0.0","System.Memory":"6.0.0.0","System.Runtime.Loader":"6.0.0.0","System.Diagnostics.Process":"6.0.0.0","System.Private.Uri":"6.0.0.0","System.ComponentModel.Primitives":"6.0.0.0","System.Net.Http":"6.0.0.0","System.Diagnostics.DiagnosticSource":"6.0.0.0","System.Net.Security":"6.0.0.0","System.Security.Cryptography.X509Certificates":"6.0.0.0","System.Security.Cryptography.Algorithms":"6.0.0.0","System.Security.Cryptography.Primitives":"6.0.0.0","System.Net.Requests":"6.0.0.0","System.Text.Json":"6.0.0.0","System.Numerics.Vectors":"6.0.0.0","System.Runtime.CompilerServices.Unsafe":"6.0.0.0","System.Text.Encoding.Extensions":"6.0.0.0","System.ObjectModel":"6.0.0.0","System.Text.Encodings.Web":"6.0.0.0","System.Runtime.Intrinsics":"6.0.0.0","System.Runtime.InteropServices":"6.0.0.0","System.Net.NameResolution":"6.0.0.0","System.Collections.NonGeneric":"6.0.0.0","System.Security.Cryptography.Encoding":"6.0.0.0","System.Formats.Asn1":"6.0.0.0","System.Runtime.Numerics":"6.0.0.0","System.Diagnostics.StackTrace":"6.0.0.0"},"event_id":"825ebfa763dd420a99243523a0165d74","timestamp":"2022-08-04T15:05:37.940945+00:00","logentry":{"message":"Hello W\u00F4rld"},"platform":"csharp","release":"ConsoleApp16@1.0.0","level":"info","request":{},"contexts":{"Current Culture":{"name":"en-US","display_name":"English (United States)","calendar":"GregorianCalendar"},"ThreadPool Info":{"min_worker_threads":10,"min_completion_port_threads":10,"max_worker_threads":32767,"max_completion_port_threads":1000,"available_worker_threads":32766,"available_completion_port_threads":1000},"Dynamic Code":{"Compiled":true,"Supported":true},"os":{"type":"os","raw_description":"Darwin 21.5.0 Darwin Kernel Version 21.5.0: Tue Apr 26 21:08:37 PDT 2022; root:xnu-8020.121.3~4/RELEASE_ARM64_T6000"},"device":{"type":"device","timezone":"America/Los_Angeles","timezone_display_name":"(UTC-08:00) Pacific Time (Los Angeles)","boot_time":"2022-07-24T03:18:01.8133283+00:00"},"runtime":{"type":"runtime","name":".NET","version":"6.0.7","raw_description":".NET 6.0.7","identifier":"osx.12-arm64"},"Memory Info":{"allocated_bytes":2242296,"high_memory_load_threshold_bytes":30923764531,"total_available_memory_bytes":34359738368,"finalization_pending_count":0,"compacted":false,"concurrent":false,"pause_durations":[0,0]},"app":{"type":"app","app_start_time":"2022-08-04T15:05:37.28985+00:00"}},"user":{},"environment":"production","sdk":{"packages":[{"name":"nuget:sentry.dotnet","version":"3.20.1"}],"name":"sentry.dotnet","version":"3.20.1"}}
mattjohnsonpint commented 2 years ago

It would appear that the null entries are removed from the context section of the payload when caching is enabled, and the length header is not updated.

We should ensure first that the length header is always correct, but also we should decide whether to either send null context info always or never. It shouldn't vary based on caching.

SimonCropp commented 2 years ago

The server-side accepts nulls for context values. but they are not show in the UI

@mattjohnsonpint @bruno-garcia can you check if the above is by design? Seems to me we should never hide information about an event??

Assuming it is by design, should we:

1 Accept and ignore null 2 Accept and send nulls.

Note the above two should mean changing Contexts to inherit from a ConcurrentDictionary<string, object?>. given with nullable reference types enabled the above sample will not compile

3 Throw when a null value is set

SimonCropp commented 2 years ago

assuming we go with 2. here is a PR https://github.com/getsentry/sentry-dotnet/pull/1932

mattjohnsonpint commented 2 years ago

On further thought - Null contexts don't make much sense. Context is extra information at the time of the event, such as the OS name or the amount of memory available, etc.

We define Contexts as extending from ConcurrentDictionary<string, object>, so if you use it in code where nullability checks are enabled (<Nullable>enable</Nullable>) the compiler will give a warning:

image image

The problem is that one could easily be using Sentry without having nullability checks enabled. So basically, any public API that exposes a class parameter has to deal with the possibility of the user passing null, even if it's been marked as not-null.

SO - In this case, we want to not accept nulls, and also ignore them. (variation on option 1)

Throwing (option 3) would be ok in some apps, but not here because we have a general principal of Sentry not itself being the cause of an issue. Say a user was passing in custom context, and it usually was not null. Throwing on null would surface to the end-user, and would prevent the error from being sent to Sentry. So, better to silently ignore.

In summary - the API surface shouldn't change here, we should just make sure we filter out nulls from context values and the length header should correctly reflect that appropriately so the envelope sends without failure.

mattjohnsonpint commented 2 years ago

Also I'll just add that it would have been a better design to encapsulate the ConcurrentDictionary rather than extending from it. Contexts : IDictionary<string, object> would have allowed us to filter out nulls when data was being added, rather than afterwards. It would have also hidden the concurrentness, which is an implementation detail. But changing it now would be a breaking change, so perhaps for the future.