RUM-15591: Expose request/response payloads on HttpRequestInfo/HttpResponseInfo - #3714
RUM-15591: Expose request/response payloads on HttpRequestInfo/HttpResponseInfo#3714kikoveiga wants to merge 1 commit into
request/response payloads on HttpRequestInfo/HttpResponseInfo#3714Conversation
…tpResponseInfo The deprecated RumResourceAttributesProvider overload handed customers the raw okhttp3.Request/Response, so they could read the payloads themselves. The library-agnostic HttpRequestInfo/HttpResponseInfo replacement exposed no way to do that (#3365). Add peekBody(maxBytes) to both core interfaces, returning an HttpBodySnapshot that carries the bytes, the declared content type and an isTruncated flag. Reading is lazy and non-consuming: nothing is copied unless the method is called, and the request stays sendable / the response stays readable. The default cap is 512 KB, matching dd-sdk-ios#3019. OkHttp implements both sides. Requests are written through a truncating sink so a large upload never allocates more than the requested maximum, and one-shot and duplex bodies are skipped because writing them would break the request. Responses go through Response.peekBody, skipping streams and WebSockets whose payloads would never complete on their own. Cronet returns null on both, documented at the override: it streams payloads directly between the application and the network stack, so exposing them would require buffering a copy of every request whether or not a snapshot is ever asked for. That trade-off is left to a follow-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🎉 All green!🧪 All tests passed 🔄 Datadog auto-retried 1 job - 1 passed on retry 🎯 Code Coverage (details) 🔗 Commit SHA: d8ec45a | Docs | Datadog PR Page | Give us feedback! |
| import java.util.Locale | ||
|
|
||
| /** | ||
| * An immutable, size-capped copy of an HTTP request or response payload. |
There was a problem hiding this comment.
if it's immutable - maybe better to declare as data class
| private const val HASH_MULTIPLIER = 31 | ||
| private const val CHARSET_PARAMETER = "charset" | ||
|
|
||
| private fun resolveCharset(contentType: String?): Charset { |
There was a problem hiding this comment.
I am bit afraid this method might be silently complex from the memory perspective because of transformations.
For instance - firstOrNull { it.lowercase(Locale.US) } - will create a String object for each element of array.
?.map { it.trim() } - as well
and so on.
so in case if this method would be called multiple times in a short period - it could create a pressure to the GC.
I am not sure that you could completley avoid that, but manual iteration over the string, might be cheaper. Good test coverage would be required as well for all supported HTTP header format
There was a problem hiding this comment.
startsWith(.., ignoreCase = true) could help here, it delegates down to String.regionMatches, which does iteration without us blowing up the method complexity (still pay the trim cost though)
Then you can defer the lowercasing cost only when a match is found
"$CHARSET_PARAMETER=" should probably also be a const so you don't pay the cost of building the search string sequence iteration
| * every upload as it flows past, at a memory cost paid by every request whether or not a | ||
| * snapshot is ever asked for. | ||
| */ | ||
| override fun peekBody(maxBytes: Long): HttpBodySnapshot? = null |
There was a problem hiding this comment.
if this api could not be supported for both implementation it's better to move peekBody only to OkHttpRequestInfo and add extension method peakBody that could check if given HttpRequestInfo is OkHttpRequestInfo then cast and call, otherwise return null.
| * as it streams past, at a memory cost paid by every request whether or not a snapshot is ever | ||
| * asked for. | ||
| */ | ||
| override fun peekBody(maxBytes: Long): HttpBodySnapshot? = null |
There was a problem hiding this comment.
same here:
if this api could not be supported for both implementation it's better to move peekBody only to OkHttpRequestInfo and add extension method peakBody that could check if given HttpRequestInfo is OkHttpRequestInfo then cast and call, otherwise return null.
| override fun newBuilder() = OkHttpRequestInfoBuilder(originalRequest.newBuilder()) | ||
|
|
||
| internal companion object { | ||
| internal const val ERROR_PEEK_REQUEST_BODY = "Unable to peek request body." |
There was a problem hiding this comment.
if companion object is internal should we make ERROR_PEEK_REQUEST_BODY internal as well?
| } | ||
| } | ||
|
|
||
| override fun flush() = Unit |
There was a problem hiding this comment.
seems like flush/close methods should somehow affect internal buffer? like cleaning it ? Otherwise there is a risk for memory leaks
|
|
||
| // A snapshot is backed by a ByteArray, which cannot hold more than Int.MAX_VALUE bytes. | ||
| // One byte is kept in reserve for the truncation probe in peekBody. | ||
| private const val MAX_SNAPSHOT_BYTES: Long = Int.MAX_VALUE.toLong() - 1L |
There was a problem hiding this comment.
Are you that it's safe to let buffer be limited with Int.MAX_VALUE? ~2GB
3 participants
What does this PR do?
Add
peekBody(maxBytes)to both core interfaces, returning anHttpBodySnapshotthat carries the bytes, the declared content type and anisTruncatedflag. Reading is lazy and non-consuming: nothing is copied unless the method is called, and the request stays sendable / the response stays readable. The default cap is 512 KB, matching dd-sdk-ios#3019.OkHttp implements both sides while Cronet returns null, because it streams payloads directly between the application and the network stack, so exposing them would require buffering a copy of every request whether or not a snapshot is ever asked for. That trade-off is left to a follow-up.
Motivation
The deprecated
RumResourceAttributesProvideroverload handed customers the rawokhttp3.Request/Response, so they could read the payloads themselves. The new library-agnostic instrumentation didn't have this option.Review checklist (to be filled by reviewers)