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

core(lantern): add configuration for precomputed network analysis #7239

Merged
merged 6 commits into from
Feb 26, 2019

Conversation

patrickhulce
Copy link
Collaborator

Summary
As discussed in the variance meeting earlier, this adds the ability to read in precomputed network analysis values and write them to a file. Nothing happens automatically for now it's up to the caller to do all the booking, etc to limit the complexity and difficult questions until we can validate how effective this is.

Related Issues/PRs
#6775

Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

Such a quick turn around!

I was thinking, should the data be additive? Like we record the data each time the --precomputed-lantern-data-path flag is present, so the format would be more like:

{  
   "additionalRttByOrigin":{  
      "domain1": [1, 2, 3],
      "domain2": [4, 5, 6]
   },
   "serverResponseTimeByOrigin":{  
      "domain1": [1, 2, 3],
      "domain2": [4, 5, 6]
   }
}

So that we can "build" more accurate averaged measurements? So when reading the file we look at all the records we've seen and extract some average or median or something?

@patrickhulce
Copy link
Collaborator Author

I was thinking, should the data be additive?

Definitely! Great idea!

IMO, this could probably wait until we prove out how much of an impact it has on the metrics, and even long-term fancy median/avg/merging logic could potentially be the caller's responsibility. I see potentially very different cases for CI and LR for example.

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

I still have strong reservations about this direction in terms of usability, desirability of effect, principle of least surprise, etc etc, but this implementation is a nice one :)

lighthouse-core/computed/load-simulator.js Show resolved Hide resolved
lighthouse-cli/cli-flags.js Outdated Show resolved Hide resolved
lighthouse-core/lib/asset-saver.js Outdated Show resolved Hide resolved
lighthouse-cli/bin.js Outdated Show resolved Hide resolved
@patrickhulce patrickhulce merged commit 41bc409 into master Feb 26, 2019
@patrickhulce patrickhulce deleted the precomputed_rtt_latency branch February 26, 2019 00:56
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.

3 participants