Skip to content
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

[Enhancement]Improve Error logging #645

Merged
merged 4 commits into from
Jan 31, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions DemoApp/Sources/Components/AppState.swift
Original file line number Diff line number Diff line change
Expand Up @@ -130,15 +130,15 @@ final class AppState: ObservableObject {
try await streamVideo?.deleteDevice(id: voipPushToken)
log.debug("✅ Removed VOIP push notification device for token: \(voipPushToken)")
} catch {
log.error("Removing VOIP push notification device for token: \(voipPushToken)")
log.error("Removing VOIP push notification device for token: \(voipPushToken)", error: error)
}
}
if let pushToken = unsecureRepository.currentPushToken() {
do {
try await streamVideo?.deleteDevice(id: pushToken)
log.debug("✅ Removed push notification device for token: \(pushToken)")
} catch {
log.error("Removing push notification device for token: \(pushToken)")
log.error("Removing push notification device for token: \(pushToken)", error: error)
}
}
await streamVideo?.disconnect()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,13 @@ final class OSLogDestination: BaseLogDestination {

switch logDetails.level {
case .debug:
logger.debug("\(formattedMessage, privacy: .public)")
logger.debug("\(formattedMessage)")
case .info:
logger.notice("\(formattedMessage, privacy: .public)")
logger.notice("\(formattedMessage)")
case .warning:
logger.warning("\(formattedMessage, privacy: .public)")
logger.warning("\(formattedMessage)")
case .error:
logger.critical("\(formattedMessage, privacy: .public)")
logger.critical("\(formattedMessage)")
}
}
}
12 changes: 0 additions & 12 deletions DemoApp/Sources/Extensions/DemoApp+Sentry.swift
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,6 @@ import Foundation
import Sentry
import StreamVideo

private struct DemoLogFormatter: LogFormatter {
func format(logDetails: LogDetails, message: String) -> String {
guard logDetails.level == .error, let error = logDetails.error else {
return message
}
return "\(message) [Error details: \(error)]"
}
}

func configureSentry() {
if AppEnvironment.configuration.isRelease {
// We're tracking Crash Reports / Issues from the Demo App to keep improving the SDK
Expand All @@ -40,9 +31,6 @@ func configureSentry() {
MemoryLogDestination.self,
OSLogDestination.self
]
LogConfig.formatters = [
DemoLogFormatter()
]
}
}

Expand Down
20 changes: 2 additions & 18 deletions Sources/StreamVideo/Errors/Errors.swift
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,10 @@ import Foundation

extension APIError: Error {}

extension Stream_Video_Sfu_Models_Error: Error, CustomStringConvertible {
var description: String {
if code == .unspecified {
return "-"
} else {
return """
code: \(code)
message: \(message)
shouldRetry: \(shouldRetry)
"""
}
}
}
extension Stream_Video_Sfu_Models_Error: Error, ReflectiveStringConvertible {}

/// A Client error.
public class ClientError: Error, CustomStringConvertible {
public class ClientError: Error, ReflectiveStringConvertible {
public struct Location: Equatable {
public let file: String
public let line: Int
Expand All @@ -47,10 +35,6 @@ public class ClientError: Error, CustomStringConvertible {
/// Retrieve the localized description for this error.
public var localizedDescription: String { message ?? errorDescription ?? "" }

public private(set) lazy var description = "Error \(type(of: self)) in \(location?.file ?? ""):\(location?.line ?? 0)"
+ (localizedDescription.isEmpty ? "" : " -> ")
+ localizedDescription

/// A client error based on an external general error.
/// - Parameters:
/// - error: an external error.
Expand Down
4 changes: 2 additions & 2 deletions Sources/StreamVideo/OpenApi/generated/Models/APIError.swift
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@

import Foundation

public final class APIError: @unchecked Sendable, Codable, JSONEncodable, Hashable {
public final class APIError: @unchecked Sendable, Codable, JSONEncodable, Hashable, ReflectiveStringConvertible {

public var code: Int
public var details: [Int]
public var duration: String
Expand Down
15 changes: 12 additions & 3 deletions Sources/StreamVideo/StreamVideo.swift
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,10 @@ public class StreamVideo: ObservableObject, @unchecked Sendable {
/// - Parameter id: the id of the device (token) for push notifications.
@discardableResult
public func setDevice(id: String) async throws -> ModelResponse {
try await setDevice(
guard !id.isEmpty else {
throw ClientError("Device id must not be empty when trying to set device.")
}
return try await setDevice(
id: id,
pushProvider: pushNotificationsConfig.pushProviderInfo.pushProvider,
name: pushNotificationsConfig.pushProviderInfo.name,
Expand All @@ -264,7 +267,10 @@ public class StreamVideo: ObservableObject, @unchecked Sendable {
/// - Parameter id: the id of the device (token) for VoIP push notifications.
@discardableResult
public func setVoipDevice(id: String) async throws -> ModelResponse {
try await setDevice(
guard !id.isEmpty else {
throw ClientError("Device id must not be empty when trying to set VoIP device.")
}
return try await setDevice(
id: id,
pushProvider: pushNotificationsConfig.voipPushProviderInfo.pushProvider,
name: pushNotificationsConfig.voipPushProviderInfo.name,
Expand All @@ -276,7 +282,10 @@ public class StreamVideo: ObservableObject, @unchecked Sendable {
/// - Parameter id: the id of the device that will be deleted.
@discardableResult
public func deleteDevice(id: String) async throws -> ModelResponse {
try await coordinatorClient.deleteDevice(id: id)
guard !id.isEmpty else {
throw ClientError("Device id must not be empty when trying to delete device.")
}
return try await coordinatorClient.deleteDevice(id: id)
}

/// Lists the devices registered for the user.
Expand Down
9 changes: 9 additions & 0 deletions Sources/StreamVideo/Utils/Logger/Logger.swift
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,15 @@ public class Logger {
fileName: StaticString = #fileID,
lineNumber: UInt = #line
) {
// If the error isn't conforming to ``ReflectiveStringConvertible`` we
// wrap it in a ``ClientError`` to provide consistent logging information.
let error = {
guard let error, (error as? ReflectiveStringConvertible) == nil else {
return error
}
return ClientError(with: error, fileName, lineNumber)
}()

log(
.error,
functionName: functionName,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,104 @@

import Foundation

/// An enumeration representing rules for skipping properties during reflective
/// string conversion.
///
/// These rules can be used to exclude properties based on specific conditions,
/// such as being empty, nil, or matching a custom rule.
public enum ReflectiveStringConvertibleSkipRule: Hashable {
/// Skip properties that are empty.
case empty

/// Skip properties that are nil.
case nilValues

/// Skip properties based on a custom rule.
///
/// - Parameters:
/// - identifier: A unique identifier for the custom rule.
/// - rule: A closure that takes a `Mirror.Child` and returns a Boolean
/// indicating whether the property should be skipped.
case custom(identifier: String, rule: (Mirror.Child) -> Bool)

/// Hashes the essential components of this value by feeding them into the
/// given hasher.
///
/// - Parameter hasher: The hasher to use when combining the components of
/// this instance.
public func hash(into hasher: inout Hasher) {
switch self {
case .empty:
hasher.combine(".empty")
case .nilValues:
hasher.combine(".nilValues")
case let .custom(identifier, _):
hasher.combine(".custom_")
hasher.combine(identifier)
}
}

/// Determines whether a given property should be skipped based on the rule.
///
/// - Parameter child: A `Mirror.Child` representing the property to check.
/// - Returns: A Boolean indicating whether the property should be skipped.
public func shouldBeSkipped(_ child: Mirror.Child) -> Bool {
switch self {
case .empty:
if (child.value as? String)?.isEmpty == true {
return true
} else if (child.value as? (any Collection))?.isEmpty == true {
return true
} else {
return false
}

case .nilValues:
return "\(child.value)" == "nil"

case let .custom(_, rule):
return rule(child)
}
}

/// Compares two `ReflectiveStringConvertibleSkipRule` values for equality.
///
/// - Parameters:
/// - lhs: A `ReflectiveStringConvertibleSkipRule` value.
/// - rhs: Another `ReflectiveStringConvertibleSkipRule` value.
/// - Returns: A Boolean indicating whether the two values are equal.
public static func == (
lhs: ReflectiveStringConvertibleSkipRule,
rhs: ReflectiveStringConvertibleSkipRule
) -> Bool {
switch (lhs, rhs) {
case (.empty, .empty):
return true
case (.nilValues, .nilValues):
return true
case let (.custom(lhsIdentifier, _), .custom(rhsIdentifier, _)) where lhsIdentifier == rhsIdentifier:
return true
default:
return false
}
}
}

/// An extension for collections of `ReflectiveStringConvertibleSkipRule` values.
extension Collection where Element == ReflectiveStringConvertibleSkipRule {
/// Determines whether a given property should be skipped based on the rules
/// in the collection.
///
/// - Parameter element: A `Mirror.Child` representing the property to check.
/// - Returns: A Boolean indicating whether the property should be skipped.
func shouldBeSkipped(_ element: Mirror.Child) -> Bool {
reduce(false) { partialResult, rule in
guard !partialResult else { return partialResult }
return rule.shouldBeSkipped(element)
}
}
}

/// A protocol that extends `CustomStringConvertible` to provide reflective string conversion capabilities.
///
/// Types conforming to this protocol can customize their string representation by excluding specific properties
Expand All @@ -17,9 +115,15 @@ public protocol ReflectiveStringConvertible: CustomStringConvertible {

/// A set of property names to be transformed during the string representation.
var propertyTransformers: [String: (Any) -> String] { get }

var skipRuleSet: Set<ReflectiveStringConvertibleSkipRule> { get }
}

public extension ReflectiveStringConvertible {
var skipRuleSet: Set<ReflectiveStringConvertibleSkipRule> {
[.empty, .nilValues]
}

/// The default separator used to join different parts of the string representation.
///
/// By default, this is set to a newline character ("\n").
Expand Down Expand Up @@ -71,16 +175,18 @@ public extension ReflectiveStringConvertible {
let excludedProperties = self.excludedProperties
mirror
.children
.filter { !skipRuleSet.shouldBeSkipped($0) }
.compactMap {
if let label = $0.label {
if let label = $0.label, !excludedProperties.contains(label) {
let value = propertyTransformers[label]?($0.value) ?? $0.value
return (label: label, value: value)
} else {
return nil
}
}
.filter { !excludedProperties.contains($0.label) }
.forEach { output.append(" - \($0.label): \($0.value)") }
.forEach { (child: (label: String, value: Any)) -> Void in
output.append(" - \(child.label): \(child.value)")
}

return output.joined(separator: separator)
}
Expand Down
Loading