[ios] Log event when offline pack is created #13039
Conversation
@"minZoom": @0, // get zoom level | ||
@"maxZoom": @0, // get zoom level | ||
@"sources": @[] // ? | ||
}]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@1ec5 I just realized I'm not sure if there is a way we can access the source of the style being used here. MGLOfflineRegion
doesn't have access to an MGLStyle
's sources, only the style URL itself, and there doesn't seem to be a way to initialize a new MGLStyle
outside of the context of an MGLMapView
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s true that MGLOfflineStorage doesn’t have access to the runtime styling models, but the first resource that mbgl::OfflineManager downloads for any pack is the style JSON file. That file tells mbgl about all the sources, so at that point it could execute a callback that tells the SDK the URLs of the sources involved.
Note that some sources aren’t backed by TileJSON files, instead being inlined in the style. Inlined sources aren’t as interesting for analytical purposes, since they tend to be one-offs. Either way, there’s an mbgl::TileSet object that the SDK could be told about, and the SDK could then decide what to send as part of the event.
@"styleURL": self.region.styleURL, | ||
@"resourcesDownloaded": [NSNumber numberWithUnsignedInteger:self.progress.countOfResourcesCompleted], | ||
@"tilesDownloaded": [NSNumber numberWithUnsignedInteger:self.progress.countOfTilesCompleted] | ||
}]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When logging the attributes of the event type, it seems like resourcesDownloaded
and tilesDownloaded
don't pass in a correct number (they're both zero). 🤔
All other attributes populate as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point countOfResourcesCompleted
etc haven't been updated (https://github.com/mapbox/mapbox-gl-native/pull/13039/files#diff-2e0199077c3e9d88f9c6669352f18d67R220).
I wonder if MGLMapView
(or something else) should register for the MGLOfflinePackProgressChangedNotification
notification - and push the event there?
9d6b6d9
to
a71e3d2
Compare
The scope of this PR only includes logging when an |
|
||
NSDictionary *attributes; | ||
|
||
if ([region isKindOfClass:[MGLTilePyramidOfflineRegion class]]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you already have tileRegion
and shapeRegion
you could just check if (tileRegion) ...
here.
@"maxZoom": [NSNumber numberWithDouble:tileRegion.maximumZoomLevel], | ||
@"styleURL": region.styleURL | ||
}; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly here, you should check if (shapeRegion) ...
However, I think it would be better if we added a readonly property (or method) to MGLOfflineRegion_Private
, along the lines of:
@property (nonatomic, readonly) NSDictionary *eventAttributes;
which both MGLTilePyramidOfflineRegion
and MGLShapeOfflineRegion
implement, e.g (tweaking your above attribute dictionary):
-(NSDictionary *)eventAttributes {
return @{
@"shapeForOfflineRegion": @"tileregion",
@"minZoom": @(self.minimumZoomLevel),
@"maxZoom": @(self.maximumZoomLevel),
@"styleURL": self.styleURL // assuming non-null
};
}
Then in this code you'd check for conformance:
NSMutableDictionary *eventAttributes = [NSMutableDictionary dictionaryWithObject:MMEventTypeOfflineDownloadStart forKey:@"event"];
// Add region specific attributes
if ([region conformsToProtocol:@protocol(MGLOfflineRegion_Private)]) {
NSDictionary *regionAttributes = ((id<MGLOfflineRegion_Private>)region).eventAttributes;
[eventAttributes addEntriesFromDictionary:regionAttributes];
}
[MGLMapboxEvents pushEvent:MMEventTypeOfflineDownloadStart withAttributes:attributes];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Longer term, we should consider whether having the pushEvent:withAttributes
call here1 is the right thing to do. Instead perhaps we delegate or broadcast a notification, that something can listen for and it's that object's responsibility to push the event. But this particular topic shouldn't hold up this PR.
1 This also goes for other places we're currently calling pushEvent...
/cc @fabian-guerra
}; | ||
} else { | ||
attributes = @{ | ||
@"event": MMEventTypeOfflineDownloadStart, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the attribute keys should be of the form MMEEventKey...
; for example, instead of event
use MMEEventKeyEvent
.
You'll probably need to add constants for the other keys?
attributes = @{ | ||
@"event": MMEventTypeOfflineDownloadStart, | ||
@"shapeForOfflineRegion": @"tileregion", | ||
@"minZoom": [NSNumber numberWithDouble:tileRegion.minimumZoomLevel], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of [NSNumber numberWithDouble:]
you can use the short-hand form @(<double>)
.
Waiting for mapbox/mapbox-events-ios#83 to merge so I can bump the library here and address #13039 (comment). |
ac06e9b
to
a14b43c
Compare
954f3b3
to
009082b
Compare
@@ -22,6 +26,17 @@ @implementation MGLShapeOfflineRegion { | |||
|
|||
@synthesize styleURL = _styleURL; | |||
|
|||
#if TARGET_OS_IPHONE || TARGET_OS_SIMULATOR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be inside the method, otherwise you'll be missing an implementation for offlineStartEventAttributes
. You could either wrap the MMEEvents, or just have an else clause where you return a @{}
009082b
to
e15b723
Compare
@@ -357,6 +363,17 @@ - (void)addPackForRegion:(id <MGLOfflineRegion>)region withContext:(NSData *)con | |||
[[strongSelf mutableArrayValueForKey:@"packs"] addObject:pack]; | |||
if (completion) { | |||
completion(pack, error); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the closing }
here, so that we always push the event regardless of whether a completion block is specified.
e15b723
to
d4354fa
Compare
Part II of #12751.
Integrates the new offline event into the Mapbox Maps SDK framework, logging an event when a new region is added with
[MGLSharedOfflineStorage addPackForRegion:withContext:completionHandler:]
.This PR also bumps
mapbox-events-ios
to the current mapbox/mapbox-events-ios@61fca37 commit onmaster
in order to take advantage of the corresponding new event class and constants.