feat: refactor network logs instrumentation #260
Conversation
Instructions and example for changelogPlease add an entry to ## Next
- refactor network logs instrumentation ([#260](https://github.com/PostHog/posthog-ios/pull/260))If none of the above apply, you can opt out of this check by adding |
| @@ -12,3 +12,10 @@ func swizzle(forClass: AnyClass, original: Selector, new: Selector) { | |||
| guard let swizzledMethod = class_getInstanceMethod(forClass, new) else { return } | |||
| method_exchangeImplementations(originalMethod, swizzledMethod) | |||
| } | |||
|
|
|||
| func swizzleClassMethod(forClass: AnyClass, original: Selector, new: Selector) { | |||
There was a problem hiding this comment.
maybe rename the one above to make more clear its about instance method?
| // don't record response body for image types | ||
| let bodyStr = requestData.base64EncodedString(options: .endLineWithLineFeed) | ||
| requestBodyLength = bodyStr.count |
There was a problem hiding this comment.
body length in bytes is != than base64 encoded char count.
we can probably read Content-Length here or just count bytes from requestData
| } | ||
|
|
||
| // grab request body | ||
| if let requestData = request.httpBody ?? request.httpBodyStream?.consume() { |
There was a problem hiding this comment.
since this has a performance impact + PII risk, we should add a new flag captureBody as part of session replay config
| if let responseData = responseData as? Data { | ||
| if let responseContentType, responseContentType == .image { | ||
| // don't record response body for image types | ||
| let bodyStr = responseData.base64EncodedString(options: .endLineWithLineFeed) | ||
| responseBodyLength = bodyStr.count | ||
| } else if let utfString = String(data: responseData, encoding: String.Encoding.utf8) { | ||
| responseBodyStr = utfString | ||
| responseBodyLength = utfString.count | ||
| } | ||
| } |
There was a problem hiding this comment.
not sure if 100% the same but we can extract from start and reuse it here? if responseData and requestData are of the same type
| } | ||
| } | ||
|
|
||
| private func getMonotonicTimeInMilliseconds() -> UInt64 { |
There was a problem hiding this comment.
getMonotonicTimeInMilliseconds already exists, can we reuse it? See URLSession extension
| requestStartTime = getMonotonicTimeInMilliseconds() | ||
| requestURL = request.url?.absoluteURL | ||
| requestMethod = request.httpMethod | ||
| requestHeaders = request.normalizedHeaderFields ?? [:] |
There was a problem hiding this comment.
similar to https://github.com/PostHog/posthog-ios/pull/260/files#r1851930833
we need a captureHeaders boolean config, plus removing some sensitive headers eg https://github.com/PostHog/posthog-js/blob/d71ecd6b5dd69c8f19fb7c957c55a6888bc76186/src/extensions/replay/config.ts#L53-L67
|
|
||
| if session == nil { | ||
| // ⚠️ - This is currently always using a fresh session with the `default` configuration | ||
| // - Need to figure out a way to use custom sessions and configurations here |
There was a problem hiding this comment.
That's one of the reasons I gave up on URLProtocol back then since it'd require figuring out more stuff rather than inspiring from the current DD approach.
We need to be sure that React native works as well, I'd need to check if they use a custom or not.
| } | ||
|
|
||
| private static func canHandle(task: URLSessionTask) -> Bool { | ||
| if #available(iOS 13.0, macOS 10.15, *) { |
There was a problem hiding this comment.
we dont need this I think
.macOS(.v10_15), .iOS(.v13), .tvOS(.v13), .watchOS(.v6),
so the SDK is always running on >= than that
| let bodyStr = responseData.base64EncodedString(options: .endLineWithLineFeed) | ||
| responseBodyLength = bodyStr.count | ||
| } else if let utfString = String(data: responseData, encoding: String.Encoding.utf8) { | ||
| responseBodyStr = utfString |
There was a problem hiding this comment.
There was a problem hiding this comment.
There was a problem hiding this comment.
|
|
||
| snapshotsData.append(data) | ||
|
|
||
| PostHogSDK.shared.capture("$snapshot", properties: [ |
There was a problem hiding this comment.
we need to check isCaptureNetworkEnabled before processing a request and before capturing so we are sure we don't capture when there's no active session
|
@ioannisj left a few comments, no need to address all of them, more about sharing some context around a few items. One suggestion is to not capture headers and body for now, so we can test/ship this PR faster. This comment is important to figure out. Make sure we fix the side effect of this issue. |
Yeah, that could potentially be a blocker for this approach actually. Looking into it |
💡 Motivation and Context
Closes #204
Also some work on #159
This is an initial implementation of using a custom URLProtocol to intercept network traffic.
TODO:
PostHogHTTPProtocolcurrently uses it's own instance for URLSession to carry out the network. This should not be the casePOST,PUTetc)💚 How did you test it?
Sample project with async/await and traditional URLSession
📝 Checklist