SentryOkHttpEvent report exceptions only on the call root span

[stefanosiano]

📜 Description

SentryOkHttpEvent now shows exceptions only on the call root span

💡 Motivation and Context

Having the expection shown in the inner spans of the http call could be confusing, as in sentry-demos/android#60

💚 How did you test it?

Unit

📝 Checklist

• I reviewed the submitted code.
• I added tests to verify the changes.
• No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
• I updated the docs if needed.
• Review from the native team if needed.
• No breaking change or entry added to the changelog.
• No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

[github-actions]

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	395.62 ms	468.12 ms	72.50 ms
Size	1.72 MiB	2.29 MiB	576.55 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
99a51e2	405.11 ms	479.65 ms	74.54 ms

App size

Revision	Plain	With Sentry	Diff
99a51e2	1.72 MiB	2.29 MiB	576.34 KiB

Previous results on branch: fix/okhttp_exception_root_only

Startup times

Revision	Plain	With Sentry	Diff
bc5005b	396.14 ms	473.92 ms	77.78 ms
ed472a4	403.96 ms	471.70 ms	67.74 ms

App size

Revision	Plain	With Sentry	Diff
bc5005b	1.72 MiB	2.29 MiB	576.34 KiB
ed472a4	1.72 MiB	2.29 MiB	576.34 KiB

[stefanosiano]

This is not going to work with errors captured by the interceptor, correct @romtsn @markushi ?
From my understanding we are not setting the throwable to the span explicitly 😕

[codecov]

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
...try/src/main/java/io/sentry/util/CheckInUtils.java	92.59% <100.00%> (+9.25%)	⬆️
...java/io/sentry/android/okhttp/SentryOkHttpUtils.kt	97.77% <97.77%> (ø)
...o/sentry/android/okhttp/SentryOkHttpInterceptor.kt	90.72% <81.81%> (-1.07%)	⬇️
...java/io/sentry/android/okhttp/SentryOkHttpEvent.kt	88.18% <81.25%> (-1.30%)	⬇️

... and 22 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

[romtsn]

@stefanosiano have you got a link to a sample event after this change?

[stefanosiano]

@stefanosiano have you got a link to a sample event after this change?

yep. This is before (the version in master)
This is after.
Sadly i found an edge case: manually starting a span while reading the response (which the demo app does) leads to this
I have to think about fixing that edge case, but it could be done on a separate PR

[stefanosiano]

Actually, the edge case happens also when using the Interceptor only, even if the EventListener collects more spans for a few more milliseconds than the Interceptor, so it's a little "worse"

[markushi]

LGTM!

[markushi]

It's actually HTTP errors we're catching here, how about naming it this way?

Suggested change

	- SentryOkHttpEvent report exceptions only on the call root span ([#2961](https://github.com/getsentry/sentry-java/pull/2961))
	- Always attach `SentryOkHttpEvent` errors to the call root span ([#2961](https://github.com/getsentry/sentry-java/pull/2961))

[stefanosiano]

Actually it's both
any exception caught by the listener is now associated to the call root span only, and the SentryHttpClientException is also associated to the call root span only.
What about "Always attach OkHttp errors and Http Client Errors only to call root span"?