[ios] #3979 compress telemetry events #5490

frederoni · 2016-06-27T12:04:10Z

WiP

Things to consider:

~~License for dependency (zlib)~~
Threshold for compression
Build request in a background thread

An event in the following test case is lat, lng, timestamp with randomized values.

Events	Raw size	Compressed size	Compression
1 event*	65 bytes	85 bytes	1.3076
2 events	117 bytes	100 bytes	0.8547
5 events	273 bytes	126 bytes	0.4615
180 events	9125 bytes	1290 bytes	0.1413

*Compressing one event actually increases the size of data on top of using unnecessary CPU cycles.

👀 @boundsj @friedbunny @1ec5

friedbunny · 2016-06-27T15:19:27Z

Awesome, these results look quite promising!

You should go ahead and add a copy of NSData+GZIP's license to our list of licenses. Looks like the license in the repo is slightly different and more up-to-date. We also already include the original zlib license, so I think we’re covered there.

@1ec5 mentioned prefixing as a thing we should do, as we’ve done with Reachability, which became MGLReachability. In this case, we’ll want to prefix the category methods with something like mgl_.

NSData+GZIP’s files should also probably live in a subfolder at platform/ios/vendor, rather than commingled with our own source in platform/ios/src. (SMCalloutView is still a submodule there, but checking in NSData+GZIP is definitely the way to go.)

1ec5 · 2016-06-27T16:05:21Z

So it turns out we already have a simple interface to zlib in the codebase, in src/mbgl/util/compression.hpp. (It's used to compress and decompress tile data, part of the reason we already include the zlib license in our LICENSE file.) The header is currently private to mbgl, but I don't see any reason we couldn't move it to include/mbgl/util/ and use it within the SDK. compression.hpp uses std::string to hold the compressed data, but it's trivial to dump that buffer into an NSData.

/cc @kkaefer

frederoni · 2016-06-28T14:22:57Z

Exposed the zlib interface we already had in include/mbgl/util/compression.hpp and got some interesting results

Events	Raw size	Compressed size	Compression
1 event	52 bytes	52 bytes	1
2 events	103 bytes	105 bytes	1.0194
5 events	251 bytes	115 bytes	0.4581
180 events	9136 bytes	267 bytes	0.0292

frederoni · 2016-06-29T16:28:19Z

Noticed that the compressed data was corrupt so even though we don't need it for the iOS SDK, I had to add -mgl_decompressedData just to be able to write a test case.

1ec5 · 2016-06-29T17:26:06Z

platform/darwin/src/NSData+MGLAdditions.mm

+{
+    NSString *dataString = [[NSString alloc] initWithData:self encoding:NSUTF8StringEncoding];
+
+    std::string *string = new std::string([dataString UTF8String]);


C++ types should be allocated on the stack, whenever possible, rather than the heap.

1ec5 · 2016-06-29T17:47:57Z

platform/darwin/src/NSData+MGLAdditions.h

+
+@end
+
+NS_ASSUME_NONNULL_END


Nit: missing newline at the end.

1ec5 · 2016-06-29T18:00:14Z

It looks like you’ve copied src/mbgl/util/compression.hpp to include/mbgl/util/compression.hpp. Instead, you should move the file there. The convention is that headers in include/ are public (to SDK code), while headers in src/ can only be used in the SDK. Once you delete src/mbgl/util/compression.hpp, you’ll need to update compression.cpp to include <mbgl/util/compression.hpp> instead of "compression.hpp".

boundsj · 2016-06-29T18:05:58Z

platform/ios/test/MGLNSDataAdditionsTests.m

+
+- (float)randomFloatBetween:(float)lowerBound and:(float)upperBound {
+    float diff = upperBound - lowerBound;
+    return (((float) (arc4random() % ((unsigned)RAND_MAX + 1)) / RAND_MAX) * diff) + lowerBound;


nit: you might consider using arc4random_uniform

Hrm, good to know. 💭

Good one but since I want lat/lng I'm probably gonna go with drand48.
I'm grabbing the value as integer after that because NSArray/NSDictionary -isEqualToDictionary/Array: returns false despite the content being the same. Any idea why?

Comparisons between floats often fail due to floating-point representations being imprecise. That’d be my first guess. If that turns out to be the case, try multiplying the latitude and longitude by enough decimal places that the imprecision wouldn’t matter much in reality, then floor the values. See this table to get an idea of how much precision you need.

1ec5 · 2016-06-30T18:25:41Z

👍

kkaefer · 2016-07-04T12:40:03Z

It's worth noting that this is a non-standard way of using HTTP; theoretically our client cannot know that the server supports Content-Encoding: gzip, but given that the endpoint is always a server we control, we can deploy this anyway.

frederoni · 2016-07-04T13:31:03Z

You're right @kkaefer. I guess we could verify supported Content-Encoding by making a HEAD request with Accept-Encoding: gzip (or in our case: deflate since we're not compressing with a gzip wrapper but zlib deflate compression) to verify that the endpoint supports it. But the endpoint doesn't support HEAD requests and it's not guaranteed even if it did. Only more likely if it's supported at http server level. And as you mentioned, the endpoint is always a server we control.

kkaefer · 2016-07-04T14:15:56Z

@frederoni not saying we should do anything else, I just wanted to make everyone aware that this is something we explicitly need to support in our server (and we already do).

frederoni added iOS Mapbox Maps SDK for iOS telemetry Integration with Mapbox Telemetry libraries ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Jun 27, 2016

frederoni force-pushed the 3979-gzip-telemetry-events branch 2 times, most recently from 255e230 to 9c7e25e Compare June 29, 2016 16:13

frederoni force-pushed the 3979-gzip-telemetry-events branch from 9c7e25e to 11b5be0 Compare June 29, 2016 16:51

1ec5 reviewed Jun 29, 2016
View reviewed changes

frederoni force-pushed the 3979-gzip-telemetry-events branch from 11b5be0 to 5b41dbf Compare June 29, 2016 17:41

1ec5 reviewed Jun 29, 2016
View reviewed changes

platform/darwin/src/NSData+MGLAdditions.h

@end

NS_ASSUME_NONNULL_END

Copy link

Contributor

1ec5 Jun 29, 2016 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: missing newline at the end.

boundsj reviewed Jun 29, 2016
View reviewed changes

bleege mentioned this pull request Jul 1, 2016

GZIP Telemetry Events In Android #5536

Closed

1ec5 added this to the ios-v3.3.0 milestone Jul 3, 2016

frederoni changed the title ~~[ios] #3979 wip gzip telemetry events~~ [ios] #3979 compress telemetry events Jul 4, 2016

frederoni added 4 commits July 4, 2016 13:53

[ios] #3979 wip gzip telemetry events

5091843

[ios] expose compression from core instead of adding a new dependency

2fefa7c

[ios] #3979 Exposed decompress from mbgl and added a test case

b80f45e

[ios] #3979 cleanup

007ff02

frederoni added 3 commits July 4, 2016 13:53

[ios] #3979 cleaned up test case

d14bdb8

[ios] #3979 cleanup

6cb4afe

[ios] #3979 gzip -> deflate

67537a4

frederoni removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Jul 4, 2016

frederoni force-pushed the 3979-gzip-telemetry-events branch from 4a00f84 to 67537a4 Compare July 4, 2016 14:58

frederoni merged commit 5464694 into release-ios-v3.3.0 Jul 5, 2016

frederoni deleted the 3979-gzip-telemetry-events branch July 5, 2016 20:46

friedbunny mentioned this pull request Jul 5, 2016

gzip telemetry events #3979

Closed

1ec5 mentioned this pull request Mar 19, 2019

Delete unused NSData category #14148

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ios] #3979 compress telemetry events #5490

[ios] #3979 compress telemetry events #5490

frederoni commented Jun 27, 2016 •

edited

Loading

friedbunny commented Jun 27, 2016 •

edited

Loading

1ec5 commented Jun 27, 2016 •

edited

Loading

frederoni commented Jun 28, 2016 •

edited

Loading

frederoni commented Jun 29, 2016

1ec5 Jun 29, 2016

1ec5 Jun 29, 2016 •

edited

Loading

1ec5 commented Jun 29, 2016

boundsj Jun 29, 2016

friedbunny Jun 29, 2016

frederoni Jun 29, 2016 •

edited

Loading

1ec5 Jun 29, 2016

1ec5 commented Jun 30, 2016

kkaefer commented Jul 4, 2016

frederoni commented Jul 4, 2016 •

edited

Loading

kkaefer commented Jul 4, 2016

[ios] #3979 compress telemetry events #5490

[ios] #3979 compress telemetry events #5490

Conversation

frederoni commented Jun 27, 2016 • edited Loading

WiP

Things to consider:

friedbunny commented Jun 27, 2016 • edited Loading

1ec5 commented Jun 27, 2016 • edited Loading

frederoni commented Jun 28, 2016 • edited Loading

frederoni commented Jun 29, 2016

1ec5 Jun 29, 2016

Choose a reason for hiding this comment

1ec5 Jun 29, 2016 • edited Loading

Choose a reason for hiding this comment

1ec5 commented Jun 29, 2016

boundsj Jun 29, 2016

Choose a reason for hiding this comment

friedbunny Jun 29, 2016

Choose a reason for hiding this comment

frederoni Jun 29, 2016 • edited Loading

Choose a reason for hiding this comment

1ec5 Jun 29, 2016

Choose a reason for hiding this comment

1ec5 commented Jun 30, 2016

kkaefer commented Jul 4, 2016

frederoni commented Jul 4, 2016 • edited Loading

kkaefer commented Jul 4, 2016

frederoni commented Jun 27, 2016 •

edited

Loading

friedbunny commented Jun 27, 2016 •

edited

Loading

1ec5 commented Jun 27, 2016 •

edited

Loading

frederoni commented Jun 28, 2016 •

edited

Loading

1ec5 Jun 29, 2016 •

edited

Loading

frederoni Jun 29, 2016 •

edited

Loading

frederoni commented Jul 4, 2016 •

edited

Loading