-
Notifications
You must be signed in to change notification settings - Fork 265
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
[GH-321] Better logging #322
Conversation
*/ | ||
class DefaultLogHandler: AirshipLogHandler { | ||
@available(macOS 11.0, iOS 14.0, watchOS 7.0, tvOS 14.0, *) | ||
static var logger: Logger = Logger(subsystem: Bundle.main.bundleIdentifier ?? "", category: "Airship") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Static here. I tried using a lazy computed property but it still caused an issue on iOS 13 and older. Static properties seem fine (they are also lazy).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another trick from my "log wrapper" type.
private var _logger: Any?
@available(iOS 14.0, *)
private var logger: Logger {
// swiftlint:disable:next force_cast
return _logger as! Logger
}
private init(subsystem: String, category: String) {
if #available(iOS 14.0, *) {
_logger = Logger(subsystem: subsystem, category: category)
} else {
// This implementation will do nothing for iOS < 14
// Implementation of log funcs might make use of Legacy OSLog API but then that presents the
// StaticString issue in the API call site
}
}
I don't like it and I will remove from my code the moment I can drop iOS 13 support.... but it gets the job done. If you plan to keep it as static var, I'd at least make it private or private(set). It doesn't fix the immutability problem, but it prevents internal classes from trying to change it (even though they wouldn't). I just like protecting code that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it should be private let. Not sure which one i like better. You can at least define the subsystem and category on the fly, not that we will. I'll probably just keep it static for now until I have the need for that. Thanks!
case .trace: return OSLogType.debug | ||
case .debug: return OSLogType.debug | ||
case .info: return OSLogType.info | ||
case .warn: return OSLogType.info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might suggest changing the level for .warn
to default
.
"Use this level to capture information about things that might result in a failure."
info
won't necessarily get saved long term if an issue happens due to internal buffering strategy.
"Use this level to capture information that may be helpful, but not essential, for troubleshooting errors."
I know this approach matches this, but I also disagree with this choice. Warn
is higher severity than Notice
in SwiftLog terms: https://github.com/chrisaljoudi/swift-log-oslog/blob/master/Sources/LoggingOSLog/LoggingOSLog.swift#L83
I understand this is the default implementation which I will override in my usage, but just something to consider. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Merging into our private repo, will be available next minor release. |
What do these changes do?
This PR does a few things:
How did you verify these changes?
Ran sample app, verified logs are coming through still.