RUM-14782: Capture OkHttp Network Headers#3204
Conversation
4ed9e06 to
a082ecb
Compare
This comment has been minimized.
This comment has been minimized.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #3204 +/- ##
===========================================
+ Coverage 71.41% 71.45% +0.04%
===========================================
Files 933 936 +3
Lines 34591 34682 +91
Branches 5849 5866 +17
===========================================
+ Hits 24701 24780 +79
- Misses 8253 8263 +10
- Partials 1637 1639 +2
🚀 New features to boost your workflow:
|
998a873 to
789aef2
Compare
d99452e to
de59997
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de599972f2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
...android-rum/src/main/kotlin/com/datadog/android/rum/internal/net/ResourceHeadersExtractor.kt
Outdated
Show resolved
Hide resolved
de59997 to
8aed174
Compare
| const val GRAPHQL_ERRORS: String = "_dd.graphql.errors" | ||
|
|
||
| /** | ||
| * The request headers of the resource. (Map<String, String>) |
There was a problem hiding this comment.
It is not quite clear what is expected to be key and value of this map, given that per HTTP spec headers can be repetitive, so if header name is the key, the value is List<String>. In such case how the values are joined to the single String, what is the separator?
There was a problem hiding this comment.
We will use comma-separated values, as also shown in the picture of the PR description.
...id-rum/src/main/kotlin/com/datadog/android/rum/configuration/ResourceHeadersConfiguration.kt
Outdated
Show resolved
Hide resolved
...id-rum/src/main/kotlin/com/datadog/android/rum/configuration/ResourceHeadersConfiguration.kt
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| val requestHeaders = if (includeDefaults) { | ||
| (DEFAULT_REQUEST_HEADERS + filteredCustom).distinct() |
There was a problem hiding this comment.
we can simply use Set then instead of List for the customHeaders property
There was a problem hiding this comment.
Maybe it's useful to keep the order of customHeaders the customer has decided. With a set, we would lose order and still need to deduplicate when joining default and custom headers.
There was a problem hiding this comment.
I also have question here, what' the point to keep the order of customer's headers, can you elaborate? also List will allow duplication, which seems not necessary
There was a problem hiding this comment.
In cases where the 2 KB limit for all headers might be exceeded, we want to keep first the DEFAULT_HEADERS and then customer's customHeaders. While the default ones are ordered alphabetically, the customer can decide to have some order on the custom ones so they prefer to have more chances of keeping this or that header (even though this is not explicit in the captureHeaders api. If this is not relevant, then yes we could just switch to using a set.
...android-rum/src/main/kotlin/com/datadog/android/rum/internal/net/ResourceHeadersExtractor.kt
Outdated
Show resolved
Hide resolved
...android-rum/src/main/kotlin/com/datadog/android/rum/internal/net/ResourceHeadersExtractor.kt
Outdated
Show resolved
Hide resolved
...android-rum/src/main/kotlin/com/datadog/android/rum/internal/net/ResourceHeadersExtractor.kt
Outdated
Show resolved
Hide resolved
...id-rum/src/test/kotlin/com/datadog/android/rum/internal/domain/scope/RumResourceScopeTest.kt
Outdated
Show resolved
Hide resolved
...oid-rum/src/test/kotlin/com/datadog/android/rum/internal/net/ResourceHeadersExtractorTest.kt
Outdated
Show resolved
Hide resolved
87abd2b to
4fdeb64
Compare
...android-rum/src/main/kotlin/com/datadog/android/rum/internal/net/ResourceHeadersExtractor.kt
Outdated
Show resolved
Hide resolved
...android-rum/src/main/kotlin/com/datadog/android/rum/internal/net/ResourceHeadersExtractor.kt
Outdated
Show resolved
Hide resolved
...tions/dd-sdk-android-okhttp/src/main/kotlin/com/datadog/android/okhttp/DatadogInterceptor.kt
Outdated
Show resolved
Hide resolved
4fdeb64 to
a5ccb6e
Compare
a5ccb6e to
6db4618
Compare
202a9b0 to
1cbdfc3
Compare
8173492 to
b2715fd
Compare
… ResourceHeadersExtractor
b2715fd to
70e7274
Compare
What does this PR do?
trackResourceHeaderspublic api inDatadogInterceptor.RumAttributes.RumResourceScopeand add them to theresource.ResourceHeadersExtractor.Motivation
Customers weren't able to easily capture network headers, having to do it manually (e.g., by creating custom attributes).
Additional notes
Here is an example taken from the sample app:
This is aligned with the iOS implementation:
Review checklist (to be filled by reviewers)