flutter
Offline support for Network page
Adding offline support to Network page.
issue link - https://github.com/flutter/devtools/issues/4470
List which issues are fixed by this PR.
Please add a note to packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md if your change requires release notes. Otherwise, add the 'release-notes-not-required' label to the PR.
Pre-launch Checklist
- [ ] I read the Contributor Guide and followed the process outlined there for submitting PRs.
- [ ] I read the Tree Hygiene wiki page, which explains my responsibilities.
- [ ] I read the Flutter Style Guide recently, and have followed its advice.
- [ ] I signed the CLA.
- [ ] I listed at least one issue that this PR fixes in the description above.
- [ ] I updated/added relevant documentation (doc comments with
///). - [ ] I added new tests to check the change I am making, or there is a reason for not adding tests.
If you need help, consider asking for help on Discord.
@hrajwade96 is this PR ready for review or is this still a work in progress?
@hrajwade96 is this PR ready for review or is this still a work in progress?
@kenzieschmoll just finishing up few things, I will mark it ready soon
Looks like there are several failing checks. Please also add an entry to NEXT_RELEASE_NOTES.md so that the release notes check passes.
You will likely have several merge conflicts after pulling in the tip of the master branch. A change just landed that upgraded the Flutter version used in DevTools, which updated to a new version of the dart formatter. Here's what I recommend to make this merge simpler for you:
- Merge the master branch into your branch.
- For all conflicts, accept your revision of the file..
- Run
dt update-flutter-sdk. This will update the flutter sdk contained attool/flutter-sdkto the version used on the DevTools CI. - Run
tool/flutter-sdk/bin/dart formatfrom thedevtools_appdirectory. This will format your files with your changes to use the new formatter.
You will likely have several merge conflicts after pulling in the tip of the master branch. A change just landed that upgraded the Flutter version used in DevTools, which updated to a new version of the dart formatter. Here's what I recommend to make this merge simpler for you:
- Merge the master branch into your branch.
- For all conflicts, accept your revision of the file..
- Run
dt update-flutter-sdk. This will update the flutter sdk contained attool/flutter-sdkto the version used on the DevTools CI.- Run
tool/flutter-sdk/bin/dart formatfrom thedevtools_appdirectory. This will format your files with your changes to use the new formatter.
@kenzieschmoll can you run the checks again, I have done the above steps, formatted the code and updated the release notes as well.
You will likely have several merge conflicts after pulling in the tip of the master branch. A change just landed that upgraded the Flutter version used in DevTools, which updated to a new version of the dart formatter. Here's what I recommend to make this merge simpler for you:
- Merge the master branch into your branch.
- For all conflicts, accept your revision of the file..
- Run
dt update-flutter-sdk. This will update the flutter sdk contained attool/flutter-sdkto the version used on the DevTools CI.- Run
tool/flutter-sdk/bin/dart formatfrom thedevtools_appdirectory. This will format your files with your changes to use the new formatter.@kenzieschmoll can you run the checks again, I have done the above steps, formatted the code and updated the release notes as well.
I pushed a minor change that I had missed pushing earlier which was the primary cause of the checks failing. @kenzieschmoll can you re-run
Looks like you still have some failing tests. Based on the errors it looks like you may need to set a global in the test setup for the failing tests: setGlobal(OfflineDataController, OfflineDataController());
You can verify the tests pass locally by running flutter test test/screens/network/
Getting closer. Another failing test: devtools/packages/devtools_app/test/shared/framework/visible_screens_test.dart.
Consider running all tests locally flutter test test/ to ensure tests are passing.
Getting closer. Another failing test:
devtools/packages/devtools_app/test/shared/framework/visible_screens_test.dart.Consider running all tests locally
flutter test test/to ensure tests are passing.
Yes I've fixed this one, I ran locally now there's just one test (below one) failing locally but mostly it's timezone bound, hence although it's failing locally, it should pass here.
test('TimestampColumn', () {
final column = TimestampColumn();
final getRequest = findRequestById('1');
// The hours field may be unreliable since it depends on the timezone the
// test is running in.
expect(column.getDisplayValue(getRequest), contains(':45:26.279'));
});
A couple things look like they are still missing from this PR. I checked the code out locally and noticed that we do not have a way to export the offline network data. This is because the OpenSaveButtonGroup was not added to _NetworkProfilerControlsState. Please apply this patch to the _NetworkProfilerControlsState.build method:
return Row(
children: [
if (!widget.offline) ...[
StartStopRecordingButton(
recording: _recording,
onPressed:
() async => await widget.controller.togglePolling(!_recording),
tooltipOverride:
_recording
? 'Stop recording network traffic'
: 'Resume recording network traffic',
minScreenWidthForTextBeforeScaling: double.infinity,
gaScreen: gac.network,
gaSelection: _recording ? gac.pause : gac.resume,
),
const SizedBox(width: denseSpacing),
ClearButton(
minScreenWidthForTextBeforeScaling:
_NetworkProfilerControls._includeTextWidth,
gaScreen: gac.network,
gaSelection: gac.clear,
onPressed: widget.controller.clear,
),
const SizedBox(width: denseSpacing),
DownloadButton(
tooltip: 'Download as .har file',
minScreenWidthForTextBeforeScaling:
_NetworkProfilerControls._includeTextWidth,
onPressed: widget.controller.exportAsHarFile,
gaScreen: gac.network,
gaSelection: gac.NetworkEvent.downloadAsHar.name,
),
const SizedBox(width: denseSpacing),
// TODO(kenz): fix focus issue when state is refreshed
Expanded(
child: SearchField<NetworkController>(
searchController: widget.controller,
searchFieldEnabled: hasRequests,
searchFieldWidth:
screenWidth <= MediaSize.xs
? defaultSearchFieldWidth
: wideSearchFieldWidth,
),
),
const SizedBox(width: denseSpacing),
Expanded(
child: StandaloneFilterField<NetworkRequest>(
controller: widget.controller,
filteredItem: 'request',
),
),
const SizedBox(width: denseSpacing),
OpenSaveButtonGroup(
screenId: ScreenMetaData.performance.id,
onSave: widget.controller.exportData,
),
],
],
);
Once I added these buttons and then attempted to export network data, I am seeing an exception:
══╡ EXCEPTION CAUGHT BY GESTURE ╞═══════════════════════════════════════════════════════════════════
The following HttpProfileRequestError was thrown while handling a gesture:
SocketException: Connection refused (OS Error: Connection refused, errno = 61), address = 127.0.0.1,
port = 53461.
When the exception was thrown, this was the stack:
dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 307:3 throw_
errors.dart:307
packages/vm_service/src/dart_io_extensions.dart 565:7 [_returnIfNoError]
dart_io_extensions.dart:565
packages/vm_service/src/dart_io_extensions.dart 542:40 get headers
dart_io_extensions.dart:542
packages/devtools_app/src/shared/http/http_request_data.dart 387:41 HttpProfileRequestDataExtension.toJson
http_request_data.dart:387
packages/devtools_app/src/shared/http/http_request_data.dart 375:50 HttpProfileRequestExtension.toJson
http_request_data.dart:375
packages/devtools_app/src/shared/http/http_request_data.dart 81:44 toJson
http_request_data.dart:81
packages/devtools_app/src/screens/network/offline_network_data.dart 87:39 <fn>
offline_network_data.dart:87
dart-sdk/lib/internal/iterable.dart 442:31 elementAt
iterable.dart:442
dart-sdk/lib/internal/iterable.dart 371:26 moveNext
iterable.dart:371
dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart 1127:20 next
operations.dart:1127
dart-sdk/lib/_internal/js_dev_runtime/patch/core_patch.dart 358:14 of
core_patch.dart:358
dart-sdk/lib/internal/iterable.dart 224:7 toList
iterable.dart:224
packages/devtools_app/src/screens/network/offline_network_data.dart 87:39 toJson
offline_network_data.dart:87
packages/devtools_app/src/screens/network/network_controller.dart 470:24 prepareOfflineScreenData
network_controller.dart:470
packages/devtools_app/src/shared/offline/offline_data.dart 188:34 exportData
offline_data.dart:188
packages/devtools_app/src/shared/ui/file_import.dart 63:21 <fn>
file_import.dart:63
packages/flutter/src/material/ink_well.dart 1185:21 handleTap
A couple things look like they are still missing from this PR. I checked the code out locally and noticed that we do not have a way to export the offline network data. This is because the
OpenSaveButtonGroupwas not added to_NetworkProfilerControlsState. Please apply this patch to the_NetworkProfilerControlsState.buildmethod:return Row( children: [ if (!widget.offline) ...[ StartStopRecordingButton( recording: _recording, onPressed: () async => await widget.controller.togglePolling(!_recording), tooltipOverride: _recording ? 'Stop recording network traffic' : 'Resume recording network traffic', minScreenWidthForTextBeforeScaling: double.infinity, gaScreen: gac.network, gaSelection: _recording ? gac.pause : gac.resume, ), const SizedBox(width: denseSpacing), ClearButton( minScreenWidthForTextBeforeScaling: _NetworkProfilerControls._includeTextWidth, gaScreen: gac.network, gaSelection: gac.clear, onPressed: widget.controller.clear, ), const SizedBox(width: denseSpacing), DownloadButton( tooltip: 'Download as .har file', minScreenWidthForTextBeforeScaling: _NetworkProfilerControls._includeTextWidth, onPressed: widget.controller.exportAsHarFile, gaScreen: gac.network, gaSelection: gac.NetworkEvent.downloadAsHar.name, ), const SizedBox(width: denseSpacing), // TODO(kenz): fix focus issue when state is refreshed Expanded( child: SearchField<NetworkController>( searchController: widget.controller, searchFieldEnabled: hasRequests, searchFieldWidth: screenWidth <= MediaSize.xs ? defaultSearchFieldWidth : wideSearchFieldWidth, ), ), const SizedBox(width: denseSpacing), Expanded( child: StandaloneFilterField<NetworkRequest>( controller: widget.controller, filteredItem: 'request', ), ), const SizedBox(width: denseSpacing), OpenSaveButtonGroup( screenId: ScreenMetaData.performance.id, onSave: widget.controller.exportData, ), ], ], );Once I added these buttons and then attempted to export network data, I am seeing an exception:
══╡ EXCEPTION CAUGHT BY GESTURE ╞═══════════════════════════════════════════════════════════════════ The following HttpProfileRequestError was thrown while handling a gesture: SocketException: Connection refused (OS Error: Connection refused, errno = 61), address = 127.0.0.1, port = 53461. When the exception was thrown, this was the stack: dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 307:3 throw_ errors.dart:307 packages/vm_service/src/dart_io_extensions.dart 565:7 [_returnIfNoError] dart_io_extensions.dart:565 packages/vm_service/src/dart_io_extensions.dart 542:40 get headers dart_io_extensions.dart:542 packages/devtools_app/src/shared/http/http_request_data.dart 387:41 HttpProfileRequestDataExtension.toJson http_request_data.dart:387 packages/devtools_app/src/shared/http/http_request_data.dart 375:50 HttpProfileRequestExtension.toJson http_request_data.dart:375 packages/devtools_app/src/shared/http/http_request_data.dart 81:44 toJson http_request_data.dart:81 packages/devtools_app/src/screens/network/offline_network_data.dart 87:39 <fn> offline_network_data.dart:87 dart-sdk/lib/internal/iterable.dart 442:31 elementAt iterable.dart:442 dart-sdk/lib/internal/iterable.dart 371:26 moveNext iterable.dart:371 dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart 1127:20 next operations.dart:1127 dart-sdk/lib/_internal/js_dev_runtime/patch/core_patch.dart 358:14 of core_patch.dart:358 dart-sdk/lib/internal/iterable.dart 224:7 toList iterable.dart:224 packages/devtools_app/src/screens/network/offline_network_data.dart 87:39 toJson offline_network_data.dart:87 packages/devtools_app/src/screens/network/network_controller.dart 470:24 prepareOfflineScreenData network_controller.dart:470 packages/devtools_app/src/shared/offline/offline_data.dart 188:34 exportData offline_data.dart:188 packages/devtools_app/src/shared/ui/file_import.dart 63:21 <fn> file_import.dart:63 packages/flutter/src/material/ink_well.dart 1185:21 handleTap
@kenzieschmoll, I wanted to clarify my understanding based on your comment:
- This PR focuses solely on viewing Network data in offline mode. Importing offline data back into DevTools is planned in the upcoming PR, as we discussed earlier (If you suggest to add it as part of this PR itself we can add it here).
- Exporting HAR files already has a dedicated Download CTA, so I assumed adding 'OpenSaveButtonGroup' wasn't necessary at this stage.
- Additionally, as previously discussed, export functionality is not supported while in offline mode. Let me know if there are any changes to this or we can go ahead with this
This PR focuses solely on viewing Network data in offline mode
By this do you mean viewing data when the Network screen is showing, and then DevTools loses connection to the running app? In other words, the "review history" feature?
Importing offline data back into DevTools is planned in the upcoming PR
Okay this sounds good. Can we turn the feature flag to off then until this support is added?
How much work is left to do to support exporting the data as JSON? I would expect there is a small bit of serialization work left to ensure the data can be serialized and deserialized to JSON, but the majority of the logic is contained in this PR. Is that accurate?
(edited) UPDATE:
I have a follow on I'd like to submit after this PR. This will essentially pave the way for you to implement the full offline support for the network screen (which is exporting the data as json and being able to re-import it as json). This follow on PR I have put to gather will combine the download buttons to let the user choose which format they'd like to download data in.
This would be behind a new experiment flag networkSaveLoad so that the network disconnect experience can still be enabled while the full offline support is under development.
I dug a little deeper on the errors I am encountering. These exceptions occur even for the "review history" behavior. Have you tested with requests that have an error? Some of the fields on HttpProfileRequestData (from the vm_service package) will throw an exception if the request has an error (hasError). We need to make sure that the serialization logic gracefully handles all of these cases.
We should also have test coverage for this case.
I dug a little deeper on the errors I am encountering. These exceptions occur even for the "review history" behavior. Have you tested with requests that have an error? Some of the fields on
HttpProfileRequestData(from the vm_service package) will throw an exception if the request has an error (hasError). We need to make sure that the serialization logic gracefully handles all of these cases.We should also have test coverage for this case.
Ok will check this case and also add test coverage for this
By this do you mean viewing data when the Network screen is showing, and then DevTools loses connection to the running app? In other words, the "review history" feature?
Yes I meant the "review history" feature.
Okay this sounds good. Can we turn the feature flag to off then until this support is added?
Sure, so as per your next comment I think you are suggesting to enable the review history (with this PR) and add another flag which gives two options to choose the format for export, and also an import button (in the next PR).
(edited) UPDATE: I have a follow on I'd like to submit after this PR. This will essentially pave the way for you to implement the full offline support for the network screen (which is exporting the data as json and being able to re-import it as json). This follow on PR I have put to gather will combine the download buttons to let the user choose which format they'd like to download data in.
This would be behind a new experiment flag
networkSaveLoadso that the network disconnect experience can still be enabled while the full offline support is under development.
Sure, I'll pick this up in the next PR. Could you elaborate on the key idea behind adding the option to export it as a '.json' file?
Sure, so as per your next comment I think you are suggesting to enable the review history (with this PR) and add another flag which gives two options to choose the format for export, and also an import button (in the next PR).
Yes you are correct sorry for the confusion. I meant to update the comment about the flag with my edit. For the next PR (adding the buttons behind a flag), I will make that change since I already have a PR prepared. I'll leave the flag off, and then you can make your change to support full offline support for the network page, which requires exporting the data to a JSON file so that it can be reloaded into DevTools in the existing offline mechanism. The majority of the work for that is done in this PR, but the final piece is supporting the full serialization / deserialization to / from JSON.
Could you elaborate on the key idea behind adding the option to export it as a '.json' file?
This is a requirement of adding full offline support for the Network page (where you can export a file and load it back into DevTools later). The offline framework in DevTools serializes to json and loads files from json.
Yes you are correct sorry for the confusion. I meant to update the comment about the flag with my edit. For the next PR (adding the buttons behind a flag), I will make that change since I already have a PR prepared. I'll leave the flag off, and then you can make your change to support full offline support for the network page, which requires exporting the data to a JSON file so that it can be reloaded into DevTools in the existing offline mechanism. The majority of the work for that is done in this PR, but the final piece is supporting the full serialization / deserialization to / from JSON.
Oh, okay, no problem. Thanks! I had the idea before, but this made it much clearer.
This is a requirement of adding full offline support for the Network page (where you can export a file and load it back into DevTools later). The offline framework in DevTools serializes to json and loads files from json.
I thought we are going to add support to re-import the .har file which we can download (like https://github.com/flutter/devtools/issues/4470#issuecomment-2297037176). Is it like har and json both formats will be supported for importing? Or now are you thinking to add support for only json for importing back.
I thought we are going to add support to re-import the .har file which we can download (like https://github.com/flutter/devtools/issues/4470#issuecomment-2297037176). Is it like har and json both formats will be supported for importing? Or now are you thinking to add support for only json for importing back.
From the work in this PR, adding support for re-importing as JSON seems like it is 90% there. Is that correct? If I understand correctly there is not much work left to support the full JSON serialization / deserialization. If this is almost complete, then I think we should push this across the finish line. This is also good for consistency with how offline data is supported for other DevTools screens.
For deserializing the .har files, this work has not started yet, right? At this point, it seems like this would take more work to support, and if the JSON serialization / deserialization is robust and well-tested, then it may not make sense to support importing both types of files.
Bringing my comment from #4470 to this thread for reference:
The more complicated part for the Network page is the fact that we support exporting as a .har file, whereas the offline framework for DevTools only supports importing & exporting to / from a JSON file. So in the current form of the offline framework, if network data is exported as a .har file, it will not be able to be re-imported back into DevTools. Only data exported as a JSON file through the DevTools offline framework can be re-imported back into DevTools.
We can tweak the logic to support loading the .har file for the network page as a special case, but we can do this as a follow up task to adding the full offline support for the Network page.
To re-state in the context of this PR, we could consider adding support for importing the .har file to DevTools as a follow up. However, we may be limited if the .har format doesn't allow us to add metadata that we would have in the JSON format (things like filters or other data specific to the DevTools network profiler), so it may not be worth doing.
As a user of this tool, I see the user journey as such:
- If I want to export network data for viewing later in Flutter DevTools, then I should download as a JSON file.
- If I want to export network data for viewing in some other tool that supports the .har format, then I should download the .har file.
From the work in this PR, adding support for re-importing as JSON seems like it is 90% there. Is that correct? If I understand correctly there is not much work left to support the full JSON serialization / deserialization. If this is almost complete, then I think we should push this across the finish line. This is also good for consistency with how offline data is supported for other DevTools screens.
Yes correct, I added the remaining piece (disconnected network body) in this PR itself , now we don't need another PR for this. (note: I haven't added OpenSaveButtonGroup for import/export cta since you would be raising a PR with new UI for that, also btw we need to add a call to_fetchFullDataBeforeExport before the json is downloaded just like in case of har).
For deserializing the .har files, this work has not started yet, right? At this point, it seems like this would take more work to support, and if the JSON serialization / deserialization is robust and well-tested, then it may not make sense to support importing both types of files. Yes not yet started,
Bringing my comment from #4470 to this thread for reference:
The more complicated part for the Network page is the fact that we support exporting as a .har file, whereas the offline framework for DevTools only supports importing & exporting to / from a JSON file. So in the current form of the offline framework, if network data is exported as a .har file, it will not be able to be re-imported back into DevTools. Only data exported as a JSON file through the DevTools offline framework can be re-imported back into DevTools. We can tweak the logic to support loading the .har file for the network page as a special case, but we can do this as a follow up task to adding the full offline support for the Network page.
To re-state in the context of this PR, we could consider adding support for importing the .har file to DevTools as a follow up. However, we may be limited if the .har format doesn't allow us to add metadata that we would have in the JSON format (things like filters or other data specific to the DevTools network profiler), so it may not be worth doing.
Ahh ok, at that time I interpreted this comment a little differently regarding the follow-up PR on re-importing .har
Hmm, After completing this PR, I'll analyze the approach to add this support and get back to you. Once I've done that, we can discuss whether it's worth pursuing. Does that work?
As a user of this tool, I see the user journey as such:
- If I want to export network data for viewing later in Flutter DevTools, then I should download as a JSON file.
- If I want to export network data for viewing in some other tool that supports the .har format, then I should download the .har file.
Hmm, yes got it, thanks
Can you merge with the master branch? The benchmark size threshold was increased recently.
Looks like a legitimate error in the performance benchmarks:
benchmark/devtools_benchmarks_test.dart 43:15 main.<fn>
Caught browser-side error: Null check operator used on a null value
at module0.NetworkController._recordingNetworkTraffic inner (http://localhost:9999/main.dart.wasm:wasm-function[19792]:0x3c6bf2)
at module0.NetworkController._recordingNetworkTraffic (http://localhost:9999/main.dart.wasm:wasm-function[19647]:0x3c295e)
at module0.NetworkController.startRecording inner (http://localhost:9999/main.dart.wasm:wasm-function[19645]:0x3c27d3)
at module0.NetworkController._initHelper inner (http://localhost:9999/main.dart.wasm:wasm-function[19637]:0x3c2665)
at module0.defaultScreens closure at file:///Users/runner/work/devtools/devtools/packages/devtools_app/lib/src/app.dart:662:25 (http://localhost:9999/main.dart.wasm:wasm-function[18664]:0x3a6bf1)
at module0.closure wrapper at file:///Users/runner/work/devtools/devtools/packages/devtools_app/lib/src/app.dart:662:25 trampoline (http://localhost:9999/main.dart.wasm:wasm-function[18683]:0x3a746e)
at module0.DevToolsScreen.controllerProvider closure at file:///Users/runner/work/devtools/devtools/packages/devtools_app/lib/src/app.dart:544:15 (http://localhost:9999/main.dart.wasm:wasm-function[26423]:0x45eebc)
at module0._CreateInheritedProviderState.value (http://localhost:9999/main.dart.wasm:wasm-function[26414]:0x45ec90)
dart:async _Completer.completeError
package:web_benchmarks/src/runner.dart 254:25 BenchmarkServer.run.<fn>
Looks like a legitimate error in the performance benchmarks:
Sorry for late response got occupied with something. I think it was trying to record as offlineDataController.showingOfflineData.value was coming as false, I have pushed one check for this. Might need to add 2 more checks at other places checking it locally will push it if working.
Looks like a legitimate error in the performance benchmarks:
Sorry for late response got occupied with something. I think it was trying to record as offlineDataController.showingOfflineData.value was coming as false, I have pushed one check for this. Might need to add 2 more checks at other places checking it locally will push it if working.
I have added the necessary handling wherever service connection was being used. Please run the checks once.