Logging functionality for CarPlay #3653
Conversation
jill-cardamon
added
op-ex
| @@ -77,6 +78,7 @@ public class CarPlayManager: NSObject { | |||
|
|
|||
| private weak var navigationService: NavigationService? | |||
| private var idleTimerCancellable: IdleTimerManager.Cancellable? | |||
| private let logger: OSLog = .init(subsystem: "com.mapbox.navigation", category: "CarPlayManager") | |||
There was a problem hiding this comment.
Should we add a factory for loggers? So that we don't duplicate subsystem value everywhere.
There was a problem hiding this comment.
Or we can go with a simpler way: creating an internal constant for the log subsystem.
There was a problem hiding this comment.
We could use something like OSLogManager but would probably want to consider using this manager for all of the SDK.
| @@ -23,7 +26,7 @@ extension NavigationMapView { | |||
| do { | |||
| try mapView.mapboxMap.style.addSource(streetsSource, id: sourceIdentifier) | |||
| } catch { | |||
| NSLog("Failed to add \(sourceIdentifier) with error: \(error.localizedDescription).") | |||
| os_log("Failed to add %s with error: %s.", log: logger, type: .error, sourceIdentifier as CVarArg, error.localizedDescription) | |||
There was a problem hiding this comment.
The %s will be redacted out in the logs. Should it be %{public}s?
There was a problem hiding this comment.
Ah yes. I will try this. Thank you
| @@ -23,7 +26,7 @@ extension NavigationMapView { | |||
| do { | |||
| try mapView.mapboxMap.style.addSource(streetsSource, id: sourceIdentifier) | |||
| } catch { | |||
| NSLog("Failed to add \(sourceIdentifier) with error: \(error.localizedDescription).") | |||
| os_log("Failed to add %s with error: %s.", log: logger, type: .error, sourceIdentifier as CVarArg, error.localizedDescription) | |||
There was a problem hiding this comment.
Why do we need casting to CVarArg here?
There was a problem hiding this comment.
Cast was needed since os_log uses StaticString
| @@ -1,6 +1,9 @@ | |||
| import MapboxMaps | |||
| import MapboxDirections | |||
| import MapboxCoreNavigation | |||
| import os.log | |||
|
|
|||
| private let logger: OSLog = .init(subsystem: "com.mapbox.navigation", category: "RoadNameLabeling") | |||
There was a problem hiding this comment.
Should we use private global constants in other classes too?
There was a problem hiding this comment.
We needed one here since you can't create stored properties in extensions. For consistency, we should either move constants to be global or use a manager for these OSLog constants
| } | ||
|
|
||
| @available(iOS 15.0, *) | ||
| func getLogEntries() throws -> [OSLogEntryLog] { |
There was a problem hiding this comment.
Is it for debugging? If so, don't forget to remove this
There was a problem hiding this comment.
This is for retrieving log entries if you want to see them printed to a file instead of looking at the log via the Console. This functionality is only available via iOS 15.
There was a problem hiding this comment.
I think there are several build errors because of OSLogEntryLog. For example getting: error: cannot find type 'OSLogEntryLog' in scope.
| @@ -333,6 +354,7 @@ extension CarPlayManager: CPApplicationDelegate { | |||
| interfaceController.setRootTemplate(mapTemplate, animated: false) | |||
|
|
|||
| eventsManager.sendCarPlayConnectEvent() | |||
| os_log("CarInterfaceController did connect to window.", log: logger, type: .info) | |||
There was a problem hiding this comment.
I think that CPApplicationDelegate delegate methods are deprecated. We'll need similar logging for CPTemplateApplicationSceneDelegate as well. I think that it'd be also beneficial to log information regarding CPInterfaceController (e.g. its address, main template etc).
| @@ -2,6 +2,7 @@ import CarPlay | |||
| import MapboxCoreNavigation | |||
| import MapboxDirections | |||
| import MapboxMaps | |||
| import os.log | |||
There was a problem hiding this comment.
We'll need to add logging for CPTemplate lifecycle callbacks in CarPlayManagerDelegate:
carPlayManager(_:templateWillAppear:animated:)carPlayManager(_:templateDidAppear:animated:)carPlayManager(_:templateWillDisappear:animated:)carPlayManager(_:templateDidDisappear:animated:)
| @@ -2,6 +2,7 @@ import Foundation | |||
| @_spi(Restricted) import MapboxMaps | |||
| import MapboxCoreNavigation | |||
| import MapboxDirections | |||
| import os.log | |||
There was a problem hiding this comment.
It'd be useful to have logging information when CarPlayActivity value changes.
| @@ -561,6 +582,7 @@ extension CarPlayManager { | |||
|
|
|||
| switch result { | |||
| case let .failure(error): | |||
| os_log("Failed to calculate routes.", log: logger, type: .debug) | |||
There was a problem hiding this comment.
This looks like an error log type.
| @@ -93,6 +95,7 @@ public class CarPlayManager: NSObject { | |||
| locationProvider.stopUpdatingHeading() | |||
| if let passiveLocationProvider = locationProvider as? PassiveLocationProvider { | |||
| passiveLocationProvider.locationManager.pauseTripSession() | |||
| os_log("Trip session paused", log: logger, type: .debug) | |||
There was a problem hiding this comment.
It'd be good to go through all methods in CarPlayManager and see what CarPlay related methods it uses. I think that we should log all callbacks we receive from it.
|
We should add logging for |
This PR adds logging functionality to CarPlay to make it easier for us to debug locally.
Still needs testing on a real CarPlay head unit.
Additional ideas: