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

Add socket profiling to Network page #2191

Merged
merged 15 commits into from
Jul 29, 2020

Conversation

kenzieschmoll
Copy link
Member

This CL

  • adds socket information to the Network profiler
  • adds an Overview UI that displays general info for both HTTP and WS requests
  • removes the "Timing" tab in the network request inspector, as that information is now in the "Overview" tab

Table with both HTTP and WS requests and overview for selected HTTP request:
Screen Shot 2020-07-17 at 3 19 06 PM
Overview for socket:
Screen Shot 2020-07-17 at 1 52 26 PM

TODO before submitting: add tests and general cleanup

@kenzieschmoll kenzieschmoll marked this pull request as ready for review July 23, 2020 18:32
@kenzieschmoll kenzieschmoll requested a review from jacob314 July 23, 2020 18:32
static const _contentTypeKey = 'content-type';
static const _cookieKey = 'cookie';
static const _errorKey = 'error';
static const _filterKey = 'filterKey';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: normalize the key names. This one has the word Key in the value but others don't.

Copy link
Member Author

Choose a reason for hiding this comment

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

filterKey is actually the name of the key in the arguments map. This would need to be changed on the VM but I think it is actually an accurate description as is because it is describing a key to group events on.

static const _requestHeadersKey = 'requestHeaders';
static const _responseHeadersKey = 'responseHeaders';
static const _statusCodeKey = 'statusCode';
static const _setCookieKey = 'set-cookie';
Copy link
Contributor

Choose a reason for hiding this comment

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

make this key camelCase?

Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps add a TODO and file a bug upstream on the VM to cleanup the names.

Copy link
Member Author

Choose a reason for hiding this comment

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

added a TODO and filed a bug. Names need to be changed upstream

@@ -126,46 +131,68 @@ class NetworkController {
invalidRequests.add(requestData);
}
}
return HttpRequests(

currentValues.removeWhere((value) => value is WebSocket);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: rather than removing all WebSocket, maybe just don't add them in the first place. it looks like we are adding them a couple lines earlier.

Copy link
Member Author

Choose a reason for hiding this comment

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

At this point, currentValues contains all the current requests we have in the profiler, which will contain ws requests if they exist. When we refresh the network data after polling, the new data may contain web sockets with the same ids as ones we already have. For this reason, we need to remove the current web sockets and replace with the updated data in the profile. We could be smarter about this and keep unchanged sockets in the list if we need to.

Copy link
Contributor

Choose a reason for hiding this comment

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

sg. can you add a comment to that effect?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@jacob314 jacob314 left a comment

Choose a reason for hiding this comment

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

lgtm

@kenzieschmoll
Copy link
Member Author

Addresses part of #2044

@kenzieschmoll kenzieschmoll merged commit 0c7aa51 into flutter:master Jul 29, 2020
@kenzieschmoll kenzieschmoll deleted the sockets branch July 29, 2020 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants