Skip to content

Commit 9e22ef4

Browse files
thisisabhashthisisabhash
committed
chore(logging): address review comments (#4218)
* chore(logging): use unique identifier per client for log streams and local storage path * enable unit tests in ci/cd * revert storage path identifier changes * fix swiftformat issues * enable flaky test and add doc comments * update Package.resolved
1 parent f7ac080 commit 9e22ef4

17 files changed

Lines changed: 198 additions & 139 deletions

File tree

‎.github/workflows/unit_test.yml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ jobs:
7070
AWSPluginsCore,
7171
AWSAPIPlugin,
7272
AWSCloudWatchLoggingPlugin,
73+
AmplifyCloudWatchLoggingClient,
7374
AWSCognitoAuthPlugin,
7475
AWSDataStorePlugin,
7576
AmplifyRecordCache,
@@ -102,6 +103,7 @@ jobs:
102103
{ scheme: AWSPluginsCore, flags: 'AWSPluginsCore,unit_tests' },
103104
{ scheme: AWSAPIPlugin, flags: 'API_plugin_unit_test,unit_tests' },
104105
{ scheme: AWSCloudWatchLoggingPlugin, flags: 'Logging_plugin_unit_test,unit_tests' },
106+
{ scheme: AmplifyCloudWatchLoggingClient, flags: 'CloudWatchLogging_client_unit_test,unit_tests' },
105107
{ scheme: AWSCognitoAuthPlugin, flags: 'Auth_plugin_unit_test,unit_tests' },
106108
{ scheme: AWSDataStorePlugin, flags: 'DataStore_plugin_unit_test,unit_tests' },
107109
{ scheme: AmplifyRecordCache, flags: 'RecordCache_unit_test,unit_tests' },

‎AmplifyClients/AmplifyCloudWatchLoggingClient/Sources/AmplifyCloudWatchLoggingClient.swift‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,11 @@ public typealias AmplifyCloudWatchLoggingClientConfigurationProvider = (
2828
/// Conforms to `LogSinkBehavior` so it can be registered with `AmplifyLogging.addSink()`
2929
/// to capture all framework log messages and forward them to CloudWatch.
3030
///
31+
/// - Important: Use a single client instance per (region, log group). CloudWatch log
32+
/// streams are keyed by device and user identifier, not by client instance, so two
33+
/// clients targeting the same region and log group would write to the same streams
34+
/// and share the same local storage directory, resulting in interleaved writes.
35+
///
3136
/// Example usage:
3237
/// ```swift
3338
/// let loggingClient = AmplifyCloudWatchLoggingClient(
@@ -117,7 +122,7 @@ public final class AmplifyCloudWatchLoggingClient: AmplifyFoundation.LogSinkBeha
117122
self.localStoreMaxSizeInMB = options.localStoreMaxSizeInMB
118123
let credentialIdentityResolver = FoundationToSDKCredentialsAdapter(provider: credentialsProvider)
119124
self.networkMonitor = NWPathMonitor()
120-
self.networkMonitor.startMonitoring(
125+
networkMonitor.startMonitoring(
121126
using: DispatchQueue(label: "com.amazonaws.amplify.cloudwatchlogging.networkmonitor")
122127
)
123128

@@ -263,7 +268,7 @@ private extension NSLock {
263268

264269
@available(iOS 13.0, macOS 12.0, tvOS 13.0, watchOS 9.0, *)
265270
extension AmplifyCloudWatchLoggingClient: CloudWatchLoggingMonitorDelegate {
266-
package func handleAutomaticFlushIntervalEvent() {
271+
func handleAutomaticFlushIntervalEvent() {
267272
Task { [weak self] in
268273
try await self?.flushLogs()
269274
}

‎AmplifyClients/AmplifyCloudWatchLoggingClient/Sources/Configuration/LogLevel+Codable.swift‎

Lines changed: 0 additions & 59 deletions
This file was deleted.

‎AmplifyClients/AmplifyCloudWatchLoggingClient/Sources/Configuration/LoggingConstraints.swift‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import AmplifyFoundation
99
import Foundation
1010

1111
@_spi(AmplifyExperimental)
12-
public struct LoggingConstraints: Codable, Sendable {
12+
public struct LoggingConstraints: Sendable {
1313
public init(
1414
defaultLogLevel: LogLevel = .error,
1515
namespaceLogLevel: [String: LogLevel] = [:],
@@ -26,10 +26,10 @@ public struct LoggingConstraints: Codable, Sendable {
2626
}
2727

2828
@_spi(AmplifyExperimental)
29-
public struct UserLogLevel: Codable, Sendable {
29+
public struct UserLogLevel: Sendable {
3030
public init(
31-
defaultLogLevel: LogLevel,
32-
namespaceLogLevel: [String: LogLevel]
31+
defaultLogLevel: LogLevel = .error,
32+
namespaceLogLevel: [String: LogLevel] = [:]
3333
) {
3434
self.defaultLogLevel = defaultLogLevel
3535
self.namespaceLogLevel = namespaceLogLevel

‎AmplifyClients/AmplifyCloudWatchLoggingClient/Sources/Consumer/CloudWatchLoggingConsumer.swift‎

Lines changed: 36 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ class CloudWatchLoggingConsumer: @unchecked Sendable {
1818
private var logStreamName: String?
1919
private var ensureLogStreamExistsComplete: Bool = false
2020
private let logger = AmplifyFoundation.AmplifyLogging.logger(for: CloudWatchLoggingConsumer.self)
21-
private let encoderLock = NSLock()
2221
private let encoder: JSONEncoder = {
2322
let encoder = JSONEncoder()
2423
encoder.dateEncodingStrategy = .millisecondsSince1970
@@ -34,12 +33,6 @@ class CloudWatchLoggingConsumer: @unchecked Sendable {
3433
self.formatter = CloudWatchLoggingStreamNameFormatter(userIdentifier: userIdentifier)
3534
self.logGroupName = logGroupName
3635
}
37-
38-
private func safeEncode(_ value: some Encodable) throws -> Data {
39-
encoderLock.lock()
40-
defer { encoderLock.unlock() }
41-
return try encoder.encode(value)
42-
}
4336
}
4437

4538
extension CloudWatchLoggingConsumer: LogBatchConsumer {
@@ -57,49 +50,57 @@ extension CloudWatchLoggingConsumer: LogBatchConsumer {
5750
return
5851
}
5952

60-
let entriesCopy = entries
53+
try await sendEntries(entries)
54+
try batch.complete()
55+
}
6156

57+
private func sendEntries(_ entries: [LogEntry]) async throws {
6258
var batchByteSize: Int
6359
do {
64-
batchByteSize = try safeEncode(entriesCopy).count
60+
batchByteSize = try encoder.encode(entries).count
6561
} catch {
6662
logger.error("Failed to encode log entries: \(error)")
67-
try batch.complete()
6863
return
6964
}
7065

71-
if entriesCopy.count > CloudWatchConstants.maxLogEvents {
72-
let smallerEntries = entriesCopy.chunked(into: CloudWatchConstants.maxLogEvents)
73-
for entries in smallerEntries {
74-
do {
75-
let entrySize = try safeEncode(entries).count
76-
if entrySize > CloudWatchConstants.maxBatchByteSize {
77-
let chunks = try chunk(entries, into: CloudWatchConstants.maxBatchByteSize)
78-
for chunk in chunks {
79-
try await sendLogEvents(chunk)
80-
}
81-
} else {
82-
try await sendLogEvents(entries)
83-
}
84-
} catch {
85-
logger.error("Error processing log batch: \(error)")
86-
continue
87-
}
88-
}
66+
if entries.count > CloudWatchConstants.maxLogEvents {
67+
try await sendEntriesExceedingMaxCount(entries)
8968
} else if batchByteSize > CloudWatchConstants.maxBatchByteSize {
69+
try await sendEntriesExceedingMaxSize(entries)
70+
} else {
71+
try await sendLogEvents(entries)
72+
}
73+
}
74+
75+
private func sendEntriesExceedingMaxCount(_ entries: [LogEntry]) async throws {
76+
let smallerEntries = entries.chunked(into: CloudWatchConstants.maxLogEvents)
77+
for entries in smallerEntries {
9078
do {
91-
let smallerEntries = try chunk(entriesCopy, into: CloudWatchConstants.maxBatchByteSize)
92-
for entries in smallerEntries {
79+
let entrySize = try encoder.encode(entries).count
80+
if entrySize > CloudWatchConstants.maxBatchByteSize {
81+
let chunks = try chunk(entries, into: CloudWatchConstants.maxBatchByteSize)
82+
for chunk in chunks {
83+
try await sendLogEvents(chunk)
84+
}
85+
} else {
9386
try await sendLogEvents(entries)
9487
}
9588
} catch {
96-
logger.error("Error chunking log entries: \(error)")
89+
logger.error("Error processing log batch: \(error)")
90+
continue
9791
}
98-
} else {
99-
try await sendLogEvents(entriesCopy)
10092
}
93+
}
10194

102-
try batch.complete()
95+
private func sendEntriesExceedingMaxSize(_ entries: [LogEntry]) async throws {
96+
do {
97+
let smallerEntries = try chunk(entries, into: CloudWatchConstants.maxBatchByteSize)
98+
for chunk in smallerEntries {
99+
try await sendLogEvents(chunk)
100+
}
101+
} catch {
102+
logger.error("Error chunking log entries: \(error)")
103+
}
103104
}
104105

105106
private func ensureLogStreamExists() async {
@@ -108,12 +109,7 @@ extension CloudWatchLoggingConsumer: LogBatchConsumer {
108109
}
109110

110111
if logStreamName == nil {
111-
let streamName = await formatter.formattedStreamName()
112-
if !streamName.isEmpty {
113-
self.logStreamName = streamName
114-
} else {
115-
self.logStreamName = "default.\(UUID().uuidString)"
116-
}
112+
self.logStreamName = await formatter.formattedStreamName()
117113
}
118114

119115
guard let logStreamName, !logStreamName.isEmpty else {

AmplifyPlugins/Internal/Sources/InternalCloudWatchLogging/Consumer/CloudWatchLoggingStreamNameFormatter.swift renamed to AmplifyClients/AmplifyCloudWatchLoggingClient/Sources/Consumer/CloudWatchLoggingStreamNameFormatter.swift

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,10 @@ import AppKit
2020
#endif
2121

2222
/// Responsible for creating pre-formatted CloudWatch stream names.
23-
package struct CloudWatchLoggingStreamNameFormatter {
23+
struct CloudWatchLoggingStreamNameFormatter {
2424

25-
package let userIdentifier: String?
26-
package var deviceIdentifier: String? {
25+
let userIdentifier: String?
26+
var deviceIdentifier: String? {
2727
get async {
2828
#if canImport(WatchKit)
2929
await WKInterfaceDevice.current().identifierForVendor?.uuidString
@@ -37,18 +37,15 @@ package struct CloudWatchLoggingStreamNameFormatter {
3737
}
3838
}
3939

40-
package init(userIdentifier: String? = nil) {
40+
init(userIdentifier: String? = nil) {
4141
self.userIdentifier = userIdentifier
4242
}
4343

44-
package func formattedStreamName() async -> String {
44+
func formattedStreamName() async -> String {
4545
return await "\(deviceIdentifier ?? "").\(userIdentifier ?? "guest")"
4646
}
4747

48-
// Add the missing deviceIdentifierFromBundle static method
4948
private static func deviceIdentifierFromBundle() -> String? {
50-
// Use bundle identifier as a fallback device identifier
51-
// This provides a consistent identifier per app installation
5249
return Bundle.main.bundleIdentifier
5350
}
5451
}

‎AmplifyClients/AmplifyCloudWatchLoggingClient/Sources/Persistence/LogEntry.swift‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,14 @@ struct LogEntry: Codable, Hashable, Sendable, LogEntryRepresentable {
3434

3535
/// - Returns: String representation of log level
3636
var logLevelName: String {
37-
return logLevel.name
37+
switch logLevel {
38+
case .error: return "ERROR"
39+
case .warn: return "WARN"
40+
case .info: return "INFO"
41+
case .debug: return "DEBUG"
42+
case .verbose: return "VERBOSE"
43+
case .none: return "NONE"
44+
}
3845
}
3946

4047
var millisecondsSince1970: Int {

‎AmplifyClients/AmplifyCloudWatchLoggingClient/Sources/Persistence/LogEntryCodec.swift‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,14 +14,13 @@ struct LogEntryCodec {
1414

1515
enum DecodingError: Error {
1616
case stringNotUtf8(String)
17-
case invalidScheme(log: URL)
17+
case invalidFileScheme(log: URL)
1818
case invalidEncoding(log: URL)
1919
}
2020

2121
func encode(entry: LogEntry) throws -> Data {
2222
let encoder = JSONEncoder()
2323
encoder.dateEncodingStrategy = .millisecondsSince1970
24-
encoder.outputFormatting = .sortedKeys
2524
var data = try encoder.encode(entry)
2625
data.append(Self.lineDelimiter)
2726
return data
@@ -41,7 +40,7 @@ struct LogEntryCodec {
4140

4241
func decode(from fileURL: URL) throws -> [LogEntry] {
4342
guard fileURL.isFileURL else {
44-
throw DecodingError.invalidScheme(log: fileURL)
43+
throw DecodingError.invalidFileScheme(log: fileURL)
4544
}
4645
let data = try Data(contentsOf: fileURL)
4746
guard let contentAsString = String(data: data, encoding: .utf8) else {

‎AmplifyClients/AmplifyCloudWatchLoggingClient/Sources/Support/CloudWatchLoggingFilter.swift‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,8 @@ final class CloudWatchLoggingFilter: CloudWatchLoggingFilterBehavior, @unchecked
6767
private extension NSLock {
6868
@discardableResult
6969
func execute<T>(_ block: () -> T) -> T {
70-
lock(); defer { unlock() }
70+
lock()
71+
defer { unlock() }
7172
return block()
7273
}
7374
}

AmplifyPlugins/Internal/Sources/InternalCloudWatchLogging/Support/CloudWatchLoggingMonitor.swift renamed to AmplifyClients/AmplifyCloudWatchLoggingClient/Sources/Support/CloudWatchLoggingMonitor.swift

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
import Foundation
99

1010
/// Provides a monitor to automatically flush the log at a specific TimeInterval.
11-
package class CloudWatchLoggingMonitor {
11+
class CloudWatchLoggingMonitor {
1212
private let automaticFlushLogsInterval: TimeInterval
1313
private var automaticFlushLogsTimer: DispatchSourceTimer? {
1414
willSet {
@@ -17,13 +17,19 @@ package class CloudWatchLoggingMonitor {
1717
}
1818

1919
private weak var eventDelegate: CloudWatchLoggingMonitorDelegate?
20+
private let queue: DispatchQueue
2021

21-
package init(flushIntervalInSeconds: TimeInterval, eventDelegate: CloudWatchLoggingMonitorDelegate?) {
22+
init(
23+
flushIntervalInSeconds: TimeInterval,
24+
eventDelegate: CloudWatchLoggingMonitorDelegate?,
25+
queue: DispatchQueue = DispatchQueue.global(qos: .background)
26+
) {
2227
self.automaticFlushLogsInterval = flushIntervalInSeconds
2328
self.eventDelegate = eventDelegate
29+
self.queue = queue
2430
}
2531

26-
package func setAutomaticFlushIntervals() {
32+
func setAutomaticFlushIntervals() {
2733
guard automaticFlushLogsInterval != .zero else {
2834
automaticFlushLogsTimer = nil
2935
return
@@ -40,13 +46,13 @@ package class CloudWatchLoggingMonitor {
4046
}
4147

4248
func createRepeatingTimer(timeInterval: TimeInterval, eventHandler: @escaping () -> Void) -> DispatchSourceTimer {
43-
let timer = DispatchSource.makeTimerSource(queue: DispatchQueue.global(qos: .background))
49+
let timer = DispatchSource.makeTimerSource(queue: queue)
4450
timer.schedule(deadline: .now() + timeInterval, repeating: timeInterval)
4551
timer.setEventHandler(handler: eventHandler)
4652
return timer
4753
}
4854
}
4955

50-
package protocol CloudWatchLoggingMonitorDelegate: AnyObject {
56+
protocol CloudWatchLoggingMonitorDelegate: AnyObject {
5157
func handleAutomaticFlushIntervalEvent()
5258
}

0 commit comments

Comments
 (0)