Add OkHttp span auto-close when response body is not read

[stefanosiano]

📜 Description

added 500 milliseconds timeout after responseHeaders to finish the call root span if the response body is not read
added scheduleFinish to SentryOkHttpEvent

💡 Motivation and Context

The OkHttp span could be running forever in case a request is made and the response is never read, as the listener doesn't receive the corresponding calls.
Fixes #2908

💚 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	358.45 ms	446.98 ms	88.53 ms
Size	1.72 MiB	2.29 MiB	575.97 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
9e60fc1	283.04 ms	324.09 ms	41.04 ms
496bdfd	272.86 ms	407.33 ms	134.48 ms
9e60fc1	310.37 ms	359.48 ms	49.11 ms
496bdfd	301.22 ms	343.96 ms	42.73 ms
87b3774	310.48 ms	362.04 ms	51.56 ms
9e60fc1	334.08 ms	380.00 ms	45.92 ms
4bf202b	331.20 ms	345.24 ms	14.04 ms
9e60fc1	337.84 ms	385.24 ms	47.40 ms
adf8fe3	300.49 ms	357.36 ms	56.87 ms
9e60fc1	308.22 ms	361.72 ms	53.50 ms

App size

Revision	Plain	With Sentry	Diff
9e60fc1	1.72 MiB	2.29 MiB	575.91 KiB
496bdfd	1.72 MiB	2.28 MiB	571.82 KiB
9e60fc1	1.72 MiB	2.29 MiB	575.91 KiB
496bdfd	1.72 MiB	2.28 MiB	571.82 KiB
87b3774	1.72 MiB	2.29 MiB	575.54 KiB
9e60fc1	1.72 MiB	2.29 MiB	575.91 KiB
4bf202b	1.72 MiB	2.29 MiB	575.54 KiB
9e60fc1	1.72 MiB	2.29 MiB	575.91 KiB
adf8fe3	1.72 MiB	2.29 MiB	575.24 KiB
9e60fc1	1.72 MiB	2.29 MiB	575.91 KiB

Previous results on branch: fix/okhttp_autofinish_responsebody

Startup times

Revision	Plain	With Sentry	Diff
7c336e8	306.74 ms	363.88 ms	57.14 ms
25c4425	306.14 ms	348.22 ms	42.08 ms
9e90d5e	381.62 ms	458.86 ms	77.24 ms

App size

Revision	Plain	With Sentry	Diff
7c336e8	1.72 MiB	2.29 MiB	575.90 KiB
25c4425	1.72 MiB	2.29 MiB	575.90 KiB
9e90d5e	1.72 MiB	2.29 MiB	575.93 KiB

[codecov]

Codecov Report

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

Files	Coverage Δ
sentry/src/main/java/io/sentry/CheckInStatus.java	100.00% <100.00%> (ø)
sentry/src/main/java/io/sentry/Hub.java	75.84% <100.00%> (+0.67%)	⬆️
sentry/src/main/java/io/sentry/HubAdapter.java	95.65% <100.00%> (+0.06%)	⬆️
sentry/src/main/java/io/sentry/IHub.java	92.59% <ø> (ø)
sentry/src/main/java/io/sentry/ISentryClient.java	95.45% <ø> (ø)
...y/src/main/java/io/sentry/MonitorScheduleType.java	100.00% <100.00%> (ø)
...y/src/main/java/io/sentry/MonitorScheduleUnit.java	100.00% <100.00%> (ø)
sentry/src/main/java/io/sentry/NoOpHub.java	47.91% <ø> (-1.02%)	⬇️
...ntry/src/main/java/io/sentry/NoOpSentryClient.java	63.63% <ø> (-6.37%)	⬇️
sentry/src/main/java/io/sentry/Sentry.java	85.95% <100.00%> (+0.06%)	⬆️
... and 9 more

... and 17 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

[markushi]

Looks good to me, please have a look at my comment!

I played around with the Response class myself yesterday, and it seems like there's no better option for us - apart from removing the feature altogether. From the docs:

Response bodies must be closed and may be consumed only once.

So the issue we're seeing here is a misuse of OkHttp itself, which in turn breaks our instrumentation 😅

In the long term I think we should think about having multiple ExecutorServices, as it's handling a lot of async timeout related tasks by now, and features will start to delay each other, ultimately making it harder to have a predictable SDK. cc @romtsn

[romtsn]

So the issue we're seeing here is a misuse of OkHttp itself, which in turn breaks our instrumentation 😅

My opinion - we should not handle this case at all, as it will be anyway a problem on the consumer's side if they are not closing response bodies (regardless if sentry is in the app or not). I'd rather us create an event in this case and warn the users they have leaked resources (but this is probably a bigger initiative to detect or Closeables that are not closed)

[stefanosiano]

So the issue we're seeing here is a misuse of OkHttp itself, which in turn breaks our instrumentation 😅

My opinion - we should not handle this case at all, as it will be anyway a problem on the consumer's side if they are not closing response bodies (regardless if sentry is in the app or not). I'd rather us create an event in this case and warn the users they have leaked resources (but this is probably a bigger initiative to detect or Closeables that are not closed)

i agree to some extent. It's true that is user's fault, but not all calls require reading the response body.
Think about a background task that sends some unimportant data, and you don't care if the server received it or not. You simply don't read the response. This is just an example coming from personal experience.
In that case we could have a transaction running forever, and that would break other features of the SDK. The philosophy should be "Even if the user fails, we shouldn't fail with him"