Repository navigation
fix: Expose an ObjC setter for consolePrint - #3120
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 582d1c2a83
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a0eaeccee
ℹ️ 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".
| case .error: .error | ||
| case .critical: .critical | ||
| } | ||
| block(message, objcLevel) |
There was a problem hiding this comment.
In Swift 6 builds of DatadogInternal, assigning this wrapper to DatadogInternal.consolePrint type-checks the wrapper as @Sendable because consolePrint is declared @Sendable; capturing the non-Sendable block here produces capture of 'block' with non-Sendable type ... in a '@Sendable' closure, so the module fails to build before ObjC callers can use the new API. Store the callback through a Sendable wrapper (or otherwise satisfy the @Sendable requirement) before assigning it.
AGENTS.md reference: AGENTS.md:L12-L12
Useful? React with 👍 / 👎.
| - [FIX] onSessionStart is now called only after sampling information used by WebView Tracking is in place, avoiding missing traces in early requests. See [#3104][] | ||
| - [FIX] Fix crash when defining `onSessionStart` in RUM configuration in Swift 6 projects. See [#3106][] | ||
| - [IMPROVEMENT] Forward `local_cache_hit` signal on RUM resources [#3074][] | ||
| - [IMPROVEMENT] Expose `setConsolePrint` to Objective-C, allowing ObjC client code to redirect SDK console output. See [#3120][] |
There was a problem hiding this comment.
It is internal. No need to be advertised.
There was a problem hiding this comment.
Whoops! Sorry, I missed the fact that this PR got approved, and it's just been sitting around for months 😓
I've rebased onto develop, and I've removed the edit to CHANGELOG.md since, as you say, it's an internal-only change.
There was also an earlier Codex review note about the method needing to be decorated @Sendable for compatibility with Swift 6, so I've addressed that as well.
## Background ### InternalLogger produces private messages to OSLog The SDK logs diagnostic output to the local device console via `InternalLogger`. By default, in iOS 14+, this implementation uses `OSLog`, and it flags all messages as `.private`. For a developer testing an iOS app, reading OSLog messages requires `Console.app` or `idevicesyslog`, and messages are redacted due to their privacy setting: ``` MyApp(DatadogInternal)[123] <Fault>: <private> ``` ### consolePrint can be overridden, but only in Swift The SDK allows an application to override this default behavior, redirecting SDK output as desired. This is achieved by overwriting the global `consolePrint` variable with a different function, which `InternalLogger` will then use. However, this override only works in Swift: global function-typed vars are not trivially exposed to Objective-C, and there's no existing setter function exposed in the Objective-C bindings. ## Changes in this PR This PR adds a single function to the ObjC bindings for `InternalLogger`: ``` public static func setConsolePrint(_ block: @escaping (String, objc_CoreLoggerLevel) -> Void) ``` ...where the provided `block` is wrapped and assigned to `DatadogInternal.consolePrint`. This allows Objective-C client code to override `consolePrint` in the same way that Swift client code can. ## Testing ### Unit test coverage The SDK's test suite has coverage for individual ObjC bindings, so I've added a simple test that verifies the expected behavior of `setConsolePrint`. ### Empirical test In my test application, I provided a `consolePrint` callback that routes output to `NSLog`: ```objc [DDDatadog setVerbosityLevel:DDCoreLoggerLevelDebug]; [DDInternalLogger setConsolePrint:^(NSString* message, DDCoreLoggerLevel level) { NSLog(@"[Datadog] %@", message); }]; ``` ...and as a result, when I ran my app with `devicectl device process launch --console`, I got unredacted output from the SDK: ``` MyApp[123:54321] [Datadog] 🔥 Datadog SDK usage error: `clientToken` cannot be empty. ```
7a0eaec to
f110380
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
`DatadogInternal.consolePrint` is `@Sendable`, since the SDK invokes it from background queues, but the wrapper assigned to it captured a non-Sendable block. This compiled in the Swift 5 language mode, but it's an error in Swift 6 (and a warning under complete concurrency checking). Require callers to pass a `@Sendable` block, and update the test to record into a `ReadWriteLock` rather than mutating captured locals.
Background
InternalLogger produces private messages to OSLog
The SDK logs diagnostic output to the local device console via
InternalLogger. By default, in iOS 14+, this implementation usesOSLog, and it flags all messages as.private.For a developer testing an iOS app, reading OSLog messages requires
Console.apporidevicesyslog, and messages are redacted due to their privacy setting:consolePrint can be overridden, but only in Swift
The SDK allows an application to override this default behavior, redirecting SDK output as desired. This is achieved by overwriting the global
consolePrintvariable with a different function, whichInternalLoggerwill then use.However, this override only works in Swift: global function-typed vars are not trivially exposed to Objective-C, and there's no existing setter function exposed in the Objective-C bindings.
Changes in this PR
This PR adds a single function to the ObjC bindings for
InternalLogger:...where the provided
blockis wrapped and assigned toDatadogInternal.consolePrint.This allows Objective-C client code to override
consolePrintin the same way that Swift client code can.Testing
Unit test coverage
The SDK's test suite has coverage for individual ObjC bindings, so I've added a simple test that verifies the expected behavior of
setConsolePrint.Empirical test
In my test application, I can now provide a
consolePrintcallback that routes output toNSLog:...and as a result, when I run my app with
devicectl device process launch --console, I get unredacted output from the SDK:Review checklist
make api-surfacewhen adding new APIs