extend event logging for debugging purposes #22

rclee · 2017-11-07T22:37:54Z

-adds new method to MMEEventLogger for writing events to a local file
-writes any event passing through sendTurnstile or pushEvent methods in MMEEventsManager

-adds new method to MMEEventLogger for writing events to a local file -writes any event passing through sendTurnstile or pushEvent methods in MMEEventsManager

boundsj · 2017-11-07T22:56:36Z

MapboxMobileEvents/MMEEventLogger.h

@@ -10,6 +10,8 @@ NS_ASSUME_NONNULL_BEGIN
 + (void)setEnabled:(BOOL)enabled;


We could probably make enabled a property.

I forgot that this class has so far only used class methods to invoke logging (e.g. [MMEEventLogger logEvent:debugEvent]). This PR introduces the notion of using an instance of MMEEventLogger in MMEEventsManager.

I think we should change the methods to make them instance based and add a singleton interface so that we can use a [MMEEventLogger sharedLogger] instance everywhere we want log messages to go to the same file with the same date. We can still create 1-off instances for other kinds of logging we may eventually want to do in different files.

boundsj

Looking good! Please have a look at the comments wrt rationalizing how we use this class as a shared instance vs one off instances.

boundsj · 2017-11-07T23:03:51Z

MapboxMobileEvents/MMEEventLogger.h

@@ -10,6 +10,8 @@ NS_ASSUME_NONNULL_BEGIN
 + (void)setEnabled:(BOOL)enabled;


I forgot that this class has so far only used class methods to invoke logging (e.g. [MMEEventLogger logEvent:debugEvent]). This PR introduces the notion of using an instance of MMEEventLogger in MMEEventsManager.

I think we should change the methods to make them instance based and add a singleton interface so that we can use a [MMEEventLogger sharedLogger] instance everywhere we want log messages to go to the same file with the same date. We can still create 1-off instances for other kinds of logging we may eventually want to do in different files.

boundsj · 2017-11-07T23:08:17Z

MapboxMobileEvents/MMEEventLogger.m

@@ -18,4 +26,44 @@ + (void)setEnabled:(BOOL)enabled {
    _enabled = enabled;
 }

+- (void)writeEventToLocalDebugLog:(MMEEvent *)event {


I think we can go ahead and combine this new method and the old logEvent. I like the name logEvent because it is easier to say. We can just assume that if something gets logged it is written to the console and to disk.

I definitely like this approach better. Initially I had similar thoughts with combining the two methods. My only concern was potentially having too many logs written locally and that a majority of the debug logs will have the same MMEEventKeyLocalDebug key.

It seems like we'll still need to have other keys for at least flush and push and that would likely result in any MMEEventKeyLocalDebug key/value pairs being unused; unless that information is interesting in some way...

Let's go ahead and default to writing "too much" for now as a first pass. We can eventually introduce the notion of log levels to filter what actually gets printed and written.

boundsj

Thanks for making the changes to allow for using the logger as a singleton. This helped illustrate for me some other changes I think we should make.

boundsj · 2017-11-08T22:44:11Z

MapboxMobileEvents/MMEEvent.m

@@ -5,6 +5,20 @@

 @implementation MMEEvent

+ (instancetype)pushEventWithAttributes:(NSDictionary *)attributes {


Instead of making an API that would need to expand for new, arbitrary things that could all be categorized as "debug" events, let's reuse the existing debugEventWithAttributes API and plan on using the attributes for distinction.

I'm saying this also because I think of push and flush as actions that we want to track in a debug log but not as actual events that we send to the server, if that makes sense.

boundsj · 2017-11-08T22:52:39Z

MapboxMobileEvents/MMEEventsManager.m

@@ -157,6 +157,12 @@ - (void)flush {
        return;
    }

+    NSDictionary *flushEventAttributes = @{MMEEventKeyEvent: MMEEventTypeFlush,
+                                          MMEEventKeyCreated: [self.dateWrapper formattedDateStringForDate:[self.dateWrapper date]]};
+    MMEEvent *flushEvent = [MMEEvent pushEventWithAttributes:flushEventAttributes];


With the changes in this PR so far, I think this should actually be flushEventWithAttributes:. But let's make the change to use the unified debugEventWithAttributes. As a debug event, the event name will be MMEEventTypeLocalDebug (that is done for us by debugEventWithAttributes:) and we could introduce a "debug event type" that we put in the attributes. In this case, that could be a string constant called MMEDebugEventTypeFlush that equals "flush".

boundsj · 2017-11-08T22:58:13Z

MapboxMobileEvents/MMEEventsManager.m

@@ -375,7 +389,7 @@ - (void)pushDebugEventWithAttributes:(MMEMapboxEventAttributes *)attributes {
    [combinedAttributes setObject:[self.dateWrapper formattedDateStringForDate:[self.dateWrapper date]] forKey:@"created"];
    [combinedAttributes setObject:self.uniqueIdentifer.rollingInstanceIdentifer forKey:@"instance"];
    MMEEvent *debugEvent = [MMEEvent debugEventWithAttributes:combinedAttributes];
-    [MMEEventLogger logEvent:debugEvent];
+    [MMEEventLogger.sharedLogger logEvent:debugEvent];


For all of the existing invocations of pushDebugEventWithAttributes: we should make sure their attributes have a MMEDebugEventType and MMEDebugEventType{foo} value that makes sense for the debug log entries they represent.

-introduces MMEDebugEventTypes to MMEConstants -adds debug types to pushDebugEventWithAttributes:

boundsj · 2017-11-09T22:22:31Z

MapboxMobileEvents/MMEEventLogger.h

+
+- (void)logEvent:(MMEEvent *)event;
+
+- (void)writeEventToLocalDebugLog:(MMEEvent *)event;


I guess we could remove this in the header file and make it an implementation detail.

boundsj · 2017-11-09T22:26:24Z

MapboxMobileEvents/MMEEventsManager.m

@@ -273,19 +290,21 @@ - (void)createAndPushEventBasedOnName:(NSString *)name attributes:(NSDictionary
    }

    if (event) {
-        [self pushDebugEventWithAttributes:@{MMEEventKeyLocalDebugDescription: [NSString stringWithFormat:@"Pushing event: %@", event]}];
+        [self pushDebugEventWithAttributes:@{MMEDebugEventType: MMEDebugEventTypePush,


This is fine for now but for MMEDebugEventTypePush and MMEDebugEventTypeTurnstile similarly, we should probably make event sub-types to make it easier to visualize them when we build something to parse these logs. Otherwise MMEDebugEventTypePush success and failure case will be hard to discern unless we parse the MMEEventKeyLocalDebugDescription

extend event logging

3498613

-adds new method to MMEEventLogger for writing events to a local file -writes any event passing through sendTurnstile or pushEvent methods in MMEEventsManager

rclee requested a review from boundsj November 7, 2017 22:37

boundsj reviewed Nov 7, 2017

View reviewed changes

Add MMEEventLogger singleton and event types

0e342b9

boundsj suggested changes Nov 8, 2017

View reviewed changes

rclee added 3 commits November 9, 2017 13:23

Add DebugTypes to MMEConstants

c180872

-introduces MMEDebugEventTypes to MMEConstants -adds debug types to pushDebugEventWithAttributes:

Remove old methods

f14c3bc

Fix missing key

1f2f1f9

boundsj previously approved these changes Nov 9, 2017

View reviewed changes

Remove writeEventToLocalDebugLog: from header

d359ab8

rclee dismissed boundsj’s stale review via d359ab8 November 9, 2017 22:33

rclee merged commit ca193a4 into master Nov 9, 2017

rclee deleted the events-profiling branch November 9, 2017 22:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extend event logging for debugging purposes #22

extend event logging for debugging purposes #22

rclee commented Nov 7, 2017

boundsj Nov 7, 2017

boundsj Nov 7, 2017

boundsj left a comment

boundsj Nov 7, 2017

boundsj Nov 7, 2017

rclee Nov 8, 2017

boundsj Nov 8, 2017

boundsj left a comment

boundsj Nov 8, 2017

boundsj Nov 8, 2017

boundsj Nov 8, 2017

boundsj Nov 9, 2017

boundsj Nov 9, 2017

		@@ -10,6 +10,8 @@ NS_ASSUME_NONNULL_BEGIN
		+ (void)setEnabled:(BOOL)enabled;

		@@ -5,6 +5,20 @@

		@implementation MMEEvent

		+ (instancetype)pushEventWithAttributes:(NSDictionary *)attributes {


		- (void)logEvent:(MMEEvent *)event;

		- (void)writeEventToLocalDebugLog:(MMEEvent *)event;

extend event logging for debugging purposes #22

extend event logging for debugging purposes #22

Conversation

rclee commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boundsj 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

Choose a reason for hiding this comment

boundsj 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment