Metrics sdk #13

wtrocki · 2018-02-06T15:55:42Z

Motivation

Initial version of Metrics Swift SDK

Progress

Initial SDK setup
Client ID
mobile service configuration (URL)
Investigate SDK version in source code
General refactoring and styles
Convert to new mobile-config.json format

wei-lee · 2018-02-06T16:08:59Z

modules/metrics/metrics/SdkVersion.swift

+    }
+
+    fileprivate func initId() {
+        if let uuid = UIDevice.current.identifierForVendor?.uuidString {


not sure if identifierForVendor fits the purpose here.

According to this from apple:

The value of this property is the same for apps that come from the same vendor running on the same device.

I believe it will return the same value for the same developer. In another word, if I have 3 different apps installed on the same device, it will return the same value for all 3 apps. Not sure this is what you are looking for?

I checked couple options:

custom libraries on github

advertisingidentifier

identifierForVendor

Was thinking that identifierForVendor will be the most appropriate option, but in fact I will go with aproach to generate this random string per installation.

I see that metrics and analytics for most of the cases are tied to user like here:
https://developers.google.com/analytics/devguides/collection/ios/v3/user-id

yeah, I think generating the device id and save it locally is enough for this use case here.

wei-lee · 2018-02-06T16:12:02Z

example/AeroGearSdkExample/AppDelegate.swift

    }

    func applicationDidBecomeActive(_: UIApplication) {
+        metrics.foreground();


the method name is a bit confusing. In metrics it is called foreground, but you are calling it from applicationDidBecomeActive, while there is also a applicationWillEnterForeground method here?

This is temporary and will be replaced with proper metrics implementation.
Our metrics can hook into application lifecycle and other specific events.

Going to rename method and remove background method as it's not used.

pb82 · 2018-02-07T08:07:54Z

modules/metrics/metrics/SdkVersion.swift

+    }
+
+    fileprivate func setupUri() {
+        if let uri = core.getConfiguration("metrics")?.config?.uri {


According to https://github.com/aerogear/proposals/blob/master/sdks/Mobile-configuration.md we should use url instead of uri.

We using old format for Android and IOS. New format has uri field on the top level.
Once cmd line tool will be changed and released after sprint we will adjust formats.
I'm going to create issue to get that aligned, but it will be separate PR.

Actually going to fix that here: https://issues.jboss.org/browse/AGIOS-648

pb82 · 2018-02-07T08:09:43Z

modules/metrics/metrics/SdkVersion.swift

+
+    fileprivate func initId() {
+        let defaults = UserDefaults.standard
+        if let versionId = defaults.string(forKey: "metrics-sdk-installation-id")  {


Maybe use a constant for metrics-sdk-installation-id ?

pb82 · 2018-02-07T08:10:42Z

modules/metrics/metrics/SdkVersion.swift

+            AgsCore.logger.error("clientId! \(clientId)")
+        } else {
+            let uuid = UUID().uuidString
+            clientId = uuid


clientId vs. installationId, i think we should have consistent naming (will also use installationId on Android).

+100 - Going to refactor names here as they are not consistent.

pb82 · 2018-02-07T08:11:36Z

modules/metrics/metrics/SdkVersion.swift

+            let requestDictionary = try? payload.adaptToDictionary()
+            core.getHttp().post(uri, body: requestDictionary, { (response, error) -> Void in
+                if let error = error {
+                    AgsCore.logger.error("An error has occurred when sending app metrics! \(error)")


Just my preference but i would remove the exclamation mark from the message.

pb82

Looks good, but added a few comments.

wtrocki · 2018-02-07T18:38:23Z

modules/metrics/AgsMetrics.swift

+    open func collectMetrics() {
+        if config.metricsEnabled() {
+            for metricsEngine: Collectable in metrics {
+                // TODO: Use single URL payload vs metrics sending data to individual URLS.


@aliok @pb82 - What will be your take on this? Should we have single UDP/HTTP metrics endpoint or each metrics should be as separate endpoint?

From mobile point of view it may be better to just have single endpoint and collect metrics into single payload that is being sent when device is online. WDYT?

+1 on this. not just when offline though. even when the device is online, we should send stuff in batches into a single URL.
but I wouldn't worry about this right now. perhaps we can start a discussion with the wider group.

@wtrocki maybe it is better to talk about this once we have more metrics (custom or system). currently all we have is 2 metrics (sdk version and app version) that can be joined in a single payload. but we shouldn't forget this subject.

wtrocki · 2018-02-07T18:43:24Z

Retrieving Version in IOS

After experimenting with multiple options like custom cocoapods plugins, plist appenders I decided to include version in core. Version will be changed by release script so there will be no additional overhead for developers to update that.

wtrocki · 2018-02-07T18:47:56Z

modules/metrics/metrics/SdkVersion.swift

+    }
+
+    func collect() {
+        // TODO: Store version in User Preferences and detect that was changed?


@aliok @pb82 WDYT? Technically this metrics will change only when user will update application so sending this metrics every time we launch app seems to be battery drainer, antipattern.

SDK version can be saved in preferences and we can detect when it was changed.
Additionally both IOS and Android have way to detect if application was installed.
However if we agree to have one single endpoint for all "OnStart" metrics then this optimization will not make sense as we could always send this information as metrics payload.

I think sending SDK init events every time is very useful. Anybody could make use of this data.
Sending once would answer "how many clients use SDK 1.2.3?".
Sending every time would also answer "how many init events happened for SDK 1.2.3 in last N days?" which is basically the how actively an SDK version is used. This is IMO important because there can be 1 installation of SDK 1.0.0 with a very active user, but there can be 100 installations of SDK 0.0.1 with nobody actually using the app.

Also, probably sending one request when app starts won't drain too much battery IMO :)

If we do this, from backend point of view it doesn't change too much. We would just need to adjust the Grafana dashboard so that it doesn't show sdk init event time series data.

@wtrocki I also agree with @aliok here. Sending the data every time the app is launched is far more useful from a metrics perspective.

wtrocki · 2018-02-07T18:58:44Z

@danielpassos Apart from couple unanswered questions I think we are in good shape to review this.
Build is failing intentionally - I left TODO statements in code.

wtrocki · 2018-02-08T09:58:17Z

@aliok Thanks for responding. Fully agree with you. To summarize:
We are going to send metrics everytime application is launched (initially) and use single payload url that will be provided in mobile-config.json

Going to push that to list as well.

danielpassos · 2018-02-08T16:41:13Z

example/AeroGearSdkExample/AppDelegate.swift

@@ -27,6 +29,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
    }

    func applicationDidBecomeActive(_: UIApplication) {
+        metrics.collectMetrics()


MetricsContainer don't have the collectMetrics method. I think it should be AgsMetrics.collectMetrics

Outdated. I have fixed that before.

danielpassos · 2018-02-08T16:46:40Z

example/AeroGearSdkExampleTests/metrics/MetricsTests.swift

+import Foundation
+import XCTest
+
+class MetricsTests: XCTestCase {


What is this test testing? 😕

danielpassos · 2018-02-08T16:47:31Z

modules/core/AgsMetadata.swift

+//  Metadata.swift
+//  AGSCore
+//
+//  Created by Wojciech Trocki on 2/7/18.


Needs copyright update

danielpassos · 2018-02-08T17:39:24Z

modules/metrics/processor/MetricsPublisher.swift

+ * Metrics publisher that logs metrics to console
+ * Can be used when remote server is not available/supported
+ */
+public class MetricsLoggerPublisher: MetricsPublisher {


Like I said before I think we should not have more than one class/protocol/whatever in the same file, It means things easy to be found.

+100, Sorry I missed that one

danielpassos · 2018-02-08T17:41:13Z

modules/metrics/data/MetricsConfig.swift

+import AGSCore
+import Foundation
+
+struct MetricsConfigData {


Where are we using it?

It's wrapper to get url and future. I wanted to showcase common swift pattern here.
While this class has little code it's good showcase (metrics is first and it will be good to show some good patterns here like optional unwrapping. Initially I had much more code here but decided to simplify that.

danielpassos · 2018-02-08T17:44:58Z

modules/metrics/data/AppData.swift

+/**
+ * Holds static application data that can be used for metrics
+ */
+public class AppData {


Just wondering if this kind of class should not be on core to be used in other modules or in the app.

danielpassos · 2018-02-08T17:46:37Z

modules/metrics/processor/Collectable.swift

+/**
+ * Protocol used for for classes that will colect metrics
+ */
+public protocol Collectable {


Why this protocol are in processor folder and the implementations are in the metrics folder?

metrics folder contains all metrics. "processor' is maybe bad name - I will rename that to engine

danielpassos · 2018-02-08T17:47:19Z

modules/metrics/processor/Collectable.swift

+/**
+ * Protocol used for for classes that will colect metrics
+ */
+public protocol Collectable {


I think it should have metrics in the name to be more explicit it's a Collectable for Metrics. Makes sense?

danielpassos · 2018-02-08T17:51:10Z

modules/metrics/AgsMetrics.swift

+    private let core: AgsCore
+    private let appData: AppData
+    private let config: MetricsConfig
+    private var publisher: MetricsPublisher!


What about to have more than one MetricsPublisher. I mean what if I wanna have Log and Network or whatever new I write?

Idea now is to use single one - strategy pattern. If there will be use case for two we could change that and iterate on publishers without hassle. Currently we support only app init event. Once more triggers will appear this functionality may change.

danielpassos · 2018-02-08T17:57:32Z

modules/metrics/MetricsContainer.swift

+ * Protocol used for mobile metrics management
+ * Allows other SDK and implementations to manage (add) metrics
+ */
+public protocol MetricsContainer {


Do we really need a protocol for this? Why not only a simple method in AgsMetrics?

This was created in order to use it in different SDK in order to register metrics.
I'm not sure if this Idea will last, but I wanted to propose some solution for extending metrics without implementing them directly in the metrics SDK.

Alternative path will be to just get specific metrics injected on user app.
I think we could talk about couple options here with @pb82 as we will still continue work on this by adding security etc.

danielpassos · 2018-02-08T19:20:04Z

aliok · 2018-02-09T08:13:47Z

@wtrocki I know this is merged, but do you mind updating the PR description? Seems outdated and could be confusing in the future.

wei-lee reviewed Feb 6, 2018

View reviewed changes

wtrocki force-pushed the metrics-sdk branch from 611a682 to 5958f20 Compare February 6, 2018 16:10

wei-lee reviewed Feb 6, 2018

View reviewed changes

wtrocki force-pushed the metrics-sdk branch from 5958f20 to 12f05bf Compare February 6, 2018 17:50

wtrocki requested review from danielpassos and pb82 February 6, 2018 17:51

pb82 reviewed Feb 7, 2018

View reviewed changes

pb82 approved these changes Feb 7, 2018

View reviewed changes

wtrocki requested a review from StevenTobin February 7, 2018 12:07

wtrocki force-pushed the metrics-sdk branch from 12f05bf to 7236c7d Compare February 7, 2018 18:32

Metrics SDK

3bf4aa7

wtrocki force-pushed the metrics-sdk branch from 7236c7d to 3bf4aa7 Compare February 7, 2018 18:37

wtrocki commented Feb 7, 2018

View reviewed changes

wtrocki added 2 commits February 7, 2018 18:40

Add documentation

fda04a3

Add supported metrics to docs

2f8912b

wtrocki commented Feb 7, 2018

View reviewed changes

Allow to override metrics setup

84bf6fd

Add publisher abstraction

e4607c4

wtrocki requested a review from aidenkeating February 8, 2018 12:52

wtrocki added 2 commits February 8, 2018 14:28

Remove not needed variable

32dbc7c

Documentation for collecting metrics

7f6a39d

wtrocki mentioned this pull request Feb 8, 2018

Add Metrics aerogear/aerogear-android-sdk#27

Merged

3 tasks

wtrocki added 2 commits February 8, 2018 15:46

Add link to Core SDK documentation

b711b4e

Adjust changes to new format

2b9c853

Extend interface to additional methods

1a152ea

wtrocki force-pushed the metrics-sdk branch from 089cd8b to 0a6657a Compare February 8, 2018 17:04

Fix lint errors

4465d37

wtrocki force-pushed the metrics-sdk branch from 0a6657a to 4465d37 Compare February 8, 2018 17:06

wtrocki added 2 commits February 8, 2018 17:24

Add swiftlint configuration

7b10e03

Fix unit test

ef2fb5f

danielpassos suggested changes Feb 8, 2018

View reviewed changes

wtrocki added 5 commits February 8, 2018 18:12

Remove empty tests

d26bc74

Delete outdated comment

d1e3e0a

Move metadata to core

01909a7

Move to MetricsCollectable

00c5e32

Fix swift lint config

34ab097

danielpassos approved these changes Feb 8, 2018

View reviewed changes

danielpassos merged commit 661390e into aerogear:master Feb 8, 2018

wtrocki deleted the metrics-sdk branch February 8, 2018 19:51

wtrocki mentioned this pull request Feb 12, 2018

Mobile App Metrics and SDK version tracking proposal (2nd one) aerogear/proposals#22

Merged

Metrics sdk #13

Metrics sdk #13

Conversation

wtrocki commented Feb 6, 2018 • edited

Motivation

Progress

Choose a reason for hiding this comment

wtrocki Feb 6, 2018 • edited

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 Feb 7, 2018 • edited

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

pb82 left a 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 7, 2018

Retrieving Version in IOS

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtrocki commented Feb 7, 2018

wtrocki commented Feb 8, 2018

Choose a reason for hiding this comment

wtrocki 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

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

danielpassos commented Feb 8, 2018

aliok commented Feb 9, 2018

wtrocki commented Feb 6, 2018 •

edited

wtrocki Feb 6, 2018 •

edited

wtrocki Feb 7, 2018 •

edited

wtrocki Feb 8, 2018 •

edited