Skip to content

Commit dea8dbc

Browse files
adinaueradinauer
authored
Merge pull request #6142 from getsentry/fix/callback-error-handling-event-processors
fix(core): [Callback Errors 3] Drop failed processor data
2 parents 0b922a6 + 38bf38c commit dea8dbc

4 files changed

Lines changed: 516 additions & 12 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@
6262

6363
- Fix SDK callback error handling ([#6140](https://github.com/getsentry/sentry-java/pull/6140))
6464
- Add `DiscardReason.CALLBACK_ERROR` and use it for telemetry dropped when a `beforeSend*` callback throws. `OnDiscardCallback` can now receive this value.
65+
- Drop telemetry and record `callback_error` when a customer event processor throws instead of continuing with a potentially partially processed item. SDK-owned processor failures are logged and processing continues without a `callback_error` client report.
6566
- Disable URL caching when reading `META-INF/MANIFEST.MF` files during version detection so that the SDK no longer keeps jar file handles open for the life of the process ([#6124](https://github.com/getsentry/sentry-java/pull/6124)
6667
- Keep the `EventListener` wrapped by `SentryOkHttpEventListener` per `Call` ([#6003](https://github.com/getsentry/sentry-java/pull/6003))
6768

‎sentry/src/main/java/io/sentry/SentryClient.java‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import io.sentry.hints.Cached;
99
import io.sentry.hints.DiskFlushNotification;
1010
import io.sentry.hints.TransactionEnd;
11+
import io.sentry.internal.eventprocessor.SentryEventProcessor;
1112
import io.sentry.logger.ILoggerBatchProcessor;
1213
import io.sentry.metrics.IMetricsBatchProcessor;
1314
import io.sentry.protocol.Contexts;
@@ -495,6 +496,12 @@ private SentryEvent processEvent(
495496
e,
496497
"An exception occurred while processing event by processor: %s",
497498
processor.getClass().getName());
499+
if (!(processor instanceof SentryEventProcessor)) {
500+
options
501+
.getClientReportRecorder()
502+
.recordLostEvent(DiscardReason.CALLBACK_ERROR, DataCategory.Error);
503+
return null;
504+
}
498505
}
499506

500507
if (event == null) {
@@ -546,6 +553,10 @@ private SentryLogEvent processLogEvent(
546553
e,
547554
"An exception occurred while processing log event by processor: %s",
548555
processor.getClass().getName());
556+
if (!(processor instanceof SentryEventProcessor)) {
557+
recordLostLogEvent(DiscardReason.CALLBACK_ERROR, eventBeforeProcessor);
558+
return null;
559+
}
549560
}
550561

551562
if (event == null) {
@@ -579,6 +590,10 @@ private SentryMetricsEvent processMetricsEvent(
579590
e,
580591
"An exception occurred while processing metrics event by processor: %s",
581592
processor.getClass().getName());
593+
if (!(processor instanceof SentryEventProcessor)) {
594+
recordLostMetricsEvent(DiscardReason.CALLBACK_ERROR, eventBeforeProcessor);
595+
return null;
596+
}
582597
}
583598

584599
if (event == null) {
@@ -611,6 +626,16 @@ private SentryMetricsEvent processMetricsEvent(
611626
e,
612627
"An exception occurred while processing transaction by processor: %s",
613628
processor.getClass().getName());
629+
if (!(processor instanceof SentryEventProcessor)) {
630+
options
631+
.getClientReportRecorder()
632+
.recordLostEvent(DiscardReason.CALLBACK_ERROR, DataCategory.Transaction);
633+
options
634+
.getClientReportRecorder()
635+
.recordLostEvent(
636+
DiscardReason.CALLBACK_ERROR, DataCategory.Span, spanCountBeforeProcessor + 1);
637+
return null;
638+
}
614639
}
615640
final int spanCountAfterProcessor = transaction == null ? 0 : transaction.getSpans().size();
616641

@@ -664,6 +689,12 @@ private SentryReplayEvent processReplayEvent(
664689
e,
665690
"An exception occurred while processing replay event by processor: %s",
666691
processor.getClass().getName());
692+
if (!(processor instanceof SentryEventProcessor)) {
693+
options
694+
.getClientReportRecorder()
695+
.recordLostEvent(DiscardReason.CALLBACK_ERROR, DataCategory.Replay);
696+
return null;
697+
}
667698
}
668699

669700
if (replayEvent == null) {
@@ -698,6 +729,12 @@ private SentryEvent processFeedbackEvent(
698729
e,
699730
"An exception occurred while processing feedback event by processor: %s",
700731
processor.getClass().getName());
732+
if (!(processor instanceof SentryEventProcessor)) {
733+
options
734+
.getClientReportRecorder()
735+
.recordLostEvent(DiscardReason.CALLBACK_ERROR, DataCategory.Feedback);
736+
return null;
737+
}
701738
}
702739

703740
if (feedbackEvent == null) {

‎sentry/src/test/java/io/sentry/SentryClientInternalEventProcessorTest.kt‎

Lines changed: 229 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,229 @@
1+
package io.sentry
2+
3+
import com.google.common.truth.Truth.assertThat
4+
import io.sentry.clientreport.ClientReportTestHelper.Companion.assertClientReport
5+
import io.sentry.clientreport.DiscardReason
6+
import io.sentry.clientreport.DiscardedEvent
7+
import io.sentry.internal.eventprocessor.SentryEventProcessor
8+
import io.sentry.protocol.Feedback
9+
import io.sentry.protocol.SentryId
10+
import io.sentry.protocol.SentryTransaction
11+
import kotlin.test.Test
12+
import org.junit.runner.RunWith
13+
import org.junit.runners.Parameterized
14+
import org.mockito.kotlin.any
15+
import org.mockito.kotlin.anyOrNull
16+
import org.mockito.kotlin.check
17+
import org.mockito.kotlin.doAnswer
18+
import org.mockito.kotlin.eq
19+
import org.mockito.kotlin.mock
20+
import org.mockito.kotlin.never
21+
import org.mockito.kotlin.same
22+
import org.mockito.kotlin.verify
23+
import org.mockito.kotlin.verifyNoInteractions
24+
import org.mockito.kotlin.whenever
25+
26+
@RunWith(Parameterized::class)
27+
class SentryClientInternalEventProcessorTest(private val onScope: Boolean) {
28+
companion object {
29+
@JvmStatic
30+
@Parameterized.Parameters(name = "onScope={0}")
31+
fun data(): List<Array<Boolean>> = listOf(arrayOf(false), arrayOf(true))
32+
}
33+
34+
private val fixture = SentryClientTest.Fixture()
35+
private val options = fixture.sentryOptions
36+
private val scope = Scope(options)
37+
private val processor = mock<SentryEventProcessor>()
38+
private val nextProcessor = mock<EventProcessor>()
39+
private val onDiscard = mock<SentryOptions.OnDiscardCallback>()
40+
private val logger = mock<ILogger>()
41+
private val failure = IllegalStateException("SDK processor failed")
42+
43+
init {
44+
options.eventProcessors.clear()
45+
options.onDiscard = onDiscard
46+
options.setLogger(logger)
47+
options.logs.isEnabled = true
48+
options.metrics.isEnabled = true
49+
if (onScope) {
50+
scope.addEventProcessor(processor)
51+
scope.addEventProcessor(nextProcessor)
52+
} else {
53+
options.addEventProcessor(processor)
54+
options.addEventProcessor(nextProcessor)
55+
}
56+
}
57+
58+
@Test
59+
fun `SDK event processor failure keeps event and runs remaining callbacks`() {
60+
val event = SentryEvent()
61+
val beforeSend = mock<SentryOptions.BeforeSendCallback>()
62+
whenever(processor.process(any<SentryEvent>(), any())).thenThrow(failure)
63+
whenever(nextProcessor.process(any<SentryEvent>(), any())).thenAnswer { it.arguments[0] }
64+
whenever(beforeSend.execute(any(), any())).thenAnswer { it.arguments[0] }
65+
options.beforeSend = beforeSend
66+
67+
val id = fixture.getSut().captureEvent(event, scope)
68+
69+
assertThat(id).isEqualTo(event.eventId)
70+
verify(nextProcessor).process(same(event), any())
71+
verify(beforeSend).execute(same(event), any())
72+
verify(fixture.transport)
73+
.send(check { assertThat(it.header.eventId).isEqualTo(id) }, anyOrNull())
74+
assertFailureLoggedWithoutLoss("event")
75+
}
76+
77+
@Test
78+
fun `SDK transaction processor failure keeps transaction and spans`() {
79+
val transaction = SentryTransaction(fixture.sentryTracer)
80+
val beforeSend = mock<SentryOptions.BeforeSendTransactionCallback>()
81+
whenever(processor.process(any<SentryTransaction>(), any())).thenThrow(failure)
82+
whenever(nextProcessor.process(any<SentryTransaction>(), any())).thenAnswer { it.arguments[0] }
83+
whenever(beforeSend.execute(any(), any())).thenAnswer { it.arguments[0] }
84+
options.beforeSendTransaction = beforeSend
85+
86+
val id = fixture.getSut().captureTransaction(transaction, scope, null)
87+
88+
assertThat(id).isEqualTo(transaction.eventId)
89+
verify(nextProcessor).process(same(transaction), any())
90+
verify(beforeSend).execute(same(transaction), any())
91+
verify(fixture.transport)
92+
.send(
93+
check {
94+
val sent = it.items.first().getTransaction(options.serializer)!!
95+
assertThat(sent.eventId).isEqualTo(id)
96+
assertThat(sent.spans).hasSize(1)
97+
},
98+
anyOrNull(),
99+
)
100+
assertFailureLoggedWithoutLoss("transaction")
101+
}
102+
103+
@Test
104+
fun `SDK feedback processor failure keeps feedback and runs remaining callbacks`() {
105+
val feedback = Feedback("message")
106+
val beforeSend = mock<SentryOptions.BeforeSendCallback>()
107+
whenever(processor.process(any<SentryEvent>(), any())).thenThrow(failure)
108+
whenever(nextProcessor.process(any<SentryEvent>(), any())).thenAnswer { it.arguments[0] }
109+
whenever(beforeSend.execute(any(), any())).thenAnswer { it.arguments[0] }
110+
options.beforeSendFeedback = beforeSend
111+
112+
val id = fixture.getSut().captureFeedback(feedback, null, scope)
113+
114+
assertThat(id).isNotEqualTo(SentryId.EMPTY_ID)
115+
verify(nextProcessor)
116+
.process(
117+
check<SentryEvent> {
118+
assertThat(it.contexts.feedback).isSameInstanceAs(feedback)
119+
},
120+
any(),
121+
)
122+
verify(beforeSend).execute(check { assertThat(it.eventId).isEqualTo(id) }, any())
123+
verify(fixture.transport)
124+
.send(check { assertThat(it.header.eventId).isEqualTo(id) }, anyOrNull())
125+
assertFailureLoggedWithoutLoss("feedback event")
126+
}
127+
128+
@Test
129+
fun `SDK log processor failure keeps log and runs remaining callbacks`() {
130+
val event = SentryLogEvent(SentryId(), SentryNanotimeDate(), "message", SentryLogLevel.WARN)
131+
val beforeSend = mock<SentryOptions.Logs.BeforeSendLogCallback>()
132+
whenever(processor.process(any<SentryLogEvent>())).thenThrow(failure)
133+
whenever(nextProcessor.process(any<SentryLogEvent>())).thenAnswer { it.arguments[0] }
134+
whenever(beforeSend.execute(any())).thenAnswer { it.arguments[0] }
135+
options.logs.beforeSend = beforeSend
136+
137+
fixture.getSut().captureLog(event, scope)
138+
139+
verify(nextProcessor).process(same(event))
140+
verify(beforeSend).execute(same(event))
141+
verify(fixture.loggerBatchProcessor).add(same(event))
142+
assertFailureLoggedWithoutLoss("log event")
143+
}
144+
145+
@Test
146+
fun `SDK metric processor failure keeps metric and runs remaining callbacks`() {
147+
val event = SentryMetricsEvent(SentryId(), SentryNanotimeDate(), "name", "gauge", 123.0)
148+
val beforeSend = mock<SentryOptions.Metrics.BeforeSendMetricCallback>()
149+
whenever(processor.process(any<SentryMetricsEvent>(), any())).thenThrow(failure)
150+
whenever(nextProcessor.process(any<SentryMetricsEvent>(), any())).thenAnswer { it.arguments[0] }
151+
whenever(beforeSend.execute(any(), any())).thenAnswer { it.arguments[0] }
152+
options.metrics.beforeSend = beforeSend
153+
154+
fixture.getSut().captureMetric(event, scope, null)
155+
156+
verify(nextProcessor).process(same(event), any())
157+
verify(beforeSend).execute(same(event), any())
158+
verify(fixture.metricsBatchProcessor).add(same(event))
159+
assertFailureLoggedWithoutLoss("metrics event")
160+
}
161+
162+
@Test
163+
fun `SDK processor returning null still drops event as event_processor`() {
164+
whenever(processor.process(any<SentryEvent>(), any())).thenReturn(null)
165+
166+
val id = fixture.getSut().captureEvent(SentryEvent(), scope)
167+
168+
assertThat(id).isEqualTo(SentryId.EMPTY_ID)
169+
verify(nextProcessor, never()).process(any<SentryEvent>(), any())
170+
verify(fixture.transport, never()).send(any(), anyOrNull())
171+
assertClientReport(
172+
options.clientReportRecorder,
173+
listOf(DiscardedEvent(DiscardReason.EVENT_PROCESSOR.reason, DataCategory.Error.category, 1)),
174+
)
175+
}
176+
177+
@Test
178+
fun `customer processor failure after SDK processor failure still drops event`() {
179+
whenever(processor.process(any<SentryEvent>(), any())).thenThrow(failure)
180+
whenever(nextProcessor.process(any<SentryEvent>(), any()))
181+
.thenThrow(IllegalArgumentException("customer"))
182+
val beforeSend = mock<SentryOptions.BeforeSendCallback>()
183+
options.beforeSend = beforeSend
184+
185+
val id = fixture.getSut().captureEvent(SentryEvent(), scope)
186+
187+
assertThat(id).isEqualTo(SentryId.EMPTY_ID)
188+
verify(nextProcessor).process(any<SentryEvent>(), any())
189+
verifyNoInteractions(beforeSend)
190+
verify(fixture.transport, never()).send(any(), anyOrNull())
191+
assertClientReport(
192+
options.clientReportRecorder,
193+
listOf(DiscardedEvent(DiscardReason.CALLBACK_ERROR.reason, DataCategory.Error.category, 1)),
194+
)
195+
verify(onDiscard).execute(DiscardReason.CALLBACK_ERROR, DataCategory.Error, 1)
196+
}
197+
198+
@Test
199+
fun `spans removed before SDK processor failure retain event_processor accounting`() {
200+
val transaction = SentryTransaction(fixture.sentryTracer)
201+
whenever(processor.process(any<SentryTransaction>(), any())).doAnswer {
202+
transaction.spans.clear()
203+
throw failure
204+
}
205+
whenever(nextProcessor.process(any<SentryTransaction>(), any())).thenAnswer { it.arguments[0] }
206+
207+
val id = fixture.getSut().captureTransaction(transaction, scope, null)
208+
209+
assertThat(id).isEqualTo(transaction.eventId)
210+
verify(nextProcessor).process(same(transaction), any())
211+
verify(fixture.transport).send(any(), anyOrNull())
212+
assertClientReport(
213+
options.clientReportRecorder,
214+
listOf(DiscardedEvent(DiscardReason.EVENT_PROCESSOR.reason, DataCategory.Span.category, 1)),
215+
)
216+
}
217+
218+
private fun assertFailureLoggedWithoutLoss(item: String) {
219+
verify(logger)
220+
.log(
221+
eq(SentryLevel.ERROR),
222+
same(failure),
223+
eq("An exception occurred while processing $item by processor: %s"),
224+
eq(processor.javaClass.name),
225+
)
226+
assertClientReport(options.clientReportRecorder, emptyList())
227+
verifyNoInteractions(onDiscard)
228+
}
229+
}

0 commit comments

Comments
 (0)