Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature - Task Metrics #1492

Merged
merged 1 commit into from Sep 9, 2016
Merged

Feature - Task Metrics #1492

merged 1 commit into from Sep 9, 2016

Conversation

cnoon
Copy link
Member

@cnoon cnoon commented Sep 9, 2016

This PR adds support for the awesome new URLSessionTaskMetric statistical tracking. This is essentially what we always wanted the Timeline to be, but we didn't have access to the same internal APIs to compute all these data points. Thanks Apple!

For the time being, it makes sense to keep the Timeline around for watchOS and earlier versions of the platforms that don't support the new APIs.

This shouldn't be a difficult feature to build, but it most definitely was due to the complexity of compiling against 4 platforms with only three of the latest actually supporting new APIs. Therefore, I had to get pretty creative with the new Response protocol and extension in combination with AnyObject abstraction while moving the metrics through the various objects.

Overall, I'm VERY happy with how this turned out. I wish it wasn't so complicated to support so many platforms, but unfortunately, it is. I abstracted the complexity away into the single Response protocol and implementation, so that certainly helps isolate all the platform complexity to a single area.

@cnoon cnoon added this to the 4.0.0 milestone Sep 9, 2016
@@ -154,6 +183,8 @@ public struct DownloadResponse<Value> {
/// The timeline of the complete lifecycle of the request.
public let timeline: Timeline

var _metrics: AnyObject?
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this AnyObject and not URLSessionTaskMetrics?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just so the internal variable didn't need be behind an availability check?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep...you can't store a URLSessionTaskMetrics as a stored property b/c it's not available on the older OS's. This is the only valid workaround I've found to the problem yet. It's nasty, but it does work.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can't put the whole stored property behind an availability check?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope.

@jshier
Copy link
Contributor

jshier commented Sep 9, 2016

Looks good.

@cnoon
Copy link
Member Author

cnoon commented Sep 9, 2016

Awesome, thanks @jshier. I'm going to go ahead and move this through.

@cnoon cnoon merged commit 5b9f8f6 into master Sep 9, 2016
@cnoon cnoon deleted the feature/task-metrics branch September 9, 2016 20:08
@cnoon cnoon removed the in progress label Sep 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants