Add more data to net-internals view of DNS cache

Add more data to net-internals view of DNS cache

The cache can now have entries that are expired because they're from a
different network, but are still stored in the cache. This change
updates net-internals to expose that information, and also exposes the
TTL for easier debugging.

BUG=703821
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2773023003
Cr-Commit-Position: refs/heads/master@{#460548}
Committed: https://chromium.googlesource.com/chromium/src/+/7a7492161dee610c0464a8f8f61b61e72db0613a

[mgersh, 2017-03-24 16:21:53]

Description was changed from

==========
Add more data to net-internals view of DNS cache

The cache can now have entries that are expired because they're from a
different network, but are still stored in the cache. This change
updates net-internals to expose that information, and also exposes the
TTL for easier debugging.

BUG=703821
==========

to

==========
Add more data to net-internals view of DNS cache

The cache can now have entries that are expired because they're from a
different network, but are still stored in the cache. This change
updates net-internals to expose that information, and also exposes the
TTL for easier debugging.

BUG=703821
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[mgersh, 2017-03-24 16:27:46]

mgersh@chromium.org changed reviewers:
+ eroman@chromium.org

[mgersh, 2017-03-24 16:27:47]

PTAL

[mgersh, 2017-03-28 22:00:41]

Ping?

[eroman, 2017-03-28 22:31:36]

Thanks for the ping, slipped my queue!

[eroman, 2017-03-28 22:41:59]

lgtm

https://codereview.chromium.org/2773023003/diff/1/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/dns_view.js (right):

https://codereview.chromium.org/2773023003/diff/1/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/dns_view.js:89:
$(DnsView.NETWORK_SPAN_ID).innerText = hostResolverCache.network_changes;
When loading older log files, network_changes will not be defined, in which case
I imagine this will display "undefined".

How about replacing that expression with:

hostResolverCache.network_changes || '';

https://codereview.chromium.org/2773023003/diff/1/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/dns_view.js:138: if (e.network_changes <
hostResolverCache.network_changes) {
same comment -- what happens to this comparison when loading older logs that
don't have network_changes properties?


Also can you add a comment explaining this logic?

https://codereview.chromium.org/2773023003/diff/1/chrome/test/data/webui/net_...
File chrome/test/data/webui/net_internals/dns_view.js (right):

https://codereview.chromium.org/2773023003/diff/1/chrome/test/data/webui/net_...
chrome/test/data/webui/net_internals/dns_view.js:1: 
nit: remove this change

https://codereview.chromium.org/2773023003/diff/1/chrome/test/data/webui/net_...
chrome/test/data/webui/net_internals/dns_view.js:153: * A Task that simulates a
network change and checks that cache entries are
nice job adding tests!

[mgersh, 2017-03-29 18:15:25]

One more question besides the one about "undefined": will this have to be merged
to the new version somehow? I know there's a deprecated/not ready yet situation
but don't know the details.

https://codereview.chromium.org/2773023003/diff/1/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/dns_view.js (right):

https://codereview.chromium.org/2773023003/diff/1/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/dns_view.js:89:
$(DnsView.NETWORK_SPAN_ID).innerText = hostResolverCache.network_changes;
On 2017/03/28 22:41:59, eroman wrote:
> When loading older log files, network_changes will not be defined, in which
case
> I imagine this will display "undefined".
> 
> How about replacing that expression with:
> 
> hostResolverCache.network_changes || '';

Ah yeah. I thought to check old Chrome loading new netlog, but not new Chrome
loading old netlog. But your proposed simple fix doesn't work because 0 is
falsy, and properly getting this and the other fields to show empty string
instead of "undefined" seems like more complexity than it's worth in this
context for a case that won't happen much. What do you think?

https://codereview.chromium.org/2773023003/diff/1/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/dns_view.js:138: if (e.network_changes <
hostResolverCache.network_changes) {
On 2017/03/28 22:41:59, eroman wrote:
> same comment -- what happens to this comparison when loading older logs that
> don't have network_changes properties?

undefined < undefined evaluates to false, so the comparison works fine as-is.
> 
> 
> Also can you add a comment explaining this logic?
Done.

https://codereview.chromium.org/2773023003/diff/1/chrome/test/data/webui/net_...
File chrome/test/data/webui/net_internals/dns_view.js (right):

https://codereview.chromium.org/2773023003/diff/1/chrome/test/data/webui/net_...
chrome/test/data/webui/net_internals/dns_view.js:1: 
On 2017/03/28 22:41:59, eroman wrote:
> nit: remove this change

Done.

https://codereview.chromium.org/2773023003/diff/1/chrome/test/data/webui/net_...
chrome/test/data/webui/net_internals/dns_view.js:153: * A Task that simulates a
network change and checks that cache entries are
On 2017/03/28 22:41:59, eroman wrote:
> nice job adding tests!

thanks :)

[eroman, 2017-03-29 18:36:11]

https://codereview.chromium.org/2773023003/diff/1/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/dns_view.js (right):

https://codereview.chromium.org/2773023003/diff/1/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/dns_view.js:89:
$(DnsView.NETWORK_SPAN_ID).innerText = hostResolverCache.network_changes;
On 2017/03/29 18:15:25, mgersh wrote:
> On 2017/03/28 22:41:59, eroman wrote:
> > When loading older log files, network_changes will not be defined, in which
> case
> > I imagine this will display "undefined".
> > 
> > How about replacing that expression with:
> > 
> > hostResolverCache.network_changes || '';
> 
> Ah yeah. I thought to check old Chrome loading new netlog, but not new Chrome
> loading old netlog. But your proposed simple fix doesn't work because 0 is
> falsy, and properly getting this and the other fields to show empty string
> instead of "undefined" seems like more complexity than it's worth in this
> context for a case that won't happen much. What do you think?

Yes I think it is worth it.

In general not doing this can lead to artifacts like "undefined" or "null"
showing up when loading logs, or in other instances even worse it throws and
exception and then things break. It is often solved by adding checks accompanied
by a TODO comment that it can be removed in milestone YYY (a couple in the
future).

What do you think about something like this:

// TODO(yyy): The |ttl| and |network_changes| properties were introduced in M___
and may not exist when loading older logs. Can remove this in M___.
function valueOrDefault(value, defaultValue) {
  if (value != undefined)
    return value;
  return defaultValue
}

...innerText = valueOrDefault(hostResolverCache.network_changes, '');

and basically wrap each call to .ttl or network_changes in a call to
valueOrDefault(); should just be a couple callsites in this file, but does give
graceful backward compatibility.

[eroman, 2017-03-29 18:36:36]

> One more question besides the one about "undefined": will this have to be
merged
> to the new version somehow? I know there's a deprecated/not ready yet
situation
> but don't know the details.

There is a version hosted in the catapult repository for the HTML/javascript.
You don't need to worry about merging the changes there, we will do it before
any switch-over.

Unfortunately the browser tests doesn't really translate to an equivalent there,
so would need to be re-implemented (or deleted) when switching over. Which is to
say, may not be worth spending too much time on browser tests for future
changes, other than pure javascript ones.

Cheers.

[mgersh, 2017-03-29 20:20:53]

https://codereview.chromium.org/2773023003/diff/1/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/dns_view.js (right):

https://codereview.chromium.org/2773023003/diff/1/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/dns_view.js:89:
$(DnsView.NETWORK_SPAN_ID).innerText = hostResolverCache.network_changes;
On 2017/03/29 18:36:11, eroman wrote:
> On 2017/03/29 18:15:25, mgersh wrote:
> > On 2017/03/28 22:41:59, eroman wrote:
> > > When loading older log files, network_changes will not be defined, in
which
> > case
> > > I imagine this will display "undefined".
> > > 
> > > How about replacing that expression with:
> > > 
> > > hostResolverCache.network_changes || '';
> > 
> > Ah yeah. I thought to check old Chrome loading new netlog, but not new
Chrome
> > loading old netlog. But your proposed simple fix doesn't work because 0 is
> > falsy, and properly getting this and the other fields to show empty string
> > instead of "undefined" seems like more complexity than it's worth in this
> > context for a case that won't happen much. What do you think?
> 
> Yes I think it is worth it.
> 
> In general not doing this can lead to artifacts like "undefined" or "null"
> showing up when loading logs, or in other instances even worse it throws and
> exception and then things break. It is often solved by adding checks
accompanied
> by a TODO comment that it can be removed in milestone YYY (a couple in the
> future).
> 
> What do you think about something like this:
> 
> // TODO(yyy): The |ttl| and |network_changes| properties were introduced in
M___
> and may not exist when loading older logs. Can remove this in M___.
> function valueOrDefault(value, defaultValue) {
>   if (value != undefined)
>     return value;
>   return defaultValue
> }
> 
> ...innerText = valueOrDefault(hostResolverCache.network_changes, '');
> 
> and basically wrap each call to .ttl or network_changes in a call to
> valueOrDefault(); should just be a couple callsites in this file, but does
give
> graceful backward compatibility.

I still don't entirely understand why it's important for fields to be empty
instead of having the word "undefined" in them, but sure, done.

[mgersh, 2017-03-29 20:21:03]

The CQ bit was checked by mgersh@chromium.org

[mgersh, 2017-03-29 20:21:04]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2773023003/#ps40001
(title: "undefined -> empty string")

[commit-bot: I haz the power, 2017-03-29 20:21:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 22:13:01]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490818863590690,
"parent_rev": "517d0115d3c435383f79a0b730b6514a6cc97f44", "commit_rev":
"7a7492161dee610c0464a8f8f61b61e72db0613a"}

[commit-bot: I haz the power, 2017-03-29 22:14:37]

Description was changed from

==========
Add more data to net-internals view of DNS cache

The cache can now have entries that are expired because they're from a
different network, but are still stored in the cache. This change
updates net-internals to expose that information, and also exposes the
TTL for easier debugging.

BUG=703821
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add more data to net-internals view of DNS cache

The cache can now have entries that are expired because they're from a
different network, but are still stored in the cache. This change
updates net-internals to expose that information, and also exposes the
TTL for easier debugging.

BUG=703821
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2773023003
Cr-Commit-Position: refs/heads/master@{#460548}
Committed:
https://chromium.googlesource.com/chromium/src/+/7a7492161dee610c0464a8f8f61b...
==========

[commit-bot: I haz the power, 2017-03-29 22:14:38]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/7a7492161dee610c0464a8f8f61b...