fix(unity): update SentryEvent to have IsCaptured to allow dropping screenshots of filtered events#5162
fix(unity): update SentryEvent to have IsCaptured to allow dropping screenshots of filtered events#5162JoshuaMoelans wants to merge 4 commits intomainfrom
SentryEvent to have IsCaptured to allow dropping screenshots of filtered events#5162Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5162 +/- ##
=======================================
Coverage 74.05% 74.06%
=======================================
Files 501 501
Lines 18111 18113 +2
Branches 3521 3521
=======================================
+ Hits 13413 13415 +2
Misses 3838 3838
Partials 860 860 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if (CaptureEnvelope(envelope)) | ||
| { | ||
| #if SENTRY_UNITY | ||
| @event.WasCaptured = true; // See SentryEvent.unity.cs for more details. |
There was a problem hiding this comment.
thought: alternative
As an alternative, we could use our "Fused-Data" infrastructure, based on ConditionalWeakTable<,>.
e.g.
/* here */
@event.SetFused(new EnvelopeCaptureStatus(true));
/* in sentry-dotnet's ScreenshotEventProcessor */
if (a.GetFused<EnvelopeCaptureStatus>() is { IsCaptured: true })
{
// Data found in CWT ... process and capture screenshot
}
else
{
// no Data found in CWT ... stop processing the screenshot
}
/* new file in src/Sentry/Platforms/Unity/ */
#if SENTRY_UNITY
namespace Sentry.Internal;
internal sealed class EnvelopeCaptureStatus
{
public EnvelopeCaptureStatus(bool isCaptured)
{
IsCaptured = isCaptured;
}
public bool IsCaptured { get; }
}
#endifBut to be fair ... now after manifesting this thought,
the "physical" property is just as good,
since it's internal and Unity-only anyway,
and also more navigable within an IDE.
I'd still like to hear feedback, though 😉
There was a problem hiding this comment.
Hmm yeah that is an option, but I'm a bit more drawn towards the (maybe less elegant-) simpler "physical" property solution for now. Since it doesn't introduce any potential future breakage of a public API, it'll be easy to replace it with something better at relatively low maintenance cost in the meantime.
….com/getsentry/sentry-dotnet into joshua/fix/unity_screenshot_handling
SentryEvent to have WasCaptured to allow dropping screenshots of filtered eventsSentryEvent to have IsCaptured to allow dropping screenshots of filtered events
As suggested in getsentry/sentry-unity#2642 , we add a flag so we can read from it during the CaptureScreenshotCoroutine. We gate it behind
#if SENTRY_UNITYto not expose it for the other .NET SDK consumers.As described in the 'Future Ideas' section of the unity PR, at some point we might want to provide sentry-unity with a wrapper
ISentryClientwhich can overwrite methods, to avoid needing changes in .NET for unity specific stuff like we need to do in this PR.