-
-
Notifications
You must be signed in to change notification settings - Fork 389
feat(network-details): Hook up request/response capture in SentryNetworkTracker #7588
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
base: mobile-935/extract-network-details
Are you sure you want to change the base?
Changes from all commits
fc0cf85
1a824a3
c0312c4
ca57ac4
281ca89
c261036
ce99b96
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -236,6 +236,16 @@ - (void)urlSessionTask:(NSURLSessionTask *)sessionTask setState:(NSURLSessionTas | |
| return; | ||
| } | ||
|
|
||
| #if SENTRY_TARGET_REPLAY_SUPPORTED | ||
| SentryOptions *options = SentrySDK.startOption; | ||
sentry[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| NSString *urlString = sessionTask.currentRequest.URL.absoluteString; | ||
| if ([self isNetworkDetailCaptureEnabledFor:urlString options:options]) { | ||
| [self captureRequestDetails:sessionTask | ||
| networkCaptureBodies:options.sessionReplay.networkCaptureBodies | ||
| networkRequestHeaders:options.sessionReplay.networkRequestHeaders]; | ||
| } | ||
43jay marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| #endif // SENTRY_TARGET_REPLAY_SUPPORTED | ||
43jay marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+239
to
+247
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Sentry's own backend requests are not automatically excluded from network detail capture, unlike breadcrumbs. This relies on user configuration to prevent capturing Sentry's own traffic. Suggested FixAdd an explicit check to exclude Sentry backend URLs before calling Prompt for AI Agent |
||
|
|
||
| if (![self isTaskSupported:sessionTask]) { | ||
| return; | ||
| } | ||
|
|
@@ -562,15 +572,104 @@ - (SentryLevel)getBreadcrumbLevel:(NSURLSessionTask *)sessionTask | |
| return breadcrumbLevel; | ||
| } | ||
|
|
||
| #if SENTRY_TARGET_REPLAY_SUPPORTED | ||
| // Associated object key for attaching SentryReplayNetworkDetails to each NSURLSessionTask. | ||
| // Safe: setAssociatedObject follows existing patterns in urlSessionTask:setState: | ||
| // and getAssociatedObject is called from blocks that hold a strong reference to the task. | ||
| static const void *SentryNetworkDetailsKey = &SentryNetworkDetailsKey; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added ptal re parallel network calls -
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the plan with the thread safety? Is this still an open task of yours?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i had it locally. just pushed (details here) |
||
|
|
||
| - (BOOL)isNetworkDetailCaptureEnabledFor:(NSString *)urlString options:(SentryOptions *)options | ||
| { | ||
| if (!options) { | ||
| return NO; | ||
| } | ||
|
|
||
| if (!urlString) { | ||
| return NO; | ||
| } | ||
|
|
||
| if (!options.sessionReplay) { | ||
| return NO; | ||
| } | ||
|
|
||
| return [options.sessionReplay isNetworkDetailCaptureEnabledFor:urlString]; | ||
| } | ||
|
|
||
| - (void)captureResponseDetails:(NSData *)data | ||
| response:(NSURLResponse *)response | ||
| requestURL:(NSURL *)requestURL | ||
| task:(NSURLSessionTask *)task | ||
| { | ||
| // TODO: Implementation | ||
| // 2. Parse response body data | ||
| // 3. Store in appropriate location for session replay | ||
| // 4. Handle size limits and truncation if needed | ||
| NSString *urlString = requestURL.absoluteString; | ||
| SentryOptions *options = SentrySDK.startOption; | ||
| if (![self isNetworkDetailCaptureEnabledFor:urlString options:options]) { | ||
| return; | ||
| } | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| @synchronized(task) { | ||
| SentryReplayNetworkDetails *details | ||
| = objc_getAssociatedObject(task, &SentryNetworkDetailsKey); | ||
| if (!details) { | ||
| SENTRY_LOG_WARN(@"[NetworkCapture] No SentryReplayNetworkDetails found for %@ - " | ||
| @"skipping response capture", | ||
| urlString); | ||
| return; | ||
sentry[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| NSInteger statusCode = 0; | ||
| NSDictionary *allHeaders = nil; | ||
| NSString *contentType = nil; | ||
| if ([response isKindOfClass:[NSHTTPURLResponse class]]) { | ||
| NSHTTPURLResponse *httpResponse = (NSHTTPURLResponse *)response; | ||
| statusCode = httpResponse.statusCode; | ||
| allHeaders = httpResponse.allHeaderFields; | ||
| contentType = httpResponse.allHeaderFields[@"Content-Type"]; | ||
| } | ||
|
|
||
| NSData *bodyData | ||
| = (options.sessionReplay.networkCaptureBodies && data.length > 0) ? data : nil; | ||
|
|
||
| [details setResponseWithStatusCode:statusCode | ||
| size:@(data ? data.length : 0) | ||
| bodyData:bodyData | ||
| contentType:contentType | ||
| allHeaders:allHeaders | ||
| configuredHeaders:options.sessionReplay.networkResponseHeaders]; | ||
| } | ||
43jay marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| - (void)captureRequestDetails:(NSURLSessionTask *)sessionTask | ||
| networkCaptureBodies:(BOOL)networkCaptureBodies | ||
| networkRequestHeaders:(NSArray<NSString *> *)networkRequestHeaders | ||
| { | ||
| if (!sessionTask || !sessionTask.currentRequest) { | ||
| return; | ||
| } | ||
|
|
||
| NSURLRequest *request = sessionTask.currentRequest; | ||
| SentryReplayNetworkDetails *details; | ||
|
|
||
| @synchronized(sessionTask) { | ||
| if (objc_getAssociatedObject(sessionTask, &SentryNetworkDetailsKey)) { | ||
| return; | ||
| } | ||
| details = | ||
| [[SentryReplayNetworkDetails alloc] initWithMethod:request.HTTPMethod ?: @"GET"]; | ||
| objc_setAssociatedObject( | ||
| sessionTask, &SentryNetworkDetailsKey, details, OBJC_ASSOCIATION_RETAIN_NONATOMIC); | ||
| } | ||
|
|
||
| // Prefer originalRequest.HTTPBody: currentRequest may reflect redirects, and its HTTPBody may be nil on in-flight tasks. | ||
| NSData *rawBody = sessionTask.originalRequest.HTTPBody ?: request.HTTPBody; | ||
| NSNumber *requestSize = rawBody ? [NSNumber numberWithUnsignedInteger:rawBody.length] : nil; | ||
| NSData *bodyData = networkCaptureBodies ? rawBody : nil; | ||
|
|
||
| [details setRequestWithSize:requestSize | ||
| bodyData:bodyData | ||
| contentType:request.allHTTPHeaderFields[@"Content-Type"] | ||
| allHeaders:request.allHTTPHeaderFields | ||
| configuredHeaders:networkRequestHeaders]; | ||
| } | ||
| #endif // SENTRY_TARGET_REPLAY_SUPPORTED | ||
|
|
||
| @end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
h: Please double-check if this is the correct approach to get the options. other parts of this file useSENTRY_UNWRAP_NULLABLE(SentryOptions, SentrySDKInternal.currentHub.client.options)Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
afaict using SentrySDK.startOption is appropriate:
Majority of code that interacts with the options in SentryNetworkTracker.m does
SentrySDK.startOptionref overSentrySDKInternal.currentHub.client.optionsrefIt's the same instance in either case, just
SentrySDKInternal.currentHub.client.optionscan be swapped out if the hub changes, but this is only used in tests.If someone calls SentrySDK.start multiple times, the SentryNetworkTracker.m would pick up the options from the most recent call => seems appropriate.
Re
SENTRY_UNWRAP_NULLABLE... seems like it should beSENTRY_UNWRAP_NULLABLE(SentryOptions, SentrySDK.startOption)in case an objC caller manages to set aniloptions ... but the other call-sites omit theSENTRY_UNWRAP_NULLABLEso i omitted as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose fo SENTRY_UNWRAP_NULLABLE is just casting from a nullable type to-non-nullable type. We use this in cases where we already did a null check and are sure that it's not null. We use a macro because it allows us to find all cases again in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
understood. don't believe it is needed here -> there's no guarantee SentryOptions won't be null, as objC callers can do so (not that they would want to)