Add Metrics #27

pb82 · 2018-02-07T15:13:12Z

Motivation

Initial version of the android metrics sdk.

Description

Example Usage:

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        ...
        MetricsService metrics = this.core.getInstance(MetricsService.class);
        metrics.init(getApplicationContext());
        ...
    }

Progress

Initial POC
Reponse handling and logging
Tests

pb82 · 2018-02-07T15:16:44Z

metrics/src/main/java/mobile/aerogear/org/metrics/MetricsService.java

+            HttpRequest request = httpService.newRequest();
+            request.post(config.getUri(), getMetricsData(context));
+            request.execute();
+            // TODO: response callback and response status logging


@secondsun I tried

final HttpResponse response = request.execute(); response.onComplete(new Runnable() { @Override public void run() { Log.d(LOG_TAG, "status: " + response.getStatus()); } });

but that results in a null pointer exception when it's trying to get the status code from the response. What's the recommended way to use the http module in such a case? It looks like it's already executing the http requests on the network thread, so no need to use AsyncTask?

wei-lee · 2018-02-07T16:04:43Z

metrics/src/main/java/mobile/aerogear/org/metrics/MetricsService.java

+
+public class MetricsService implements ServiceModule {
+    public final static String LOG_TAG = "AEROGEAR/METRICS";
+    private final static String STORAGE_NAME = "aerogear-metrics";


I would normally suggest using the full package name to avoid potential naming conflicts.

danielpassos · 2018-02-07T16:50:37Z

build.gradle

@@ -33,6 +33,14 @@ subprojects {
    }
 }

+// We want to expose the SDK version and name to the metrics subproject
+project(':metrics') {


I think it should be the core responsibility

danielpassos · 2018-02-07T16:51:28Z

metrics/build.gradle

+        testInstrumentationRunner "android.support.test.runner.AndroidJUnitRunner"
+
+        // Required for metrics, exposed by the parent project
+        versionName project.ext.versionName


I think it should be the core responsibility

Using Core for versioning on IOS

danielpassos · 2018-02-07T16:51:39Z

metrics/build.gradle

+
+dependencies {
+    implementation project(path: ':core')
+    implementation fileTree(dir: 'libs', include: ['*.jar'])


It should be removed

danielpassos · 2018-02-07T16:52:18Z

metrics/build.gradle

+    implementation 'com.android.support:appcompat-v7:26.1.0'
+    testImplementation 'junit:junit:4.12'
+    androidTestImplementation 'com.android.support.test:runner:1.0.1'
+    androidTestImplementation 'com.android.support.test.espresso:espresso-core:3.0.1'


Version is coming from the BOM and should be removed from here

danielpassos · 2018-02-07T16:53:43Z

metrics/src/main/AndroidManifest.xml

@@ -0,0 +1,2 @@
+<manifest xmlns:android="http://schemas.android.com/apk/res/android"
+    package="mobile.aerogear.org.metrics" />


We are using a different kind of package (correctly or not) in the other modules org.aerogear.mobile.[module-name]

danielpassos · 2018-02-07T16:56:48Z

metrics/src/main/java/mobile/aerogear/org/metrics/MetricsService.java

+
+    @Override
+    public void configure(MobileCore core, ServiceConfiguration serviceConfiguration) {
+        config = new MetricsConfig(serviceConfiguration);


I think we can use ServiceConfiguration instead of creating a new one

danielpassos · 2018-02-07T16:57:20Z

metrics/src/main/java/mobile/aerogear/org/metrics/MetricsService.java

+
+    @Override
+    public void destroy() {
+        // Not used


I think the comment is not necessarily, and it can be a empty method

danielpassos · 2018-02-07T16:58:23Z

metrics/src/main/java/mobile/aerogear/org/metrics/MetricsService.java

+        }
+    }
+
+    private byte[] getMetricsData(final Context context) throws JSONException {


Only a personal opinion it can be createMetricsData or only metricsData, but again it's only a personal thought

danielpassos · 2018-02-07T16:59:54Z

metrics/src/main/java/mobile/aerogear/org/metrics/MetricsService.java

+                .getPackageManager()
+                .getPackageInfo(context.getPackageName(), 0).versionName;
+        } catch (PackageManager.NameNotFoundException e) {
+            logger.error(LOG_TAG, e);


Not sure if's going to happen but I think we should raise an Exception instead of return null

danielpassos · 2018-02-07T17:00:23Z

metrics/src/main/java/mobile/aerogear/org/metrics/MetricsService.java

+
+public class MetricsService implements ServiceModule {
+    private final static String MODULE_NAME = "metrics";
+    private final static String LOG_TAG = "AEROGEAR/METRICS";


s/LOG_TAG/TAG

aliok · 2018-02-08T06:54:03Z

metrics/src/main/java/mobile/aerogear/org/metrics/MetricsService.java

+     * @param context Android app context
+     * @return String Client ID
+     */
+    private String getOrCreateClientId(final Context context) {


@pb82 @wtrocki @danielpassos
First thing I noticed is private methods/fields/etc.. I think we should better go with protected as we are developing an SDK for our users and not an app. They might want to override stuff.
Just a general comment to keep in mind to make things extendable.

Very nice catch!!!

aliok · 2018-02-08T06:58:06Z

metrics/src/main/java/mobile/aerogear/org/metrics/MetricsService.java

+        result.put("appVersion", getAppVersion(context));
+        result.put("sdkVersion", BuildConfig.VERSION_NAME);
+
+        return result.toString().getBytes();


Similar concern to make things extendable.
I would make this method protected and make it return JSONObject. If a user wants to override metrics sent, they should be able to just call super, get the JSONObject and then modify it.
toString or getBytes() conversion could very well happen in the caller of this method, if it is only called from one place.

aliok · 2018-02-08T09:07:29Z

metrics/src/main/java/mobile/aerogear/org/metrics/MetricsService.java

+     *
+     * @param context Android application context
+     */
+    public void init(final Context context) {


IMO this depends on how we call this init method. Perhaps we can still throw the exception and let the caller decide what to do with it.
Looked for callers, couldn't find any at the moment. How is this supposed to be called?

wtrocki · 2018-02-08T15:23:08Z

See also IOS SDK implementation: aerogear/aerogear-ios-sdk#13

pb82 · 2018-02-08T17:36:17Z

gradle-mvn-push.gradle

@@ -103,7 +103,7 @@ afterEvaluate { project ->
            source = variant.javaCompile.source
            ext.androidJar = project.files(android.getBootClasspath().join(File.pathSeparator))
            classpath = files(variant.javaCompile.classpath.files) + files(ext.androidJar)
-            exclude '**/BuildConfig.java'
+            exclude '**/BuildConfig'


For some reason those exclusions don't work with .java and gradle gives out warnings. That's why i've changed it, it seems to work as expected this way.

pb82 · 2018-02-08T17:37:44Z

metrics/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker

@@ -0,0 +1 @@
+mock-maker-inline


This allows Mockito to mock final classes and methods

danielpassos · 2018-02-09T00:10:39Z

metrics/src/main/java/org/aerogear/mobile/metrics/MetricsService.java

+import java.util.UUID;
+
+public class MetricsService implements ServiceModule {
+    public final static String STORAGE_NAME = "mobile.aerogear.org.metrics";


s/mobile.aerogear.org.metrics/org.aerogear.mobile.metrics

danielpassos · 2018-02-09T00:15:45Z

@pb82 Just need a very small fix and we are good to go. Awesome work here man.

aliok · 2018-02-09T08:12:03Z

I haven't actually tried running the code, but looks good to me 👍
I still have the feeling that we should use protected for almost every private thing in order to allow extension of things, but this is something that could be done anytime.

pb82 · 2018-02-09T08:18:58Z

@danielpassos Changed the storage name to the actual package name
@aliok Yes that can be done at any time. I also feel that we need to think about how we want people to extend the SDK. Right now they could override those methods in their own implementation but then, what do they do with it? There must be a documented way to register your own implementation in the SDK (core is responsible for creating the instances).

pb82 requested review from danielpassos, aliok, wtrocki and secondsun February 7, 2018 15:13

pb82 commented Feb 7, 2018

View reviewed changes

wei-lee reviewed Feb 7, 2018

View reviewed changes

pb82 force-pushed the AGDROID-719 branch 5 times, most recently from 774f8d3 to e0ffe89 Compare February 7, 2018 16:48

danielpassos changed the title ~~Agdroid 719~~ Add Metrics Feb 7, 2018

danielpassos suggested changes Feb 7, 2018

View reviewed changes

aliok reviewed Feb 8, 2018

View reviewed changes

pb82 force-pushed the AGDROID-719 branch from e0ffe89 to 3b6bcda Compare February 8, 2018 17:32

pb82 commented Feb 8, 2018

View reviewed changes

danielpassos suggested changes Feb 9, 2018

View reviewed changes

pb82 added 3 commits February 9, 2018 09:15

AGDROID-719 add metrics service

7805c8f

AGDROID-719 collect metrics data and send it to server

d4230fa

Refactor and add unit test

62c6250

pb82 force-pushed the AGDROID-719 branch from ae0b6f2 to 62c6250 Compare February 9, 2018 08:15

danielpassos approved these changes Feb 9, 2018

View reviewed changes

danielpassos merged commit 4f50915 into aerogear:master Feb 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Metrics #27

Add Metrics #27

pb82 commented Feb 7, 2018 •

edited

pb82 Feb 7, 2018

wei-lee Feb 7, 2018

danielpassos Feb 7, 2018

danielpassos Feb 7, 2018

wtrocki Feb 8, 2018

danielpassos Feb 7, 2018

danielpassos Feb 7, 2018

danielpassos Feb 7, 2018

danielpassos Feb 7, 2018

danielpassos Feb 7, 2018

danielpassos Feb 7, 2018

danielpassos Feb 7, 2018

danielpassos Feb 7, 2018

aliok Feb 8, 2018

danielpassos Feb 8, 2018

aliok Feb 8, 2018

aliok Feb 8, 2018

wtrocki commented Feb 8, 2018 •

edited

pb82 Feb 8, 2018 •

edited

pb82 Feb 8, 2018

danielpassos Feb 9, 2018

danielpassos commented Feb 9, 2018

aliok commented Feb 9, 2018

pb82 commented Feb 9, 2018

		@@ -0,0 +1,2 @@
		<manifest xmlns:android="http://schemas.android.com/apk/res/android"
		package="mobile.aerogear.org.metrics" />

		@@ -0,0 +1 @@
		mock-maker-inline

Add Metrics #27

Add Metrics #27

Conversation

pb82 commented Feb 7, 2018 • edited

Motivation

Description

Progress

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtrocki commented Feb 8, 2018 • edited

pb82 Feb 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielpassos commented Feb 9, 2018

aliok commented Feb 9, 2018

pb82 commented Feb 9, 2018

pb82 commented Feb 7, 2018 •

edited

wtrocki commented Feb 8, 2018 •

edited

pb82 Feb 8, 2018 •

edited