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

Fix context object with circular reference prevents event from being sent #2210

Merged
merged 9 commits into from
Mar 6, 2023

Conversation

mattjohnsonpint
Copy link
Contributor

@mattjohnsonpint mattjohnsonpint commented Mar 4, 2023

When a circular reference exists in an object being added to an event as contexts or extras, the SDK will now do the following:

  • If the object graph can be serialized without error, and without reaching the default max depth limit set by System.Text.Json (depth = 64), then it will be serialized normally. This is covers the majority of cases.
  • If serializing the object graph would error due to exceeding the max depth, then we will retry while using the ReferenceHandler.Preserve option provided by System.Text.Json. This results in a much smaller object graph, but one that has been tagged with $id and $ref metadata instead of materializing the references.

To opt out of this behavior, set SentryOptions.JsonPreserveReferences = false. If serialization were to fail due to exceeding the max depth, then the object graph will be truncated at that depth. This was the intended behavior of the previous implementation, though it had a bug which has now been fixed as well.

In any case, the net result is that such an object no longer prevents the event from being sent to Sentry.

Fixes #2209

Also:

  • I added TimeSpan, DateOnly, and TimeOnly to the list of directly-serializable types, since they were in the same code path.
  • ReferenceHandler.Preserve requires a minimum dependency on STJ 5. We already had 5.0.2 as the min dependency on .NET Framework and .NET Standard, but we are still building for .NET Core 3.0 so we need to bump the STJ dependency there as well. This only affects the netcoreapp3.0 target. .NET 5+ will continue to use the STJ provided with the .NET SDK. (We'll drop these old targets in the next major.)
  • I performed some other minor refactoring along the way, as necessary.

@@ -448,10 +529,19 @@ public static void Deconstruct(this JsonProperty jsonProperty, out string name,
// 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)
Copy link
Contributor Author

@mattjohnsonpint mattjohnsonpint Mar 4, 2023

Choose a reason for hiding this comment

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

This was the actual cause of the original bug. The assumption that the depth won't have changed is true when a property being serialized throws an exception on its own, but is false when an exception occurs for reaching max depth.

@mattjohnsonpint mattjohnsonpint merged commit 67c3f44 into main Mar 6, 2023
@mattjohnsonpint mattjohnsonpint deleted the fix/circular-reference-contexts branch March 6, 2023 17:44
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.

Context object with circular reference prevents event from being sent
3 participants