Skip to content

CaptureFeedback now returns a SentryId #4613

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: version6
Choose a base branch
from
+93 −62
Open

CaptureFeedback now returns a SentryId #4613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1a76177
CaptureFeedback now returns a SentryId
jamescrosswell Oct 9, 2025
98bf2f6
Changelog
jamescrosswell Oct 9, 2025
f58417b
Update ApiApprovalTests.Run.Net4_8.verified.txt
jamescrosswell Oct 9, 2025
04e9a56
Fixed client disposal
jamescrosswell Oct 9, 2025
0cda3df
Merge remote-tracking branch 'origin/capture-feedback-return' into ca…
jamescrosswell Oct 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

- SentryOptions.IsEnvironmentUser now defaults to false on MAUI. The means the User.Name will no longer be set, by default, to the name of the device ([#4606](https://github.com/getsentry/sentry-dotnet/pull/4606))
- Remove unnecessary files from SentryCocoaFramework before packing ([#4602](https://github.com/getsentry/sentry-dotnet/pull/4602))
- CaptureFeedback now returns a SentryId that can be used to determine whether the call succeeded or failed ([#4613](https://github.com/getsentry/sentry-dotnet/pull/4613))

## 6.0.0-preview.1

Expand Down
6 changes: 4 additions & 2 deletions src/Sentry/Extensibility/DisabledHub.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,15 +176,17 @@ public bool CaptureEnvelope(Envelope envelope)
/// <summary>
/// No-Op.
/// </summary>
public void CaptureFeedback(SentryFeedback feedback, Action<Scope> configureScope, SentryHint? hint = null)
public SentryId CaptureFeedback(SentryFeedback feedback, Action<Scope> configureScope, SentryHint? hint = null)
{
return SentryId.Empty;
}

/// <summary>
/// No-Op.
/// </summary>
public void CaptureFeedback(SentryFeedback feedback, Scope? scope = null, SentryHint? hint = null)
public SentryId CaptureFeedback(SentryFeedback feedback, Scope? scope = null, SentryHint? hint = null)
{
return SentryId.Empty;
}

/// <summary>
Expand Down
4 changes: 2 additions & 2 deletions src/Sentry/Extensibility/HubAdapter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,15 +260,15 @@ public SentryId CaptureEvent(SentryEvent evt, Scope? scope, SentryHint? hint = n
/// </summary>
[DebuggerStepThrough]
[EditorBrowsable(EditorBrowsableState.Never)]
public void CaptureFeedback(SentryFeedback feedback, Action<Scope> configureScope, SentryHint? hint = null)
public SentryId CaptureFeedback(SentryFeedback feedback, Action<Scope> configureScope, SentryHint? hint = null)
=> SentrySdk.CaptureFeedback(feedback, configureScope, hint);

/// <summary>
/// Forwards the call to <see cref="SentrySdk"/>.
/// </summary>
[DebuggerStepThrough]
[EditorBrowsable(EditorBrowsableState.Never)]
public void CaptureFeedback(SentryFeedback feedback, Scope? scope = null, SentryHint? hint = null)
public SentryId CaptureFeedback(SentryFeedback feedback, Scope? scope = null, SentryHint? hint = null)
=> SentrySdk.CaptureFeedback(feedback, scope, hint);

/// <summary>
Expand Down
3 changes: 2 additions & 1 deletion src/Sentry/IHub.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,5 +135,6 @@ public TransactionContext ContinueTrace(
/// <param name="feedback">The feedback to send to Sentry.</param>
/// <param name="configureScope">Callback method to configure the scope.</param>
/// <param name="hint">An optional hint providing high level context for the source of the event, including attachments</param>
public void CaptureFeedback(SentryFeedback feedback, Action<Scope> configureScope, SentryHint? hint = null);
/// <returns>The SentryId of the captured feedback, if successful. SentryId.Empty if feedback capture fails.</returns>
public SentryId CaptureFeedback(SentryFeedback feedback, Action<Scope> configureScope, SentryHint? hint = null);
}
3 changes: 2 additions & 1 deletion src/Sentry/ISentryClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ public interface ISentryClient
/// <param name="feedback">The feedback to send to Sentry.</param>
/// <param name="scope">An optional scope to be applied to the event.</param>
/// <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);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@jamescrosswell jamescrosswell Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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

  1. User calls CaptureEvent
  2. Capture "fails" due to: SDK not initialized, dropped due to filtering, beforeSend, processor?
  3. 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.

Copy link
Collaborator Author

@jamescrosswell jamescrosswell Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice but I don't see this as a requirement. We've got that pattern all over the rest of the SDK. I think it's fine if we document it. I can do that together with the bump in the Unity SDK.


/// <summary>
/// Captures a user feedback.
Expand Down
14 changes: 8 additions & 6 deletions src/Sentry/Internal/Hub.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,31 +589,32 @@ private SentryId CaptureEvent(SentryEvent evt, SentryHint? hint, Scope scope)
}
}

public void CaptureFeedback(SentryFeedback feedback, Action<Scope> configureScope, SentryHint? hint = null)
public SentryId CaptureFeedback(SentryFeedback feedback, Action<Scope> configureScope, SentryHint? hint = null)
{
if (!IsEnabled)
{
return;
return SentryId.Empty;
}

try
{
var clonedScope = CurrentScope.Clone();
configureScope(clonedScope);

CaptureFeedback(feedback, clonedScope, hint);
return CaptureFeedback(feedback, clonedScope, hint);
}
catch (Exception e)
{
_options.LogError(e, "Failure to capture feedback");
return SentryId.Empty;
}
}

public void CaptureFeedback(SentryFeedback feedback, Scope? scope = null, SentryHint? hint = null)
public SentryId CaptureFeedback(SentryFeedback feedback, Scope? scope = null, SentryHint? hint = null)
{
if (!IsEnabled)
{
return;
return SentryId.Empty;
}

try
Expand All @@ -625,11 +626,12 @@ public void CaptureFeedback(SentryFeedback feedback, Scope? scope = null, Sentry
}

scope ??= CurrentScope;
CurrentClient.CaptureFeedback(feedback, scope, hint);
return CurrentClient.CaptureFeedback(feedback, scope, hint);
}
catch (Exception e)
{
_options.LogError(e, "Failure to capture feedback");
return SentryId.Empty;
}
}

Expand Down
18 changes: 15 additions & 3 deletions src/Sentry/SentryClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ public class SentryClient : ISentryClient, IDisposable
private readonly ISessionManager _sessionManager;
private readonly RandomValuesFactory _randomValuesFactory;
private readonly Enricher _enricher;
private volatile int _isDisposed = 0;

internal IBackgroundWorker Worker { get; }

Expand Down Expand Up @@ -85,12 +86,12 @@ public SentryId CaptureEvent(SentryEvent? @event, Scope? scope = null, SentryHin
}

/// <inheritdoc />
public void CaptureFeedback(SentryFeedback feedback, Scope? scope = null, SentryHint? hint = null)
public SentryId CaptureFeedback(SentryFeedback feedback, Scope? scope = null, SentryHint? hint = null)
{
if (string.IsNullOrEmpty(feedback.Message))
{
_options.LogWarning("Feedback dropped due to empty message.");
return;
return SentryId.Empty;
}

scope ??= new Scope(_options);
Expand All @@ -116,7 +117,7 @@ public void CaptureFeedback(SentryFeedback feedback, Scope? scope = null, Sentry

var attachments = hint.Attachments.ToList();
var envelope = Envelope.FromFeedback(evt, _options.DiagnosticLogger, attachments, scope.SessionUpdate);
CaptureEnvelope(envelope);
return CaptureEnvelope(envelope) ? evt.EventId : SentryId.Empty;
}

/// <inheritdoc />
Expand Down Expand Up @@ -438,6 +439,12 @@ private SentryId DoSendEvent(SentryEvent @event, SentryHint? hint, Scope? scope)
/// <inheritdoc cref="ISentryClient.CaptureEnvelope"/>
public bool CaptureEnvelope(Envelope envelope)
{
if (_isDisposed == 1)
{
_options.LogWarning("Enqueue envelope failed: disposed client");
Copy link
Contributor

Choose a reason for hiding this comment

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

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

return false;
}

if (Worker.EnqueueEnvelope(envelope))
{
_options.LogInfo("Envelope queued up: '{0}'", envelope.TryGetEventId(_options.DiagnosticLogger));
Expand All @@ -456,6 +463,11 @@ public bool CaptureEnvelope(Envelope envelope)
/// </summary>
public void Dispose()
{
if (Interlocked.Exchange(ref _isDisposed, 1) == 1)
{
return;
}

_options.LogDebug("Flushing SentryClient.");

try
Expand Down
4 changes: 2 additions & 2 deletions src/Sentry/SentrySdk.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,14 +546,14 @@ public static SentryId CaptureMessage(string message, Action<Scope> configureSco
/// Captures feedback from the user.
/// </summary>
[DebuggerStepThrough]
public static void CaptureFeedback(SentryFeedback feedback, Action<Scope> configureScope, SentryHint? hint = null)
public static SentryId CaptureFeedback(SentryFeedback feedback, Action<Scope> configureScope, SentryHint? hint = null)
=> CurrentHub.CaptureFeedback(feedback, configureScope, hint);

/// <summary>
/// Captures feedback from the user.
/// </summary>
[DebuggerStepThrough]
public static void CaptureFeedback(SentryFeedback feedback, Scope? scope = null, SentryHint? hint = null)
public static SentryId CaptureFeedback(SentryFeedback feedback, Scope? scope = null, SentryHint? hint = null)
=> CurrentHub.CaptureFeedback(feedback, scope, hint);

/// <summary>
Expand Down
18 changes: 9 additions & 9 deletions test/Sentry.Tests/ApiApprovalTests.Run.DotNet10_0.verified.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ namespace Sentry
void BindException(System.Exception exception, Sentry.ISpan span);
Sentry.SentryId CaptureEvent(Sentry.SentryEvent evt, System.Action<Sentry.Scope> configureScope);
Sentry.SentryId CaptureEvent(Sentry.SentryEvent evt, Sentry.SentryHint? hint, System.Action<Sentry.Scope> configureScope);
void CaptureFeedback(Sentry.SentryFeedback feedback, System.Action<Sentry.Scope> configureScope, Sentry.SentryHint? hint = null);
Sentry.SentryId CaptureFeedback(Sentry.SentryFeedback feedback, System.Action<Sentry.Scope> configureScope, Sentry.SentryHint? hint = null);
Sentry.TransactionContext ContinueTrace(Sentry.SentryTraceHeader? traceHeader, Sentry.BaggageHeader? baggageHeader, string? name = null, string? operation = null);
Sentry.TransactionContext ContinueTrace(string? traceHeader, string? baggageHeader, string? name = null, string? operation = null);
void EndSession(Sentry.SessionEndStatus status = 0);
Expand All @@ -218,7 +218,7 @@ namespace Sentry
Sentry.SentryId CaptureCheckIn(string monitorSlug, Sentry.CheckInStatus status, Sentry.SentryId? sentryId = default, System.TimeSpan? duration = default, Sentry.Scope? scope = null, System.Action<Sentry.SentryMonitorOptions>? configureMonitorOptions = null);
bool CaptureEnvelope(Sentry.Protocol.Envelopes.Envelope envelope);
Sentry.SentryId CaptureEvent(Sentry.SentryEvent evt, Sentry.Scope? scope = null, Sentry.SentryHint? hint = null);
void CaptureFeedback(Sentry.SentryFeedback feedback, Sentry.Scope? scope = null, Sentry.SentryHint? hint = null);
Sentry.SentryId CaptureFeedback(Sentry.SentryFeedback feedback, Sentry.Scope? scope = null, Sentry.SentryHint? hint = null);
void CaptureSession(Sentry.SessionUpdate sessionUpdate);
void CaptureTransaction(Sentry.SentryTransaction transaction);
void CaptureTransaction(Sentry.SentryTransaction transaction, Sentry.Scope? scope, Sentry.SentryHint? hint);
Expand Down Expand Up @@ -448,7 +448,7 @@ namespace Sentry
public Sentry.SentryId CaptureCheckIn(string monitorSlug, Sentry.CheckInStatus status, Sentry.SentryId? sentryId = default, System.TimeSpan? duration = default, Sentry.Scope? scope = null, System.Action<Sentry.SentryMonitorOptions>? configureMonitorOptions = null) { }
public bool CaptureEnvelope(Sentry.Protocol.Envelopes.Envelope envelope) { }
public Sentry.SentryId CaptureEvent(Sentry.SentryEvent? @event, Sentry.Scope? scope = null, Sentry.SentryHint? hint = null) { }
public void CaptureFeedback(Sentry.SentryFeedback feedback, Sentry.Scope? scope = null, Sentry.SentryHint? hint = null) { }
public Sentry.SentryId CaptureFeedback(Sentry.SentryFeedback feedback, Sentry.Scope? scope = null, Sentry.SentryHint? hint = null) { }
public void CaptureSession(Sentry.SessionUpdate sessionUpdate) { }
public void CaptureTransaction(Sentry.SentryTransaction transaction) { }
public void CaptureTransaction(Sentry.SentryTransaction transaction, Sentry.Scope? scope, Sentry.SentryHint? hint) { }
Expand Down Expand Up @@ -844,8 +844,8 @@ namespace Sentry
public static Sentry.SentryId CaptureEvent(Sentry.SentryEvent evt, Sentry.SentryHint? hint, System.Action<Sentry.Scope> configureScope) { }
public static Sentry.SentryId CaptureException(System.Exception exception) { }
public static Sentry.SentryId CaptureException(System.Exception exception, System.Action<Sentry.Scope> configureScope) { }
public static void CaptureFeedback(Sentry.SentryFeedback feedback, Sentry.Scope? scope = null, Sentry.SentryHint? hint = null) { }
public static void CaptureFeedback(Sentry.SentryFeedback feedback, System.Action<Sentry.Scope> configureScope, Sentry.SentryHint? hint = null) { }
public static Sentry.SentryId CaptureFeedback(Sentry.SentryFeedback feedback, Sentry.Scope? scope = null, Sentry.SentryHint? hint = null) { }
public static Sentry.SentryId CaptureFeedback(Sentry.SentryFeedback feedback, System.Action<Sentry.Scope> configureScope, Sentry.SentryHint? hint = null) { }
public static void CaptureFeedback(string message, string? contactEmail = null, string? name = null, string? replayId = null, string? url = null, Sentry.SentryId? associatedEventId = default, Sentry.Scope? scope = null, Sentry.SentryHint? hint = null) { }
public static Sentry.SentryId CaptureMessage(string message, Sentry.SentryLevel level = 1) { }
public static Sentry.SentryId CaptureMessage(string message, System.Action<Sentry.Scope> configureScope, Sentry.SentryLevel level = 1) { }
Expand Down Expand Up @@ -1388,8 +1388,8 @@ namespace Sentry.Extensibility
public Sentry.SentryId CaptureEvent(Sentry.SentryEvent evt, System.Action<Sentry.Scope> configureScope) { }
public Sentry.SentryId CaptureEvent(Sentry.SentryEvent evt, Sentry.Scope? scope = null, Sentry.SentryHint? hint = null) { }
public Sentry.SentryId CaptureEvent(Sentry.SentryEvent evt, Sentry.SentryHint? hint, System.Action<Sentry.Scope> configureScope) { }
public void CaptureFeedback(Sentry.SentryFeedback feedback, Sentry.Scope? scope = null, Sentry.SentryHint? hint = null) { }
public void CaptureFeedback(Sentry.SentryFeedback feedback, System.Action<Sentry.Scope> configureScope, Sentry.SentryHint? hint = null) { }
public Sentry.SentryId CaptureFeedback(Sentry.SentryFeedback feedback, Sentry.Scope? scope = null, Sentry.SentryHint? hint = null) { }
public Sentry.SentryId CaptureFeedback(Sentry.SentryFeedback feedback, System.Action<Sentry.Scope> configureScope, Sentry.SentryHint? hint = null) { }
public void CaptureSession(Sentry.SessionUpdate sessionUpdate) { }
public void CaptureTransaction(Sentry.SentryTransaction transaction) { }
public void CaptureTransaction(Sentry.SentryTransaction transaction, Sentry.Scope? scope, Sentry.SentryHint? hint) { }
Expand Down Expand Up @@ -1440,8 +1440,8 @@ namespace Sentry.Extensibility
public Sentry.SentryId CaptureEvent(Sentry.SentryEvent evt, Sentry.Scope? scope, Sentry.SentryHint? hint = null) { }
public Sentry.SentryId CaptureEvent(Sentry.SentryEvent evt, Sentry.SentryHint? hint, System.Action<Sentry.Scope> configureScope) { }
public Sentry.SentryId CaptureException(System.Exception exception) { }
public void CaptureFeedback(Sentry.SentryFeedback feedback, Sentry.Scope? scope = null, Sentry.SentryHint? hint = null) { }
public void CaptureFeedback(Sentry.SentryFeedback feedback, System.Action<Sentry.Scope> configureScope, Sentry.SentryHint? hint = null) { }
public Sentry.SentryId CaptureFeedback(Sentry.SentryFeedback feedback, Sentry.Scope? scope = null, Sentry.SentryHint? hint = null) { }
public Sentry.SentryId CaptureFeedback(Sentry.SentryFeedback feedback, System.Action<Sentry.Scope> configureScope, Sentry.SentryHint? hint = null) { }
public void CaptureSession(Sentry.SessionUpdate sessionUpdate) { }
public void CaptureTransaction(Sentry.SentryTransaction transaction) { }
public void CaptureTransaction(Sentry.SentryTransaction transaction, Sentry.Scope? scope, Sentry.SentryHint? hint) { }
Expand Down
Loading
Loading