Conversation
shouldRecordPayload is asked about the session, and a session is normally shared across everything an app does, so it can only answer for all requests at once. An app that wants bodies from one endpoint has to record them from all of them. Adds shouldRecordPayloadForRequest, asked first and falling back to the per-session callback when it is not implemented or returns nil, so existing behaviour is unchanged.
| /// per-session callback. | ||
| private func shouldRecordPayload(for session: URLSession, taskId: String?) -> Bool { | ||
| let config = configuration | ||
| if let perRequest = config.shouldRecordPayloadForRequest, |
There was a problem hiding this comment.
Can this gate the completion-handler paths too? I merged this with #1163 locally, and those wrappers still call shouldRecordPayload(session) directly, so shouldRecordPayloadForRequest is ignored for data, download, and upload tasks using completion handlers. A test for that path would pin the new API consistently.
There was a problem hiding this comment.
Good catch — you're right, I only changed the delegate guards. I pulled #1163 into this branch like you did locally and routed all four call sites through the same helper, so the per-request callback now decides for the completion handler paths too. Added a test for a completion handler task that checks the callback is asked and the body actually comes through. Note this branch now carries #1163's commits, so if that one lands first I'll rebase and they'll drop out.
| /// A session is usually shared across every call an app makes, so `shouldRecordPayload` can only | ||
| /// answer for all of them at once. This is consulted first, and `shouldRecordPayload` is used when | ||
| /// it is not implemented or returns nil, so existing behaviour is unchanged. | ||
| public var shouldRecordPayloadForRequest: ((URLRequest) -> (Bool)?)? |
There was a problem hiding this comment.
Please add this callback to Sources/Instrumentation/URLSession/README.md too. That guide lists the configuration callbacks but still only exposes the session-level payload gate.
There was a problem hiding this comment.
Added it, plus a line on shouldRecordPayload saying it gates every path, not just the delegate one. Worth flagging that #1168 changes receivedResponse to take the request, so that README entry goes stale too — I'll update it there.
… paths shouldRecordPayload is opt-in and defaults to off, and the session delegate path checks it before accumulating a body. The completion handler paths passed the response body straight to receivedResponse and receivedError regardless, so an application that never enabled payload recording still had bodies handed to its telemetry callbacks. Withhold the payload from those callbacks unless recording is enabled. The caller's own completion handler still receives its data unchanged.
The documentation said the callback was only necessary when using a session delegate, which is no longer accurate now that the completion handler paths use it as their opt-in gate too. Also states what it does not affect, since the caller's own completion handler still receives its data either way.
The completion handler wrappers asked shouldRecordPayload about the session directly, so shouldRecordPayloadForRequest was ignored for data, download and upload tasks created with a handler. They now go through the same helper as the session delegate path. Also documents both callbacks in the URLSession README, which only listed the per-session gate.
# Conflicts: # Tests/InstrumentationTests/URLSessionTests/URLSessionInstrumentationTests.swift
| `shouldInstrument: ((URLRequest) -> (Bool)?)?` : Filter which requests you want to instrument, all by default | ||
|
|
||
| `shouldRecordPayload: ((URLSession) -> (Bool)?)?`: Implement if you want the session to record payload data, false by default. | ||
| `shouldRecordPayload: ((URLSession) -> (Bool)?)?`: Implement if you want the session to record payload data, false by default. It gates the payload passed to `receivedResponse` and `receivedError` on every path, whether the request uses the session delegate, a completion handler or async/await. |
There was a problem hiding this comment.
Can we either support async/await here or remove it from this sentence? AsyncTaskDelegate always passes dataOrFile: nil and never calls either payload predicate. I reproduced it with data(from:): the per-request callback was not invoked and receivedResponse got no payload.
# Conflicts: # Tests/InstrumentationTests/URLSessionTests/URLSessionInstrumentationTests.swift
… implying it works The line claimed the gate applies on every path including async/await, but AsyncTaskDelegate always reports nil payload and never consults either predicate, so async requests report no payload whatever the setting is.
aranhave
left a comment
aranhave
left a comment
There was a problem hiding this comment.
Looks good! Just need to fix the conflicts.
3 participants
Last of the batch from our SDK.
shouldRecordPayloadasks about the session. In our app, and I'd guess in most, one session is shared across everything, so that callback can only answer for all requests at once. We want bodies from a couple of endpoints and definitely not from the rest, and there's no way to say that today — it's all or nothing.So this adds
shouldRecordPayloadForRequest, which gets the request instead of the session:It's asked first, and falls back to
shouldRecordPayloadwhen it isn't implemented or returns nil, so nothing changes for anyone already using the session-level one.Two notes:
shouldRecordPayloadat all. If you'd rather land that one first and have me rebase this on top, that's fine by me.HttpTestServerreturns one, so I added a smallURLProtocolstub scoped to a single sentinel host (payload.stub). It only claims requests for that host, so registering it can't affect anything else in the suite. Happy to move it somewhere more general if you'd prefer it reusable.Test asserts the callback is asked with the request that's being sent, and fails if the per-request lookup is skipped. URLSession suite green, full package suite green (597 tests).
🤖 Generated with Claude Code