CaptureFeedback now returns a SentryId #4613

jamescrosswell · 2025-10-09T01:48:45Z

SentrySdk.CaptureFeedback now returns a SentryId that indicates either the id of the newly captured feedback (when the call succeeds) or SentryId.Empty when capturing feedback fails.

Resolves #4319

Add return value to CaptureFeedback #4319

Note

I could see SDK users wanting to know what the error is, if capturing fails... so maybe we need to build a mechanism in to allow this? Perhaps we should be returning a more complex type than just SentryId then... some CaptureFeedbackResult type or something that includes success/failure and optionally an error code and/or message.

Resolves #4319 - #4319

jamescrosswell · 2025-10-09T02:03:28Z

src/Sentry/ISentryClient.cs

    /// <param name="hint">An optional hint providing high level context for the source of the event</param>
-    public void CaptureFeedback(SentryFeedback feedback, Scope? scope = null, SentryHint? hint = null);
+    /// <returns>The SentryId of the captured feedback, if successful. SentryId.Empty if feedback capture fails.</returns>
+    public SentryId CaptureFeedback(SentryFeedback feedback, Scope? scope = null, SentryHint? hint = null);


Arguably this could be called TryCaptureFeedback now but I don't think that's consistent with the other SDKs (or the CaptureEvent method)... so for consistency I've left it as is.

I was also wondering about the other SentryId-returning Capture*-methods 🤔

Both other SentryId-returning Capture*-methods

SentryId CaptureEvent

SentryId CaptureCheckIn

do return SentryId.Empty when unsuccessful.

If I get that right, then bool Try*-methods should communicate the "main-exceptional" flow when returning false, not every reason for completing unsuccessfully.
E.g. Dictionary<TKey, TValue>.TryAdd only returns false when "An item with the same key has already been added.". But it still throws if null is passed as a key.

What would be the "main-error" that TryCaptureFeedback would communicate?

Hub not enabled

Message is null or empty

Envelope not enqueued

TryCaptureEvent would also have multiple failure cases

Event is null

Event dropped via Exception-Filters

Event dropped via Event-Processor

Event dropped via SetBeforeSend

Event dropped by sampling

Event not enqueued

Hmm ... we could return something like a "CaptureEventStatus" struct or enum.
Similar to BCL methods returning OperationStatus when it comes to buffers.

Or am I dramatically overthinking this?

Also ... I guess we're a bit inconsistent with void vs bool, too:

void CaptureTransaction

void CaptureSession

Could (or should) these be bool-returning as well?

Could (or should) these be bool-returning as well?

I think @bitsandfoxes wanted a SentryId on this, since it's not just SDK user facing but SDK user's user facing... so the SDK user could actually want to report success/failure of this method to the app user.

That's probably not the case for CaptureTransaction or CaptureSession. The SDK user is highly unlikely to do anything with any hypothetical return value for those functions. They're really just best effort.

What would be the "main-error" that TryCaptureFeedback would communicate?

This is probably the main one:

sentry-dotnet/src/Sentry/SentryClient.cs

Lines 91 to 95 in 0cda3df

if (string.IsNullOrEmpty(feedback.Message))

{

_options.LogWarning("Feedback dropped due to empty message.");

return SentryId.Empty;

}

DisabledHub is possible, if an empty string was used for the DSN when initialising the SDK.

I think failure to enqueue the envelope would only happen if we'd disposed of the SentryClient (which would only happen when shutting down the app)... so a vanishingly small chance of that happening when the app is capturing user feedback (and no chance to do anything with that error code if/when it did happen).

I'd argue to not name it Try* but instead that the static API is mostly intended as fire & forget. The return values are not intended to be used to control the flow or have the option to recover from a failure.

Consider the following

User calls CaptureEvent

Capture "fails" due to: SDK not initialized, dropped due to filtering, beforeSend, processor?

What do we expect the user to do now?

If they need the ID for some reason they should have it, but prepending the capture methods with a Try* implies some sort of recovery mechanism to me that does not exist.

@bitsandfoxes agree re the method name.

Is there any value in changing the result type from SentryId to CaptureFeedbackResult (to be able to distinguish failure reasons) or is this overkill do you think? See this comment above - I think there are only really two possible reasons for failure.

codecov · 2025-10-09T02:09:11Z

Codecov Report

❌ Patch coverage is 73.33333% with 4 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (version6@33615f4). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/Sentry/Extensibility/DisabledHub.cs	0.00%	2 Missing ⚠️
src/Sentry/Internal/Hub.cs	66.66%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             version6    #4613   +/-   ##
===========================================
  Coverage            ?   73.10%           
===========================================
  Files               ?      479           
  Lines               ?    17388           
  Branches            ?     3435           
===========================================
  Hits                ?    12711           
  Misses              ?     3818           
  Partials            ?      859

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…pture-feedback-return

test/Sentry.Tests/SentryClientTests.cs

test/Sentry.Tests/HubTests.cs

bitsandfoxes · 2025-10-10T10:26:29Z

src/Sentry/SentryClient.cs

    {
+        if (_isDisposed == 1)
+        {
+            _options.LogWarning("Enqueue envelope failed: disposed client");


Suggested change

_options.LogWarning("Enqueue envelope failed: disposed client");

_options.LogWarning("Enqueue envelope failed. Client is disposed.");

src/Sentry/SentryClient.cs

bitsandfoxes · 2025-10-10T10:36:54Z

src/Sentry/ISentryClient.cs

    /// <param name="hint">An optional hint providing high level context for the source of the event</param>
-    public void CaptureFeedback(SentryFeedback feedback, Scope? scope = null, SentryHint? hint = null);
+    /// <returns>The SentryId of the captured feedback, if successful. SentryId.Empty if feedback capture fails.</returns>
+    public SentryId CaptureFeedback(SentryFeedback feedback, Scope? scope = null, SentryHint? hint = null);


I'd argue to not name it Try* but instead that the static API is mostly intended as fire & forget. The return values are not intended to be used to control the flow or have the option to recover from a failure.

Consider the following

User calls CaptureEvent

Capture "fails" due to: SDK not initialized, dropped due to filtering, beforeSend, processor?

What do we expect the user to do now?

If they need the ID for some reason they should have it, but prepending the capture methods with a Try* implies some sort of recovery mechanism to me that does not exist.

bitsandfoxes · 2025-10-10T10:38:15Z

This looks great from the SDK for Unity POV.

jamescrosswell added 2 commits October 9, 2025 14:46

CaptureFeedback now returns a SentryId

1a76177

Resolves #4319 - #4319

Changelog

98bf2f6

jamescrosswell commented Oct 9, 2025

View reviewed changes

Update ApiApprovalTests.Run.Net4_8.verified.txt

f58417b

jamescrosswell marked this pull request as ready for review October 9, 2025 05:50

jamescrosswell requested a review from Flash0ver as a code owner October 9, 2025 05:50

This comment was marked as outdated.

Sign in to view

jamescrosswell added 2 commits October 9, 2025 21:21

Fixed client disposal

04e9a56

Merge remote-tracking branch 'origin/capture-feedback-return' into ca…

0cda3df

…pture-feedback-return

cursor bot reviewed Oct 9, 2025

View reviewed changes

test/Sentry.Tests/SentryClientTests.cs Show resolved Hide resolved

test/Sentry.Tests/HubTests.cs Show resolved Hide resolved

Flash0ver requested a review from bitsandfoxes October 10, 2025 09:03

bitsandfoxes reviewed Oct 10, 2025

View reviewed changes

jamescrosswell requested a review from bitsandfoxes October 12, 2025 20:54

	if (string.IsNullOrEmpty(feedback.Message))
	{
	_options.LogWarning("Feedback dropped due to empty message.");
	return SentryId.Empty;
	}

	_options.LogWarning("Enqueue envelope failed: disposed client");
	_options.LogWarning("Enqueue envelope failed. Client is disposed.");

Uh oh!

CaptureFeedback now returns a SentryId #4613

Are you sure you want to change the base?

CaptureFeedback now returns a SentryId #4613

Conversation

jamescrosswell commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamescrosswell Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

Flash0ver Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Flash0ver Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

jamescrosswell Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitsandfoxes Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

jamescrosswell Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

bitsandfoxes Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bitsandfoxes Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

bitsandfoxes commented Oct 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jamescrosswell commented Oct 9, 2025 •

edited

Loading

jamescrosswell Oct 10, 2025 •

edited

Loading

jamescrosswell Oct 10, 2025 •

edited

Loading

codecov bot commented Oct 9, 2025 •

edited

Loading