[CHNL-29461] Add on/off switch for logging capabilities#523
[CHNL-29461] Add on/off switch for logging capabilities#523
Conversation
Add KlaviyoLogConfig with a thread-safe isLoggingEnabled toggle that controls all Logger output, LoggerClient.error, and runtimeWarn() across KlaviyoCore, KlaviyoSwift, KlaviyoForms, and KlaviyoLocation modules. When disabled, Logger computed properties return Logger(OSLog.disabled), which the OS optimises to a no-op. When re-enabled, logging resumes immediately. Public API on KlaviyoSDK: - isLoggingEnabled: Bool (read-only property) - setLoggingEnabled(_:) -> KlaviyoSDK (chainable) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // MARK: - Logging Configuration | ||
|
|
||
| /// Thread-safe global logging toggle for the Klaviyo SDK. | ||
| /// |
There was a problem hiding this comment.
Consider using package access level instead of public.
Since the package uses swift-tools-version: 5.9, you can use the package access level here. This would make KlaviyoLogConfig accessible from all modules within the package (KlaviyoCore, KlaviyoSwift, KlaviyoForms, KlaviyoLocation) but not from external SDK consumers.
Currently, with public access, SDK consumers can bypass the intended KlaviyoSDK.setLoggingEnabled(_:) API and directly call KlaviyoLogConfig.shared.isLoggingEnabled = false. Using package access would enforce the designed API surface:
package final class KlaviyoLogConfig: @unchecked Sendable {
package static let shared = KlaviyoLogConfig()
private let lock = NSLock()
private var _isLoggingEnabled: Bool = true
private init() {}
package var isLoggingEnabled: Bool {
// ...
}
}This ensures consumers use KlaviyoSDK().setLoggingEnabled(_:) as intended.
There was a problem hiding this comment.
Addressed in da46dc7 — Changed KlaviyoLogConfig from public to package access level (class, shared instance, and isLoggingEnabled property). SDK consumers must now use KlaviyoSDK().setLoggingEnabled(_:) as the intended API surface.
| // Created by Andrew Balmer on 1/28/25. | ||
| // | ||
|
|
||
| import KlaviyoCore |
There was a problem hiding this comment.
Missing explicit dependency in Package.swift.
This file now imports KlaviyoCore directly, but the KlaviyoForms target in Package.swift only declares KlaviyoSwift as a dependency:
.target(
name: "KlaviyoForms",
dependencies: ["KlaviyoSwift"], // ← no explicit KlaviyoCore
...
)This works today because KlaviyoSwift transitively depends on KlaviyoCore, but relying on transitive imports is fragile. If KlaviyoSwift ever refactored its dependency graph, this would silently break.
Please add "KlaviyoCore" as an explicit dependency:
.target(
name: "KlaviyoForms",
dependencies: ["KlaviyoSwift", "KlaviyoCore"],
...
)There was a problem hiding this comment.
Addressed in da46dc7 — Added "KlaviyoCore" as an explicit dependency for the KlaviyoForms target in Package.swift.
ndurell
left a comment
ndurell
left a comment
There was a problem hiding this comment.
AI Review — Round 1
Posted 2 inline comment(s) on specific lines. Please address each one.
Summary of findings:
-
Package.swift: Missing explicit KlaviyoCore dependency for KlaviyoForms — The PR adds
import KlaviyoCoreto KlaviyoForms but doesn't update Package.swift to declare the direct dependency. It compiles via transitive dependency through KlaviyoSwift, but this should be made explicit. -
KlaviyoLogConfig: Consider
packageaccess level — Since the package uses swift-tools-version 5.9,KlaviyoLogConfigcan usepackageaccess instead ofpublicto prevent SDK consumers from bypassing the intendedKlaviyoSDK.setLoggingEnabled(_:)API.
What looks good:
- Thread-safe implementation with NSLock +
@unchecked Sendableis correct - All four module Logger extensions consistently updated to computed properties
- LoggerClient.production.error and runtimeWarn() properly gated
- Chainable API follows existing KlaviyoSDK patterns exactly
- Test coverage is thorough (thread safety, toggle behavior, SDK integration)
- Default behavior preserved (logging enabled)
- No breaking changes
Posted by VK Conductor PR Review agent. Addressing this feedback will trigger another review cycle.
- Add explicit KlaviyoCore dependency for KlaviyoForms target in Package.swift - Change KlaviyoLogConfig access level from public to package to enforce KlaviyoSDK as the intended API surface - Gate EventBuffer.swift Logger instance by logging toggle - Add logging state reset in KlaviyoSDKTests tearDown for robustness Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
AI Review — Round 2LGTM — no new issues found. All feedback from Round 1 and Bugbot has been properly addressed in da46dc7:
All acceptance criteria verified:
Posted by VK Conductor PR Review agent. |
Gate the vendor runtimeWarn function in KlaviyoSwift/Vendor/ComposableArchitecture/Misc.swift with KlaviyoLogConfig.shared.isLoggingEnabled, ensuring all runtime warnings across both KlaviyoCore and KlaviyoSwift modules are silenced when logging is disabled. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ndurell
left a comment
ndurell
left a comment
There was a problem hiding this comment.
AI Review — Round 3
LGTM — no new issues found. All prior feedback (Round 1 AI review, Bugbot rounds 1 & 2) has been properly addressed across commits da46dc7 and 1b32912:
- Package.swift: KlaviyoForms declares explicit KlaviyoCore dependency ✓
- KlaviyoLogConfig: Uses
packageaccess level ✓ - EventBuffer.swift: Logger converted to toggle-aware computed property ✓
- KlaviyoSDKTests: tearDown resets logging state ✓
- Vendor runtimeWarn: Gated by
KlaviyoLogConfig.shared.isLoggingEnabled✓
All acceptance criteria verified:
setLoggingEnabled(_:)— present, chainable,@discardableResultisLoggingEnabled— present, read-only- Default enabled —
_isLoggingEnabled = true - All loggers silenced when disabled — KlaviyoCore, KlaviyoSwift, KlaviyoForms, KlaviyoLocation, LoggerClient, both runtimeWarn copies
- Re-enable works immediately — computed properties
- Thread-safe — NSLock +
@unchecked Sendable+ concurrent tests
Proceeding to CI.
Posted by VK Conductor PR Review agent.
The @_transparent attribute causes compile errors when referencing package-scoped KlaviyoLogConfig from the inlined function body. Removing it while keeping @inline(__always) preserves the inlining optimization without the access-level conflict. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
| } | ||
| } | ||
|
|
||
| @_transparent |
There was a problem hiding this comment.
Removed @_transparent from vendored TCA runtimeWarn
Low Severity
The @_transparent attribute was removed from the vendored TCA runtimeWarn function when adding the KlaviyoLogConfig guard. @_transparent forces SIL-level inlining even in -Onone (debug) builds, while @inline(__always) is only a hint the optimizer may ignore in unoptimized builds. Since runtimeWarn emits os_log(.fault) with the com.apple.runtime-issues subsystem specifically to surface warnings in Xcode's issue navigator at the call site, losing guaranteed inlining in debug builds could cause those runtime warnings to be attributed to the wrong source location. The KlaviyoCore version of runtimeWarn in LoggerClient.swift never had @_transparent, so this may have been an intentional consistency choice — but it's worth confirming.
|
Note on The
|
|
testing this, and so far it looks good -- should we also bridge this to RN? and update the README to mention it somewhere? |
Package.swift
Outdated
| .target( | ||
| name: "KlaviyoForms", | ||
| dependencies: ["KlaviyoSwift"], | ||
| dependencies: ["KlaviyoSwift", "KlaviyoCore"], |
There was a problem hiding this comment.
tiniest lil nit in the world, but for formatting could we put each dependency on a new line like KlaviyoFormsTest, KlaviyoLocation, and KlaviyoLocationTests -- above targets are also inconsistent
There was a problem hiding this comment.
Addressed in 952fc83 — Formatted all target dependency arrays in Package.swift to one dependency per line for consistency. Updated KlaviyoCore, KlaviyoSwift, and KlaviyoForms targets to match the multi-line style already used by KlaviyoFormsTests, KlaviyoLocation, and KlaviyoLocationTests.
Woah thanks for testing! Yes can do that next! |
…pendencies Put each dependency on its own line for consistency across all targets, as requested by reviewer. This aligns KlaviyoCore, KlaviyoSwift, and KlaviyoForms target formatting with KlaviyoFormsTests, KlaviyoLocation, and KlaviyoLocationTests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
||
| @available(iOS 14.0, *) | ||
| extension Logger { | ||
| private static var subsystem = Bundle.main.bundleIdentifier ?? "" |
There was a problem hiding this comment.
just now catching this -- not major but shouldn't this be "com.klaviyo.klaviyo-swift-sdk.klaviyoLocation" to be consistent with the other subsystems
ab1470
left a comment
ab1470
left a comment
There was a problem hiding this comment.
This looks great to me! Thanks for picking this up
3 participants
Description
Adds a configurable logging toggle to the Klaviyo Swift SDK, allowing developers to enable or disable all SDK logging at runtime. This addresses GitHub Issue #475.
Changes
Sources/KlaviyoCore/Utils/Logger+Ext.swift: AddedKlaviyoLogConfig— a thread-safe singleton with anisLoggingEnabledflag. Convertedstatic letLogger instances to computedstatic varproperties that returnLogger(OSLog.disabled)when logging is disabled.Sources/KlaviyoSwift/Utilities/Logger+Ext.swift: Convertedstatic letloggers to computed properties gated on the toggle.Sources/KlaviyoForms/Utilities/Logger+Ext.swift: Convertedstatic letloggers to computed properties gated on the toggle.Sources/KlaviyoLocation/Utilities/Logger+Ext.swift: Convertedstatic letlogger to computed property gated on the toggle.Sources/KlaviyoCore/Utils/LoggerClient.swift: GatedLoggerClient.production.errorandruntimeWarn()behind the toggle.Sources/KlaviyoSwift/Klaviyo.swift: Added public API —isLoggingEnabled(read-only) andsetLoggingEnabled(_:)(chainable).New Public API
Checklist
Authors and Reviewers, please ensure the following:
General
Tests
Test Plan
Test Plansection below, including evidence that it was carried out)Risk Impact
NSLock+Boolcheck is sub-microsecond. Logger instances are now computed properties instead of stored, butLogger(OSLog.disabled)is optimized by the OS.isLoggingEnabled = true, preserving current behavior. Safe to revert.Test Plan
Automated implementation via AI Agent for CHNL-29461.
Changes implemented:
KlaviyoLogConfigthread-safe singleton inSources/KlaviyoCore/Utils/Logger+Ext.swiftLoggerClient.production.errorandruntimeWarn()on the togglesetLoggingEnabled(_:)andisLoggingEnabledtoKlaviyoSDKTesting performed:
Tests/KlaviyoCoreTests/KlaviyoLogConfigTests.swift:Tests/KlaviyoSwiftTests/KlaviyoSDKTests.swift:isLoggingEnableddefaults to truesetLoggingEnabled(false)disablessetLoggingEnabledis chainableNote: Pre-commit hooks (SwiftLint/SwiftFormat) could not be run locally due to environment setup issues (SwiftLint build failure in pre-commit sandbox). CI should run these checks.
Canary / Manual Verification
This change is fully covered by automated tests. Manual verification can be done by:
KlaviyoSDK().setLoggingEnabled(false)in a test appKlaviyoSDK().setLoggingEnabled(true)and verifying logs resumeDeploy Plan
Standard deployment — merge when CI passes and reviews are complete.
If a rollback is needed: revert this PR and the previous behavior will be restored.
Post-Deploy Monitoring
Closes CHNL-29461
🤖 Generated with VK Conductor