Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RUM-6235 Handle sse request #2270

Merged
merged 3 commits into from
Sep 19, 2024
Merged

RUM-6235 Handle sse request #2270

merged 3 commits into from
Sep 19, 2024

Conversation

xgouchet
Copy link
Member

What does this PR do?

This PR Handles properly SSE (Server Sent Event) network request instrumented with our OkHttp interceptor.
In order to report the size of a downloaded resource, we use the peekBody() method which tries to read the whole content of the network request. In case of a SSE request, it would wait until the end of the stream to eventually complete the interception and forward the content to the receiver, blocking the server messages.

ServerSent events should have the text/server-event Mime Type, which we can use to detect such use case.

Motivation

Fixes #2266

@xgouchet xgouchet requested review from a team as code owners September 19, 2024 09:06
@xgouchet xgouchet requested a review from 0xnm September 19, 2024 10:49
Copy link
Member

@0xnm 0xnm left a comment

Choose a reason for hiding this comment

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

there is one comment about arrayOf -> setOf (performance-wise on such small collection it won't matter though), otherwise lgtm

@@ -89,7 +89,8 @@ timber = "5.0.1"
coroutines = "1.4.2"

# Local Server
ktor = "1.6.0"
ktor = "1.6.8"
ktorServer = "3.0.0-rc-1"
Copy link
Member

Choose a reason for hiding this comment

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

is there a concern if we are using a rc here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This lib is only used for the sample app, so it's ok

@xgouchet xgouchet force-pushed the xgouchet/RUM-6235/sse_request branch from b3aafcb to 307372d Compare September 19, 2024 13:41
@xgouchet xgouchet requested a review from mariusc83 September 19, 2024 13:41
@xgouchet xgouchet force-pushed the xgouchet/RUM-6235/sse_request branch from 307372d to 4fc8a54 Compare September 19, 2024 13:51
@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.90%. Comparing base (949a967) to head (4fc8a54).
Report is 10 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2270      +/-   ##
===========================================
+ Coverage    69.80%   69.90%   +0.10%     
===========================================
  Files          731      731              
  Lines        27156    27160       +4     
  Branches      4572     4575       +3     
===========================================
+ Hits         18954    18985      +31     
+ Misses        6919     6901      -18     
+ Partials      1283     1274       -9     
Files with missing lines Coverage Δ
...n/com/datadog/android/okhttp/DatadogInterceptor.kt 60.23% <100.00%> (+1.65%) ⬆️

... and 33 files with indirect coverage changes

@xgouchet xgouchet merged commit a6bd6a5 into develop Sep 19, 2024
23 checks passed
@xgouchet xgouchet deleted the xgouchet/RUM-6235/sse_request branch September 19, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Okhttp with DatadogInterceptor is blocking SSE streams
4 participants