fix: Don't increase session's error count for dropped events

[kevinrenskers]

📜 Description

Don't immediately update the session's error count. Only do this after running the beforeSend block.

💡 Motivation and Context

Closes #1777

💚 How did you test it?

Unit tests

📝 Checklist

• I reviewed the submitted code
• I added tests to verify the changes
• I updated the docs if needed
• Review from the native team if needed
• No breaking changes

🔮 Next steps

[kevinrenskers]

With this small change we first check if we still have an event after running the beforeSend block, and only then do we update the session's error count. But - [SentryClient captureSession:] is still executed, which still calls captureEnvelope which then sends the envelope. I guess we don't want to do that anymore if I read this correctly?

Second question: is this only for errors or also for exceptions? I.e. the whole captureException flow.

[github-actions]

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1244.64 ms	1272.50 ms	27.86 ms
Size	20.75 KiB	374.53 KiB	353.78 KiB

Baseline results on branch: master

Startup times

Revision	Plain	With Sentry	Diff
f444dc4	1242.55 ms	1248.82 ms	6.27 ms
0fdf0b2	1243.92 ms	1250.86 ms	6.94 ms
59afa00	1209.29 ms	1237.88 ms	28.59 ms
411a940	1232.33 ms	1259.33 ms	27.01 ms
6dc0bd1	1220.49 ms	1237.44 ms	16.95 ms
9754750	1234.71 ms	1265.21 ms	30.49 ms
c2a9b60	1222.10 ms	1240.62 ms	18.52 ms
791123d	1217.52 ms	1253.08 ms	35.56 ms
4e037c4	1205.00 ms	1227.58 ms	22.58 ms
b15627c	1228.88 ms	1269.70 ms	40.82 ms

App size

Revision	Plain	With Sentry	Diff
f444dc4	20.75 KiB	367.00 KiB	346.25 KiB
0fdf0b2	20.51 KiB	332.90 KiB	312.39 KiB
59afa00	20.50 KiB	365.16 KiB	344.65 KiB
411a940	20.50 KiB	361.79 KiB	341.29 KiB
6dc0bd1	20.51 KiB	333.58 KiB	313.07 KiB
9754750	20.75 KiB	374.16 KiB	353.41 KiB
c2a9b60	20.50 KiB	333.54 KiB	313.04 KiB
791123d	20.51 KiB	331.81 KiB	311.30 KiB
4e037c4	20.50 KiB	361.80 KiB	341.29 KiB
b15627c	20.50 KiB	337.76 KiB	317.25 KiB

Previous results on branch: feat/1777-no-session-update-for-dropped-events

Startup times

Revision	Plain	With Sentry	Diff
0d5ca6b	1249.73 ms	1270.78 ms	21.05 ms
041dc77	1254.86 ms	1274.57 ms	19.71 ms
3ca16ab	1251.62 ms	1279.98 ms	28.36 ms
50e4603	1226.04 ms	1251.92 ms	25.88 ms
db3535b	1190.22 ms	1221.26 ms	31.04 ms
50e4603	1196.42 ms	1227.18 ms	30.76 ms
9f1fe4c	1263.76 ms	1269.14 ms	5.38 ms
5000a1e	1251.86 ms	1270.22 ms	18.36 ms
db3535b	1227.28 ms	1253.43 ms	26.15 ms
457f28e	1239.00 ms	1250.72 ms	11.72 ms

App size

Revision	Plain	With Sentry	Diff
0d5ca6b	20.75 KiB	374.53 KiB	353.78 KiB
041dc77	20.75 KiB	374.19 KiB	353.44 KiB
3ca16ab	20.75 KiB	374.61 KiB	353.86 KiB
50e4603	20.75 KiB	374.61 KiB	353.86 KiB
db3535b	20.75 KiB	374.12 KiB	353.37 KiB
50e4603	20.75 KiB	374.61 KiB	353.86 KiB
9f1fe4c	20.75 KiB	374.24 KiB	353.48 KiB
5000a1e	20.75 KiB	367.28 KiB	346.52 KiB
db3535b	20.75 KiB	374.13 KiB	353.37 KiB
457f28e	20.75 KiB	374.50 KiB	353.74 KiB

[philipphofmann]

Don't the docs state that clearly?

Events may be dropped by our filtering mechanisms (sample rate, beforeSend, event processors or ignored exception types). Only events dropped due to sampling should update the session despite being dropped as we assume the event was dropped to save quota but would have been something the developer cares about. Events dropped due to other reasons should not update the session as we assume they are more likely to be dropped because the developer chooses to ignore them.

These docs don't mention rate limiting, though. I'm going to clarify this. So just ignore it for now.

Basically, we need to change when we increment the session error count in the hub. I think the only change for our SDK now is to check if the event gets dropped in beforeSend or in an event processor. So we need to refactor the code that only if the event doesn't get dropped there, we update the session. That applies to anything updating the session, meaning exceptions, errors, events, crashes, etc. . I'm unsure what the best way is to solve this. Can you suggest a concept in this draft PR?

[kevinrenskers]

Can you suggest a concept in this draft PR?

I do have a concept for error catching, and I guess this can pretty easily be extended to other events and exceptions and stuff.

[philipphofmann]

@kevinrenskers, rate limited events should update the session, see getsentry/develop#756.

[kevinrenskers]

Yes, rate limiting happens after this code runs so is unaffected by this change.

[kevinrenskers]

I don't think you've answered the most important question:

But - [SentryClient captureSession:] is still executed, which still calls captureEnvelope which then sends the envelope. I guess we don't want to do that anymore if I read this correctly?

Can I please get some input on the code change I have made, if this is on the correct path, and if my assumption is correct?

[philipphofmann]

One important issue to address. Apart from that already looks great 😀

[philipphofmann]

h: Sorry, I think I forgot to mention that yesterday on our call, or maybe we didn't touch that topic. If we don't increment the error count, there is not need to send a session update, as the session stays the same. In that case, we should only send the event without the session.

[kevinrenskers]

I've made the change as we discussed at 14:30 but actually it doesn't change anything - we already didn't send the session update if the event was nil. That's because even though yes we were calling [self sendEvent:event withSession:session withScope:scope], that doesn't do anything at all if there is no event.

So in the end the most recent commit is just cleaning up, but doesn't change any behavior as nothing needed fixing.

[philipphofmann]

Thank you @kevinrenskers 👏

[philipphofmann]

m: That needs to move up a bit.

[kevinrenskers]

It would be really nice to automate the changelogs :(
Like https://github.com/loopwerk/tag-changelog, or the many many other solutions out there.