Closed Bug 1434333 Opened 7 years ago Closed 7 years ago

Import an existing HAR file

Categories

(DevTools :: Netmonitor, enhancement, P2)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: Honza, Assigned: Honza)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1434333 - Import an existing HAR file;
59 bytes, text/x-review-board-request
ochameau
: review+
Details 
It should be possible to import an existing *.har file into the Network monitor panel. I can be initially done by dragging and dropping har file onto the Net panel.

Later when we work on Bug 1403530 - we might use the new button for export & import.

Honza
Assignee: nobody → odvarko
Blocks: netmonitor-har
Severity: normal → enhancement
Priority: -- → P2
Comment on attachment 8948417 [details]
Bug 1434333 - Import an existing HAR file;

https://reviewboard.mozilla.org/r/217878/#review223632


Static analysis found 2 defects in this patch.
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/netmonitor/src/components/App.js:11
(Diff revision 1)
>  
> -const { createFactory } = require("devtools/client/shared/vendor/react");
> +const { Component, createFactory } = require("devtools/client/shared/vendor/react");
>  const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
>  const dom = require("devtools/client/shared/vendor/react-dom-factories");
>  const { connect } = require("devtools/client/shared/vendor/react-redux");
> +const Actions = require("../actions/index");

Error: 'Actions' is assigned a value but never used. [eslint: no-unused-vars]

::: devtools/client/netmonitor/src/components/App.js:139
(Diff revision 1)
> +      fileReader.onloadend = () => {
> +        callback(fileReader.result);
> -};
> +      };
> +      fileReader.readAsText(file);
> +    }
> +  }

Error: Missing semicolon. [eslint: semi]
@Dragana: can you remind me why bug 1384679 introduced `timing offsets`?

HAR spec [1] (the spec for data collected by network panel) is defining 'timings', but there are no offsets. Why we had to add the set of offsets and send it from the backend instead of doing the calculation on the client side? The `timings` HAR section should be enough to get all timing data we need ...


Honza

[1] http://www.softwareishard.com/blog/har-12-spec/#timings
Flags: needinfo?(dd.mozilla)
Just to explain the context a bit more. I am exporting all the data from Net panel (as HAR) and importing it again (import is being implemented in this bug). But, since 'offsets' are not part of the HAR spec I am getting wrong timing info after import...

Honza
Patch from bug 1311171 is needed since there is a small collision
(but it doesn't depend on the functionality).

Honza
Comment on attachment 8948417 [details]
Bug 1434333 - Import an existing HAR file;

https://reviewboard.mozilla.org/r/217878/#review224184


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/netmonitor/src/components/App.js:162
(Diff revision 2)
> -  // True if the stats panel is opened.
> +        callback(fileReader.result);
> -  statisticsOpen: PropTypes.bool.isRequired,
> -};
> +      };
> +      fileReader.readAsText(file);
> +    }
> +  }

Error: Missing semicolon. [eslint: semi]
Comment on attachment 8948417 [details]
Bug 1434333 - Import an existing HAR file;

https://reviewboard.mozilla.org/r/217878/#review225502

Sorry for the delay in the review.

It works great overall, but another review would be helpful I think.

* If I record HAR with cached request and then import it, I get this exception when selecting the cached requests and opening their response sidebar:
ResponsePanel.js, line 145: TypeError: mimeType is undefined
Also their "transfered side" is -1B, may be we could assume this is cached request when it is -1?
It could be worth looking at what other browsers do.

::: devtools/client/netmonitor/src/components/App.js:98
(Diff revision 3)
> +        if (har) {
> +          this.appendPreview(har);
> +        }
> +      });
> +      reader();
> +    }

In HarImporter.doImport, you do:
   this.actions.clearRequests();
So I'm not sure there is any value in importing more than one HAR file at a time?

::: devtools/client/netmonitor/src/components/App.js:111
(Diff revision 3)
> +    try {
> +      let importer = new HarImporter(this.props.actions);
> +      importer.import(har);
> +    } catch (err) {
> +      if (openSplitConsole) {
> +        openSplitConsole(err);

This is surprising, as you won't see any error if you don't have the split console opened. (I rarely have it opened outside of debugger)

Shouldn't we use notification bar?
If the error is one or two lines, that's a common way to report errors in toolbox.
Like here in inspector:
https://searchfox.org/mozilla-central/source/devtools/client/inspector/inspector.js#239-246
webconsole:
https://searchfox.org/mozilla-central/source/devtools/client/webconsole/utils.js#257-265
storage:
https://searchfox.org/mozilla-central/source/devtools/client/storage/ui.js#1268
styleeditor:
https://searchfox.org/mozilla-central/source/devtools/client/styleeditor/styleeditor-panel.js#87

::: devtools/client/netmonitor/src/components/App.js:145
(Diff revision 3)
> -      })
> +        }),
> +        div({className: "dropHarFiles"},
> +          div({}, DROP_HAR_FILES)
> +        )
> -    )
> +      )
> -  );
> +    );

When dropping over the StatisticsPanel, it looks broken. The pie chart and text all reset to gray/"loading", but never updates.
Shouldn't you put all this drag'n drop/har importer code inside MonitorPanel?

Also, wouldn't it be better to have a dedicated component, similar to VisibilityHandler, which would only implement this?

::: devtools/client/netmonitor/src/constants.js:321
(Diff revision 3)
> +  "connect",
> +  "ssl",
> +  "send",
> +  "wait",
> +  "receive",
> +];

Do not hesitate to submit multiples patches to mozreview. This change could have been done in a dedicated changeset and help reviewing this particular change only once.

::: devtools/client/netmonitor/src/har/har-builder.js:115
(Diff revision 3)
>  
>      entry.request = await this.buildRequest(file);
>      entry.response = await this.buildResponse(file);
>      entry.cache = this.buildCache(file);
>      entry.timings = eventTimings ? eventTimings.timings : {};
> +    entry._securityState = file.securityState;

Why do you prefix this with `_`?
Could you test this new field in existing har tests?
It is not clear what should be this attribute.

::: devtools/client/netmonitor/src/har/har-builder.js:219
(Diff revision 3)
>    buildCookies: function (input) {
>      if (!input) {
>        return [];
>      }
>  
> -    return this.buildNameValuePairs(input.cookies);
> +    return this.buildNameValuePairs(input.cookies || input);

This is really surprising, isn't that completely broken?
I mean, is `input.cookies` ever defined?
It would be great to have this tested.

::: devtools/client/netmonitor/src/har/har-importer.js:31
(Diff revision 3)
> +    this.doImport(json);
> +  },
> +
> +  doImport: function (har) {
> +    this.actions.clearRequests();
> +

Should you warn when har.log.version < 1.2?

::: devtools/client/netmonitor/src/har/har-importer.js:127
(Diff revision 3)
> +        time: startedMillis + onContentLoad,
> +      });
> +      this.actions.addTimingMarker({
> +        name: "dom-complete",
> +        time: startedMillis + onLoad,
> +      });

Should you call addTimingMarker when pageTimings.onContentLoad / pageTimings.onLoad are equal to 0/-1?

::: devtools/client/netmonitor/src/har/test/browser_net_har_import.js:11
(Diff revision 3)
> +/**
> + * Tests for importing HAR data.
> + */
> +add_task(function* () {
> +  let { tab, monitor } = yield initNetMonitor(
> +    HAR_EXAMPLE_URL + "html_har_post-data-test-page.html");

It would be great to complexify this test page or fork it to cover more usecases:
* cookies
* headers
* cached requests
Attachment #8948417 - Flags: review?(poirot.alex)
Comment on attachment 8948417 [details]
Bug 1434333 - Import an existing HAR file;

https://reviewboard.mozilla.org/r/217878/#review226796


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/netmonitor/src/selectors/timing-markers.js:10
(Diff revision 4)
>  "use strict";
>  
>  function getDisplayedTimingMarker(state, marker) {
> -  return state.timingMarkers[marker] - state.requests.firstStartedMillis;
> +  let value = state.timingMarkers[marker];
> +  if (value == -1) {
> +    return value

Error: Missing semicolon. [eslint: semi]
(In reply to Alexandre Poirot [:ochameau] from comment #9)
> Comment on attachment 8948417 [details]
> 1434333 - Import an existing HAR file;
> 
> https://reviewboard.mozilla.org/r/217878/#review225502
> 
> Sorry for the delay in the review.
Np, thanks for the review!


> * If I record HAR with cached request and then import it, I get this
> exception when selecting the cached requests and opening their response
> sidebar:
> ResponsePanel.js, line 145: TypeError: mimeType is undefined
> Also their "transfered side" is -1B, may be we could assume this is cached
> request when it is -1?
> It could be worth looking at what other browsers do.
This is not reproducible anymore (according to my testing and our chat).

> In HarImporter.doImport, you do:
>    this.actions.clearRequests();
> So I'm not sure there is any value in importing more than one HAR file at a
> time? 
This might be useful, but requires some UI/UX analysis. E.g. we might want to display list of pages like HAR Viewer [1] does and visually separate requests that belong to different pages. For now, It's enough to import just one HAR file (just like Chrome does).

[1] http://www.softwareishard.com/har/viewer/?path=examples/inline-scripts-block.har

In any case I filled bug 1438792, so it isn't forgotten.

> This is surprising, as you won't see any error if you don't have the split
> console opened. (I rarely have it opened outside of debugger)
I can't reproduce this. But there are bugs I am seeing
(so, perhaps these are what you are talking about):

1) The repeat number isn't displayed
2) The log should be an error (JSON parsing error)

It's related to the Console panel. I crated a report for this - bug 1438482.
Patch already attached.


> Shouldn't we use notification bar?
The log represents real JSON parsing error and it should be consistent
with how to display other errors. Note that e.g. source maps also 
display warnings as real warnings.

> When dropping over the StatisticsPanel, it looks broken. The pie chart and
> text all reset to gray/"loading", but never updates.
> Shouldn't you put all this drag'n drop/har importer code inside MonitorPanel?
> Also, wouldn't it be better to have a dedicated component, similar to
> VisibilityHandler, which would only implement this?
Yes, good point, done.

> > +    entry._securityState = file.securityState;
> 
> Why do you prefix this with `_`?

_securityState is custom filed in the HAR format and those
must use `_` prefix according to the spec.


> Could you test this new field in existing har tests?
Done

> I mean, is `input.cookies` ever defined?
Yes, but it's one of the things that should be cleaned up at some point
(not related to import though)

> It would be great to have this tested.
Done

> Should you warn when har.log.version < 1.2?
No, we actually export HAR 1.1 at this point
(we should upgrade to 1.2 at some point).

> Should you call addTimingMarker when pageTimings.onContentLoad /
> pageTimings.onLoad are equal to 0/-1?
Good catch, adding only if time > 0

> It would be great to complexify this test page or fork it to cover more
> usecases:
> * cookies
> * headers
> * cached requests
Done 

I also reported bug 1438831 for the total time issue.

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #3)
> @Dragana: can you remind me why bug 1384679 introduced `timing offsets`?
> 
> HAR spec [1] (the spec for data collected by network panel) is defining
> 'timings', but there are no offsets. Why we had to add the set of offsets
> and send it from the backend instead of doing the calculation on the client
> side? The `timings` HAR section should be enough to get all timing data we
> need ...
> 
> 
> Honza
> 
> [1] http://www.softwareishard.com/blog/har-12-spec/#timings

without TCP Fast Open and TLS early data each timing period starts when the previous one ends, e.g. sending data starts when connecting is finished. we did not need offsets 

with TCP Fast Open and TLS early data sending data can start at the same time as connect (we can send data on TCP syn packet). also tls handshake can carry application data thereby overlapping a sending data period and tls handshake period. Timing offsets are start time of each period.
Flags: needinfo?(dd.mozilla)
Comment on attachment 8948417 [details]
Bug 1434333 - Import an existing HAR file;

https://reviewboard.mozilla.org/r/217878/#review227382

The drag'n drop overlay is not longer displayed? Could it be linux specific? Related to React 16?

Otherwise it works great now!

::: devtools/client/netmonitor/src/components/DropHarHandler.js:82
(Diff revision 5)
> +        if (har) {
> +          this.appendPreview(har);
> +        }
> +      });
> +      reader();
> +    }

Until you figure out the UX to handle more than one HAR file, you should process only one and ignore the others. Processing the others is only a waste of computation as only the latest will be displayed.

::: devtools/client/netmonitor/src/components/DropHarHandler.js:95
(Diff revision 5)
> +    try {
> +      let importer = new HarImporter(this.props.actions);
> +      importer.import(har);
> +    } catch (err) {
> +      if (openSplitConsole) {
> +        openSplitConsole(err);

It may help to log a first line with a message like:
  "Error while processing HAR file: ..."
  Then the error here.

::: devtools/client/netmonitor/src/components/DropHarHandler.js:112
(Diff revision 5)
> +        onDragLeave: this.onDragLeave,
> +        onDrop: this.onDrop},
> +        this.props.children,
> +        div({className: "dropHarFiles"},
> +          div({}, DROP_HAR_FILES)
> +        )

The drag'n drop overlay isn't displayed anymore?
Could it be broken only on linux?

::: devtools/client/netmonitor/src/components/DropHarHandler.js:131
(Diff revision 5)
> +      fileReader.onloadend = () => {
> +        callback(fileReader.result);
> +      };
> +      fileReader.readAsText(file);
> +    }
> +  };

It would be easier to follow by using Promise here:
 return Promise.resolve(file.getAsText(""));
 return new Promise(resolve => {
   let fileReader = new FileReader();
   fileReader.onloadend = () => {
     resolve(fileReader.result);
   };
 });

Also, could you add a comment explaining why there is two codepaths here. Is it because of launchpad?

::: devtools/client/netmonitor/src/har/har-builder.js:115
(Diff revision 5)
>  
>      entry.request = await this.buildRequest(file);
>      entry.response = await this.buildResponse(file);
>      entry.cache = this.buildCache(file);
>      entry.timings = eventTimings ? eventTimings.timings : {};
> +    entry._securityState = file.securityState;

Please add a comment explaining why this particular field is "underscored".

::: devtools/client/netmonitor/src/har/har-builder.js:219
(Diff revision 5)
>    buildCookies: function (input) {
>      if (!input) {
>        return [];
>      }
>  
> -    return this.buildNameValuePairs(input.cookies);
> +    return this.buildNameValuePairs(input.cookies || input);

Could you file a bug about this, it is still unclear what is going on here.
Attachment #8948417 - Flags: review?(poirot.alex)
Thanks for the review!

(In reply to Alexandre Poirot [:ochameau] from comment #15)
> Until you figure out the UX to handle more than one HAR file, you should
> process only one and ignore the others. Processing the others is only a
> waste of computation as only the latest will be displayed.
Done

> It may help to log a first line with a message like:
>   "Error while processing HAR file: ..."
>   Then the error here.
Done

> The drag'n drop overlay isn't displayed anymore?
> Could it be broken only on linux?
Fixed

> It would be easier to follow by using Promise here:
>  return Promise.resolve(file.getAsText(""));
>  return new Promise(resolve => {
>    let fileReader = new FileReader();
>    fileReader.onloadend = () => {
>      resolve(fileReader.result);
>    };
>  });
Done

> Also, could you add a comment explaining why there is two codepaths here. Is
> it because of launchpad?
Yes (when net panel runs within a tab on different browser)
I tested again and it looks like FileReader is supported by both
Firefox and Chrome, so I used only FileReader + new comment.

> Please add a comment explaining why this particular field is "underscored".
Done

> > -    return this.buildNameValuePairs(input.cookies);
> > +    return this.buildNameValuePairs(input.cookies || input);
> 
> Could you file a bug about this, it is still unclear what is going on here.
Bug 1440275 - Properly store cookies in internal Net panel data structure

Honza
Comment on attachment 8948417 [details]
Bug 1434333 - Import an existing HAR file;

https://reviewboard.mozilla.org/r/217878/#review228988

I have a couple of minor comments, but it works great now.
Thanks Honza!

::: devtools/client/netmonitor/initializer.js:74
(Diff revision 6)
>      };
>  
> +    const openSplitConsole = (err) => {
> +      toolbox.openSplitConsole().then(() => {
> +        toolbox.target.logErrorInPage("Error while processing HAR file: "
> +          + err.message, "har");

The error message prefix should rather be in DropHarHandler.js's appendPreview.
You hand over `openSplitConsole` to many React components that may start using it now. If they do, they won't want to use this HAR import specific prefix.

::: devtools/client/netmonitor/src/components/DropHarHandler.js:83
(Diff revision 6)
> +      let reader = getFileReader(file).then(har => {
> +        if (har) {
> +          this.appendPreview(har);
> +        }
> +      });
> +      reader();

This line looks like a lefover.
Also, you may use async/await instead of using .then().

::: devtools/client/netmonitor/src/components/DropHarHandler.js:109
(Diff revision 6)
> +  render() {
> +    return (
> +      div({
> +        onDragEnter: this.onDragEnter,
> +        onDragOver: this.onDragOver,
> +        onDragLeave: this.onDragLeave,

With current patch, the drag'n drop overlay blinks as you move over netmonitor individual DOM elements, like column headers.
I don't know much about drag/drop events, but firefox codebase seems to use dragexit instead of dragleave:
https://searchfox.org/mozilla-central/source/browser/base/content/browser.xul#658-661
And if you replace leave by exit here, it no longer blinks.

::: devtools/client/netmonitor/src/components/DropHarHandler.js:122
(Diff revision 6)
> +  }
> +}
> +
> +// Helpers
> +
> +function getFileReader(file) {

nit: this function doesn't returns a FileReader, but the file content, so its name could be improved.

::: devtools/client/netmonitor/src/components/DropHarHandler.js:126
(Diff revision 6)
> +
> +function getFileReader(file) {
> +  // Test whether the browser supports FileReaders
> +  // (in case the Network panel is running in a tab)
> +  if (typeof FileReader == "undefined") {
> +    return Promise.resolve(null);

Shouldn't we instead reject the promise with an explicit error message?
But if FileReader is available in both launchpad and firefox, this check isn't really necessary.
Attachment #8948417 - Flags: review?(poirot.alex) → review+
(In reply to Alexandre Poirot [:ochameau] from comment #18)
> The error message prefix should rather be in DropHarHandler.js's
> appendPreview.
Done

> This line looks like a lefover.
Yes, removed

> With current patch, the drag'n drop overlay blinks as you move over
> netmonitor individual DOM elements, like column headers.
> I don't know much about drag/drop events, but firefox codebase seems to use
> dragexit instead of dragleave:
> https://searchfox.org/mozilla-central/source/browser/base/content/browser.
> xul#658-661
> And if you replace leave by exit here, it no longer blinks.
Nice!

> nit: this function doesn't returns a FileReader, but the file content, so
> its name could be improved.
Done

> Shouldn't we instead reject the promise with an explicit error message?
> But if FileReader is available in both launchpad and firefox, this check
> isn't really necessary.
Done

Honza
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 19 changes to 19 files
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Rev 1f95d943bf50 needs "Bug N" or "No bug" in the commit message.
remote: Jan Odvarko <odvarko@gmail.com>
remote: 1434333 - Import an existing HAR file; r=ochameau
remote: 
remote: MozReview-Commit-ID: 1N0FIcWzbkc
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.c_commitmessage hook failed
abort: push failed on remote
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 19 changes to 19 files
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Rev 1f95d943bf50 needs "Bug N" or "No bug" in the commit message.
remote: Jan Odvarko <odvarko@gmail.com>
remote: 1434333 - Import an existing HAR file; r=ochameau
remote: 
remote: MozReview-Commit-ID: 1N0FIcWzbkc
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.c_commitmessage hook failed
abort: push failed on remote
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 19 changes to 19 files
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Rev edaecc351479 needs "Bug N" or "No bug" in the commit message.
remote: Jan Odvarko <odvarko@gmail.com>
remote: 1434333 - Import an existing HAR file; r=ochameau
remote: 
remote: MozReview-Commit-ID: 1N0FIcWzbkc
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.c_commitmessage hook failed
abort: push failed on remote
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8dd669972fed
Import an existing HAR file; r=ochameau
https://hg.mozilla.org/mozilla-central/rev/8dd669972fed
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: