Added a Logger that can log messages in different levels. #156

brototyp · 2017-06-05T10:56:27Z

Closes : #147

Add Documentation to the new property
Add Documentation in the Readme
Add Documentation on how to write the own Logger
Add Changelog entry

minitrue22 · 2017-06-05T17:14:40Z

Example/iOS Example/AppDelegate.swift

@@ -43,6 +43,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
                return
        }
        PiwikTracker.configureSharedInstance(withSiteID: siteid, baseURL: baseurl)
+        PiwikTracker.shared?.logger?.minLevel = .info


I know it is not purpose of this PR but,
I would really suggest staying away from singleton and shared instances.

A design goal would be to run multiple trackers instances in the same app.

Let the consumer of the framework create their own shared instance if they want.

Maybe create an issue for tracking this refactoring if we think it is a good idea.

There is a ticket (#118) to ensure the Tracker can be instantiated multiple times and I agree that it is necessary to do so. But I don't think we should remove the default, shared logger. I think most users just use a single tracker and are used to have a shared instance. At the same time it doesn't break any other features.

I agree with @brototyp , I also think that users would like to have a shared instance to as a convenient way to use a logger. More ambitious users can use their own instances of loggers.

My experience from looking at a lot of iOS/Mac code is a pattern of overusing singletons. I am just trying to avoid adding to this problem :)

I personally do not see any reason for the framework to provide a singleton. A user that really want to use the singleton pattern in their code can just declare a public variable in the AppDelegate or introduce a Tracker class with a shared variable or a global variable.

Up to you. I am just on a personal crusade to reduce the use of singletons.

Maybe look at Alamofire if you want to offer a "default" instance without having to instantiate one.

minitrue22 · 2017-06-05T17:18:05Z

PiwikTracker/Logger.swift

@@ -0,0 +1,55 @@
+import Foundation
+
+public enum LogLevel: Int {


Does it have to be public?

See the comment below.

minitrue22 · 2017-06-05T17:18:21Z

PiwikTracker/Logger.swift

+    }
+}
+
+public protocol Logger {


Does it have to be public?

See the comment below.

minitrue22 · 2017-06-05T17:24:39Z

PiwikTracker/Logger.swift

+
+public protocol Logger {
+    var minLevel: LogLevel { get set }
+    func log(message: ()->(String), with level: LogLevel)


Could capture file, function line number in the logs

func log(_ message: Void -> String, with level: LogLevel, file: String = #file, function: String = #function, line: Int = #line )

the extension methods below also needs the same params for this to work

Love it! Good idea!

Just changed this. It's great!

minitrue22 · 2017-06-05T17:29:28Z

PiwikTracker/Logger.swift

+    }
+}
+
+final class DefaultLogger: Logger {


I would consider making this a struct.
But final class is almost as good :)

I rather like to use structs for data structures whose main task is to hold data. For things like this, where the main task is to "do things" or "define behavior" I like to use classes. What do you think about this?

minitrue22 · 2017-06-05T17:34:26Z

PiwikTracker/PiwikTracker.swift

@@ -22,6 +22,8 @@ final public class PiwikTracker: NSObject {
    private var queue: Queue
    internal let siteId: String

+    public var logger: Logger? = DefaultLogger(minLevel: .warning)


I would probably make this non optional and not expose the Logger implementations outside the framework.

If the consumer of the framework needs to change the logging level I would hide that behind a method on the PiwikTracker class, e.g. enableDebugLogging that creates and assign new immutable DefaultLogger instance with an appropriate log level.

See the comment below.

I still suggest making the logger non optional even if exposing it as a property.
The code will become more easy to write and reason about and the user will be explicitly forced to provide a value.

You can provide a default implementation of the ´NoOpLogger´ that user can set to make it easy for them.

I created a DisabledLogger.

minitrue22 · 2017-06-05T17:35:44Z

PiwikTracker/PiwikTracker.swift

@@ -22,6 +22,8 @@ final public class PiwikTracker: NSObject {
    private var queue: Queue
    internal let siteId: String

+    public var logger: Logger? = DefaultLogger(minLevel: .warning)
+
    internal static var _sharedInstance: PiwikTracker?


I personally think _ prefixing is a thing of the past but accept if someone else thinks otherwise :)

Do you think omitting the underscore would be best?

I would not use '_' in any variable or instance names.

Often if you find yourself facing problems with naming resorting to using _ or other type of prefix/post fix to avoid naming conflicts it is an indication of a design problem.

minitrue22 · 2017-06-05T17:46:26Z

PiwikTracker/PiwikTracker.swift

@@ -55,6 +57,7 @@ final public class PiwikTracker: NSObject {

    internal func queue(event: Event) {
        guard !isOptedOut else { return }
+        logger?.verbose(){ return "Queued event: \(event)" }


Would

logger?.verbose { "Queued event: \(event)" }

work?

func verbose(_ message: @autoclosure ()->(String)) { ... }

will allow and even conciser syntax
verbose("123")

Oh! I didn't know about the @autoclosure. It's perfect for this use case. Will add it.

Just changed this. It's great!

minitrue22 · 2017-06-05T17:49:46Z

Podfile.lock

@@ -1,8 +1,8 @@
 PODS:


Not sure the .lock file should be tracked by Git.
Maybe add it to .gitignore?

Generally I like to track the .lock file to define which exact versions this is ensured to work with. In this case, since there are only dependencies for the tests, I agree.

brototyp · 2017-06-06T07:30:32Z

One core feature I wanted to go for in this implementation is a logger that can be replaced. So one could write an adapter for their own Logger (Cocoalumberjack, XCGLogger, ...). That's why the Logger protocol and LogLevel enum as well as there logger property of the Tracker are public.

minitrue22 · 2017-06-13T03:17:26Z

One core feature I wanted to go for in this implementation is a logger that can be replaced. So one could write an adapter for their own Logger (Cocoalumberjack, XCGLogger, ...). That's why the Logger protocol and LogLevel enum as well as there logger property of the Tracker are public.

Ok, I understand, maybe a good idea.

I just think logging is not a core feature for a consumer of the framework. I almost think the framework should never log anything when used in production. Any information that needs to reach the consumer should be a return values or exception. Logging in the context of this framework is only for developers of the framework it self. This will force the developer of the framework to think about the design.

But I understand the purpose and it is ok.

minitrue22 · 2017-06-13T03:33:46Z

PiwikTracker/Logger.swift

+
+    func log(_ message: @autoclosure () -> String, with level: LogLevel, file: String = #file, function: String = #function, line: Int = #line) {
+        guard level.rawValue >= minLevel.rawValue else { return }
+        print("PiwikTracker [\(level.shortcut)] \(message())")


Forgot about this.
print is not thread safe (I think).
If this method is called from different thread the print statements will be jumbled up.

You can solve that by doing the printing on a serial queue.
If you do that just must be careful about running message() on the calling thread before switching to a serial queue or you might get crashes depending on what the provided closure does.

You should probably add a comment on the Logger protocol that the implementation needs to deal with any threading themselves. Of instead of exposing the Logger to the consumer expose an Appender the consumer can set on the Logger and let Logger handle the threading.

Right! Thanks for pointing this out!

brototyp added the swift label Jun 5, 2017

brototyp added this to the 4.1.0 milestone Jun 5, 2017

brototyp requested a review from thorstenstark June 5, 2017 10:56

minitrue22 reviewed Jun 5, 2017

View reviewed changes

brototyp force-pushed the feature/147-logger branch 2 times, most recently from 4a177ce to cd80305 Compare June 7, 2017 15:54

minitrue22 reviewed Jun 13, 2017

View reviewed changes

Added a Logger that can log messages in different levels.

71ffe7a

brototyp force-pushed the feature/147-logger branch from cd80305 to 71ffe7a Compare June 22, 2017 19:56

brototyp merged commit 046965d into develop Jun 22, 2017

brototyp deleted the feature/147-logger branch October 14, 2017 10:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a Logger that can log messages in different levels. #156

Added a Logger that can log messages in different levels. #156

brototyp commented Jun 5, 2017 •

edited

minitrue22 Jun 5, 2017

minitrue22 Jun 5, 2017

brototyp Jun 6, 2017

thorstenstark Jun 12, 2017

minitrue22 Jun 13, 2017 •

edited

minitrue22 Jun 13, 2017

minitrue22 Jun 5, 2017

brototyp Jun 6, 2017

minitrue22 Jun 5, 2017

brototyp Jun 6, 2017

minitrue22 Jun 5, 2017

brototyp Jun 6, 2017

brototyp Jun 7, 2017

minitrue22 Jun 5, 2017

brototyp Jun 6, 2017

minitrue22 Jun 5, 2017

brototyp Jun 6, 2017

minitrue22 Jun 13, 2017 •

edited

brototyp Jun 22, 2017

minitrue22 Jun 5, 2017

brototyp Jun 6, 2017

minitrue22 Jun 13, 2017

minitrue22 Jun 5, 2017

minitrue22 Jun 5, 2017 •

edited

brototyp Jun 6, 2017

brototyp Jun 7, 2017

minitrue22 Jun 5, 2017

brototyp Jun 6, 2017

brototyp commented Jun 6, 2017

minitrue22 commented Jun 13, 2017 •

edited

minitrue22 Jun 13, 2017 •

edited

brototyp Jun 22, 2017

		@@ -0,0 +1,55 @@
		import Foundation

		public enum LogLevel: Int {

Added a Logger that can log messages in different levels. #156

Added a Logger that can log messages in different levels. #156

Conversation

brototyp commented Jun 5, 2017 • 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

minitrue22 Jun 13, 2017 • 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

minitrue22 Jun 13, 2017 • 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

minitrue22 Jun 5, 2017 • 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

brototyp commented Jun 6, 2017

minitrue22 commented Jun 13, 2017 • edited

minitrue22 Jun 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brototyp commented Jun 5, 2017 •

edited

minitrue22 Jun 13, 2017 •

edited

minitrue22 Jun 13, 2017 •

edited

minitrue22 Jun 5, 2017 •

edited

minitrue22 commented Jun 13, 2017 •

edited

minitrue22 Jun 13, 2017 •

edited