Telemetry module #282
Telemetry module #282
Conversation
|
I've included all the changes I wanted for this iteration, CI is approving. @tobrun or @ivovandongen please review. |
| private DisplayMetrics displayMetrics = null; | ||
| private Intent batteryStatus = null; | ||
| private TelemetryClient client = null; | ||
| private Vector<Hashtable<String, Object>> events = new Vector<>(); |
There was a problem hiding this comment.
Is their a reason why Vector > List for this use-case?
There was a problem hiding this comment.
I believe the rationale behind using Vector here is that all operations are synchronized as we expect events coming from different threads. I'd explore other approaches in a separate ticket.
| * | ||
| * @return MapboxTelemetry | ||
| */ | ||
| public static MapboxTelemetry getInstance() { |
There was a problem hiding this comment.
synchronised keyword for thread safety or annotate the method to be called only from main thread?
There was a problem hiding this comment.
Yes. This got lost somehow, added now.
| * @param accessToken The accessToken associated with the application | ||
| * @param locationEngine Initialize telemetry with a custom location engine | ||
| */ | ||
| public void initialize(@NonNull Context context, @NonNull String accessToken, |
There was a problem hiding this comment.
Some thoughts: I'm not a big supporter of the Singleton paradigm with a separate initialization method. I think this can be overcome by using a separate parameterized getInstance method and handling those parameters as static (see MapboxMap.java in GL SDK) or by extracting some logic and wrapping it around a factory or other design pattern.
There was a problem hiding this comment.
Agreed. Let's ticket that separately.
| return; | ||
| } | ||
|
|
||
| Timber.v("Initializing telemetry."); |
There was a problem hiding this comment.
Right. Notice all this log calls are .v as in verbose. These can easily be filtered out by the right Timber Tree. Either way, we can remove these after the SDK happens and we're sure the refactor is working as expected.
| context.getPackageName(), PackageManager.GET_SERVICES); | ||
| if (packageInfo.services != null) { | ||
| for (ServiceInfo service : packageInfo.services) { | ||
| if (TextUtils.equals("com.mapbox.services.android.telemetry.service.TelemetryService", service.name)) { |
There was a problem hiding this comment.
Yes, extracted as TelemetryConstants.TELEMETRY_SERVICE_NAME.
| @Override | ||
| public void onCreate() { | ||
| // Enable location listening for lifecycle of app | ||
| Timber.v("Create event."); |
| */ | ||
| @Override | ||
| public int onStartCommand(Intent intent, int flags, int startId) { | ||
| Timber.v("Start command event."); |
| */ | ||
| @Override | ||
| public void onTaskRemoved(Intent rootIntent) { | ||
| Timber.v("Task removed event."); |
| */ | ||
| @Override | ||
| public void onDestroy() { | ||
| Timber.v("Destroy event."); |
|
|
||
| private void shutdownTelemetryService() { | ||
| try { | ||
| Timber.v("Unregistering location receiver."); |
|
@tobrun Thanks much for the review, I've addressed now the most immediate issues. Care to give it another pass? |
Extracted from the main SDK and refactored for a more loosely coupled architecture. Please note this is still a WIP.
Next steps
/cc: @mapbox/android