feat: BeforeCaptureScreenshot option#3661
Conversation
|
|
I did add a change log, maybe I did it wrong 🤔 updateit was an indentation issue, my bad |
a97e7a3 to
c453695
Compare
bitsandfoxes
left a comment
There was a problem hiding this comment.
Fwiw, PR titles we loosely follow a feat:, fix:, chore: convention. No need to add the issue number.
|
@romtsn how do we deal with this on Android for example? |
|
Looks like we tried binding from Android but we never added Attachment on the .NET hint: |
|
From in this PR: This:
But unfortunately no docs anywhere to be found |
jamescrosswell
left a comment
There was a problem hiding this comment.
The code looks good. I made a few styling suggestions.
I'm not entirely sure what this new option allows SDK users to do that they can't already do with SentryOptions.SetBeforeSend... The Android docs state:
Because capturing screenshots can be a resource-intensive operation on Android, it's limited to one screenshot every 2 seconds using a debouncing mechanism. This behavior can be overruled if you supply a BeforeCaptureCallback for screenshots in the SentryAndroidOptions.
The BeforeCaptureCallback also allows you to customize the behavior based on event data, so you can decide when to capture a screenshot and when not to. For example, you can decide to only capture screenshots of crashed and fatal events:
But it doesn't look like in the .NET implementation we're using the result of this callback in any way to determine whether a screenshot is added or not in SentryMauiScreenshotProcessor.cs.
I think it'd be worth adding an example of the new API to one of our samples, demonstrating how you can do something with it that can't already be achieved using SetBeforeSend. That'd be a good sanity check for us, as maintainers (to ensure we're building something meaningful), and would help demonstrate the value of the new functionality to SDK users.
Co-authored-by: James Crosswell <jamescrosswell@users.noreply.github.com>
we've added this callback because FWIW, our callback does not return an event but a boolean that defines whether the screenshot should be captured or not, see: https://github.com/getsentry/sentry-java/blob/b5b093e5f805d0d02c7be344403804e7a7605398/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java#L77-L88 |
…/getsentry/sentry-dotnet into feat/programmatic_sc_attachement
…oreCaptureScreenshotInternal invocation Co-authored-by: James Crosswell <jamescrosswell@users.noreply.github.com>
jamescrosswell
left a comment
There was a problem hiding this comment.
Just a couple of notes about comments but otherwise looks good, as long as CI is happy 👍🏻
It would be good to add some docs on this to the Sentry Docs as well.
Co-authored-by: James Crosswell <jamescrosswell@users.noreply.github.com>
Co-authored-by: James Crosswell <jamescrosswell@users.noreply.github.com>
537b91e to
4517762
Compare
UpdateThe callback works similarly to the Android SDK. It returns a boolean that determines whether to ignore the capture.
(details) |
jamescrosswell
left a comment
There was a problem hiding this comment.
Functionally, looks good. I think just tweak the tests...
Relevant to #3353, I added a option to have a callback just before attaching a screenshot.
Note
I didn't had a closing keyword to this PR, as I don't believe that change alone fixes the core issue. However this change was demanded