Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions src/Sentry/CaptureFeedbackErrorReason.cs
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,9 @@ public enum CaptureFeedbackResult
/// The <see cref="SentryFeedback"/> was discarded by an event processor.
/// </summary>
DroppedByEventProcessor,
/// <summary>
/// The <see cref="SentryFeedback"/> was discarded by the <c>BeforeSendFeedback</c> callback
/// either by returning null or by throwing an exception.
/// </summary>
DroppedByBeforeSendFeedback,
}
27 changes: 27 additions & 0 deletions src/Sentry/Internal/SentryEventHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -74,4 +74,31 @@ internal static class SentryEventHelper

return @event;
}

public static SentryEvent? DoBeforeSendFeedback(SentryEvent? @event, SentryHint hint, SentryOptions options)
{
if (@event is null || options.BeforeSendFeedbackInternal is null)
{
return @event;
}

options.LogDebug("Calling the BeforeSendFeedback callback.");
try
{
@event = options.BeforeSendFeedbackInternal.Invoke(@event, hint);
if (@event == null)
{
options.ClientReportRecorder.RecordDiscardedEvent(DiscardReason.BeforeSend, DataCategory.Feedback);
options.LogInfo("Feedback dropped by BeforeSendFeedback callback.");
}
}
catch (Exception e)
{
options.ClientReportRecorder.RecordDiscardedEvent(DiscardReason.BeforeSend, DataCategory.Feedback);
options.LogError(e, "The BeforeSendFeedback callback threw an exception. The feedback will be dropped.");
return null;
}

return @event;
}
}
8 changes: 8 additions & 0 deletions src/Sentry/SentryClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,14 @@ public SentryId CaptureFeedback(SentryFeedback feedback, out CaptureFeedbackResu
return SentryId.Empty; // Dropped by an event processor
}

if (SentryEventHelper.DoBeforeSendFeedback(processedEvent, hint, _options) is not { } feedbackEvent)
{
result = CaptureFeedbackResult.DroppedByBeforeSendFeedback;
return SentryId.Empty;
}

processedEvent = feedbackEvent;

var attachments = hint.Attachments.ToList();
var envelope = Envelope.FromFeedback(processedEvent, _options.DiagnosticLogger, attachments, scope.SessionUpdate);
if (CaptureEnvelope(envelope))
Expand Down
24 changes: 24 additions & 0 deletions src/Sentry/SentryOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,30 @@ public void SetBeforeSendTransaction(Func<SentryTransaction, SentryTransaction?>
_beforeSendTransaction = (transaction, _) => beforeSendTransaction(transaction);
}

private Func<SentryEvent, SentryHint, SentryEvent?>? _beforeSendFeedback;

internal Func<SentryEvent, SentryHint, SentryEvent?>? BeforeSendFeedbackInternal => _beforeSendFeedback;

/// <summary>
/// Configures a callback to invoke before sending user feedback to Sentry.
/// Return null or throw to drop the feedback.
/// </summary>
/// <param name="beforeSendFeedback">The callback</param>
public void SetBeforeSendFeedback(Func<SentryEvent, SentryHint, SentryEvent?> beforeSendFeedback)
{
_beforeSendFeedback = beforeSendFeedback;
}

/// <summary>
/// Configures a callback to invoke before sending user feedback to Sentry.
/// Return null or throw to drop the feedback.
/// </summary>
/// <param name="beforeSendFeedback">The callback</param>
public void SetBeforeSendFeedback(Func<SentryEvent, SentryEvent?> beforeSendFeedback)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

More of a question than a comment, but I'm not sure if we need this overload. Originally we added an overload on the BeforeSend event when we wanted to introduce a variant that accepted a SentryHint parameter without that being a breaking change... we never made a conscious decision that we wanted two overloads of the callback though.

If we only had a single version of the callback that accepted both a SentryEvent and a SentryHint argument, users could easily ignore the second argument.

On the other hand, if we do that now for this callback, SetBeforeSendFeedback is then the odd one out (inconsistent)... so probably not something we want to do in this PR but something we could consider for all of the BeforeSend* callbacks as a breaking change in the next major:

  • Do we want to simplify the BeforeSend* callbacks so that these are not overloaded?

@Flash0ver @vladbrincoveanu what are your thoughts?

@vladbrincoveanu vladbrincoveanu Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These overloads are just adapters. They just use the SentryHint version and their docs hide half the feature.
I agree, we can batch mark them as [Obsolete] for a next minor and remove them in the future major, so users gets some warnings first. This could be another good starter ticket, for cleanups.

3 concerns:

  • the overload without SentryHint is the one used in docs, we need to updated the entries too.
  • by marking them as obsolete first in a minor vers, the consumers either get a warning or their build can actually break if they have TreatWarningsAsErrors set up. Is this acceptable?
  • SetBeforeSendLog and SetBeforeSendMetric do not have 2 overloads and we need to decide if it makes sense to add a SentryHint for logging and metrics.

1 suggestion:

  • another possible doc update: new data for Before callbacks will go into SentryHint, not into a new delegate parameter, so we enforce it. Ofc, if we ever need to add a cancellationtoken or async, we will have a new overload.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

we can batch mark them as [Obsolete] for a next minor and remove them in the future major

Probably more effort that it's worth... The overloads don't add too much noise.

SetBeforeSendLog and SetBeforeSendMetric do not have 2 overloads and we need to decide if it makes sense to add a SentryHint for logging and metrics.

The main reason for the Hint parameter was to make it possible to pass in attachments... which I don't think make sense for metrics/logs (neither support attachments).

{
_beforeSendFeedback = (@event, _) => beforeSendFeedback(@event);
}

private Func<Breadcrumb, SentryHint, Breadcrumb?>? _beforeBreadcrumb;

internal Func<Breadcrumb, SentryHint, Breadcrumb?>? BeforeBreadcrumbInternal => _beforeBreadcrumb;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ namespace Sentry
DisabledHub = 2,
EmptyMessage = 3,
DroppedByEventProcessor = 4,
DroppedByBeforeSendFeedback = 5,
}
public enum CheckInStatus
{
Expand Down Expand Up @@ -903,6 +904,8 @@ namespace Sentry
public void SetBeforeBreadcrumb(System.Func<Sentry.Breadcrumb, Sentry.SentryHint, Sentry.Breadcrumb?> beforeBreadcrumb) { }
public void SetBeforeSend(System.Func<Sentry.SentryEvent, Sentry.SentryEvent?> beforeSend) { }
public void SetBeforeSend(System.Func<Sentry.SentryEvent, Sentry.SentryHint, Sentry.SentryEvent?> beforeSend) { }
public void SetBeforeSendFeedback(System.Func<Sentry.SentryEvent, Sentry.SentryEvent?> beforeSendFeedback) { }
public void SetBeforeSendFeedback(System.Func<Sentry.SentryEvent, Sentry.SentryHint, Sentry.SentryEvent?> beforeSendFeedback) { }
public void SetBeforeSendLog(System.Func<Sentry.SentryLog, Sentry.SentryLog?> beforeSendLog) { }
public void SetBeforeSendMetric(System.Func<Sentry.SentryMetric, Sentry.SentryMetric?> beforeSendMetric) { }
public void SetBeforeSendTransaction(System.Func<Sentry.SentryTransaction, Sentry.SentryTransaction?> beforeSendTransaction) { }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ namespace Sentry
DisabledHub = 2,
EmptyMessage = 3,
DroppedByEventProcessor = 4,
DroppedByBeforeSendFeedback = 5,
}
public enum CheckInStatus
{
Expand Down Expand Up @@ -903,6 +904,8 @@ namespace Sentry
public void SetBeforeBreadcrumb(System.Func<Sentry.Breadcrumb, Sentry.SentryHint, Sentry.Breadcrumb?> beforeBreadcrumb) { }
public void SetBeforeSend(System.Func<Sentry.SentryEvent, Sentry.SentryEvent?> beforeSend) { }
public void SetBeforeSend(System.Func<Sentry.SentryEvent, Sentry.SentryHint, Sentry.SentryEvent?> beforeSend) { }
public void SetBeforeSendFeedback(System.Func<Sentry.SentryEvent, Sentry.SentryEvent?> beforeSendFeedback) { }
public void SetBeforeSendFeedback(System.Func<Sentry.SentryEvent, Sentry.SentryHint, Sentry.SentryEvent?> beforeSendFeedback) { }
public void SetBeforeSendLog(System.Func<Sentry.SentryLog, Sentry.SentryLog?> beforeSendLog) { }
public void SetBeforeSendMetric(System.Func<Sentry.SentryMetric, Sentry.SentryMetric?> beforeSendMetric) { }
public void SetBeforeSendTransaction(System.Func<Sentry.SentryTransaction, Sentry.SentryTransaction?> beforeSendTransaction) { }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ namespace Sentry
DisabledHub = 2,
EmptyMessage = 3,
DroppedByEventProcessor = 4,
DroppedByBeforeSendFeedback = 5,
}
public enum CheckInStatus
{
Expand Down Expand Up @@ -903,6 +904,8 @@ namespace Sentry
public void SetBeforeBreadcrumb(System.Func<Sentry.Breadcrumb, Sentry.SentryHint, Sentry.Breadcrumb?> beforeBreadcrumb) { }
public void SetBeforeSend(System.Func<Sentry.SentryEvent, Sentry.SentryEvent?> beforeSend) { }
public void SetBeforeSend(System.Func<Sentry.SentryEvent, Sentry.SentryHint, Sentry.SentryEvent?> beforeSend) { }
public void SetBeforeSendFeedback(System.Func<Sentry.SentryEvent, Sentry.SentryEvent?> beforeSendFeedback) { }
public void SetBeforeSendFeedback(System.Func<Sentry.SentryEvent, Sentry.SentryHint, Sentry.SentryEvent?> beforeSendFeedback) { }
public void SetBeforeSendLog(System.Func<Sentry.SentryLog, Sentry.SentryLog?> beforeSendLog) { }
public void SetBeforeSendMetric(System.Func<Sentry.SentryMetric, Sentry.SentryMetric?> beforeSendMetric) { }
public void SetBeforeSendTransaction(System.Func<Sentry.SentryTransaction, Sentry.SentryTransaction?> beforeSendTransaction) { }
Expand Down
3 changes: 3 additions & 0 deletions test/Sentry.Tests/ApiApprovalTests.Run.Net4_8.verified.txt
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ namespace Sentry
DisabledHub = 2,
EmptyMessage = 3,
DroppedByEventProcessor = 4,
DroppedByBeforeSendFeedback = 5,
}
public enum CheckInStatus
{
Expand Down Expand Up @@ -884,6 +885,8 @@ namespace Sentry
public void SetBeforeBreadcrumb(System.Func<Sentry.Breadcrumb, Sentry.SentryHint, Sentry.Breadcrumb?> beforeBreadcrumb) { }
public void SetBeforeSend(System.Func<Sentry.SentryEvent, Sentry.SentryEvent?> beforeSend) { }
public void SetBeforeSend(System.Func<Sentry.SentryEvent, Sentry.SentryHint, Sentry.SentryEvent?> beforeSend) { }
public void SetBeforeSendFeedback(System.Func<Sentry.SentryEvent, Sentry.SentryEvent?> beforeSendFeedback) { }
public void SetBeforeSendFeedback(System.Func<Sentry.SentryEvent, Sentry.SentryHint, Sentry.SentryEvent?> beforeSendFeedback) { }
public void SetBeforeSendLog(System.Func<Sentry.SentryLog, Sentry.SentryLog?> beforeSendLog) { }
public void SetBeforeSendMetric(System.Func<Sentry.SentryMetric, Sentry.SentryMetric?> beforeSendMetric) { }
public void SetBeforeSendTransaction(System.Func<Sentry.SentryTransaction, Sentry.SentryTransaction?> beforeSendTransaction) { }
Expand Down
110 changes: 110 additions & 0 deletions test/Sentry.Tests/SentryClientTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1035,6 +1035,116 @@ public void CaptureFeedback_EventDropped_SendsClientReport()
_fixture.ClientReportRecorder.Received(1).RecordDiscardedEvent(expectedReason, DataCategory.Feedback);
}

[Fact]
public void CaptureFeedback_BeforeSendFeedbackSet_CallbackInvokedAndMutationApplied()
{
//Arrange
var feedback = new SentryFeedback("Everything is great!");
SentryEvent received = null;
_fixture.SentryOptions.SetBeforeSendFeedback((@event, _) =>
{
received = @event;
@event.SetTag("scrubbed", "true");
return @event;
});
var sut = _fixture.GetSut();

Envelope envelope = null;
sut.Worker.When(w => w.EnqueueEnvelope(Arg.Any<Envelope>()))
.Do(callback => envelope = callback.Arg<Envelope>());

//Act
var id = sut.CaptureFeedback(feedback, out var result);

//Assert
result.Should().Be(CaptureFeedbackResult.Success);
id.Should().NotBe(SentryId.Empty);
received.Should().NotBeNull();
received.Contexts.Feedback.Should().NotBeNull();
received.Contexts.Feedback!.Message.Should().Be("Everything is great!");
var item = envelope.Items.First(x => x.TryGetType() == EnvelopeItem.TypeValueFeedback);
var @event = (SentryEvent)((JsonSerializable)item.Payload).Source;
@event.Tags.Should().Contain(new KeyValuePair<string, string>("scrubbed", "true"));
}

[Fact]
public void CaptureFeedback_BeforeSendFeedbackSet_ReceivesHintFromCaller()
{
//Arrange
var feedback = new SentryFeedback("Everything is great!");
var hint = new SentryHint();
var attachment = AttachmentHelper.FakeAttachment("scrub-me.txt");
hint.Attachments.Add(attachment);
SentryHint receivedHint = null;
_fixture.SentryOptions.SetBeforeSendFeedback((@event, h) =>
{
receivedHint = h;
return @event;
});
var sut = _fixture.GetSut();

//Act
var id = sut.CaptureFeedback(feedback, out var result, null, hint);

//Assert
result.Should().Be(CaptureFeedbackResult.Success);
id.Should().NotBe(SentryId.Empty);
receivedHint.Should().BeSameAs(hint);
receivedHint.Attachments.Should().Contain(attachment);
}

[Fact]
public void CaptureFeedback_BeforeSendFeedbackReturnsNull_FeedbackDropped()
{
//Arrange
var feedback = new SentryFeedback("Everything is great!");
_fixture.SentryOptions.SetBeforeSendFeedback((SentryEvent _) => null);
var sut = _fixture.GetSut();

//Act
var id = sut.CaptureFeedback(feedback, out var result);

//Assert
result.Should().Be(CaptureFeedbackResult.DroppedByBeforeSendFeedback);
id.Should().Be(SentryId.Empty);
_ = sut.Worker.DidNotReceive().EnqueueEnvelope(Arg.Any<Envelope>());
_fixture.ClientReportRecorder.Received(1).RecordDiscardedEvent(DiscardReason.BeforeSend, DataCategory.Feedback);
}

[Fact]
public void CaptureFeedback_BeforeSendFeedbackThrows_FeedbackDropped()
{
//Arrange
var feedback = new SentryFeedback("Everything is great!");
_fixture.SentryOptions.SetBeforeSendFeedback((SentryEvent _) => throw new InvalidOperationException("boom"));
var sut = _fixture.GetSut();

//Act
var id = sut.CaptureFeedback(feedback, out var result);

//Assert
result.Should().Be(CaptureFeedbackResult.DroppedByBeforeSendFeedback);
id.Should().Be(SentryId.Empty);
_ = sut.Worker.DidNotReceive().EnqueueEnvelope(Arg.Any<Envelope>());
_fixture.ClientReportRecorder.Received(1).RecordDiscardedEvent(DiscardReason.BeforeSend, DataCategory.Feedback);
}

[Fact]
public void CaptureFeedback_EnqueueFails_ReturnsUnknownError()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Seems unrelated to this PR but harmless...

{
//Arrange
var feedback = new SentryFeedback("Everything is great!");
_fixture.BackgroundWorker.EnqueueEnvelope(Arg.Any<Envelope>()).Returns(false);
var sut = _fixture.GetSut();

//Act
var id = sut.CaptureFeedback(feedback, out var result);

//Assert
result.Should().Be(CaptureFeedbackResult.UnknownError);
id.Should().Be(SentryId.Empty);
}

[Fact]
public void CaptureFeedback_WithHint_HasHintAttachment()
{
Expand Down
Loading