Make clock sync latency metric go between each pair of clock domains.

Make the clock sync metric report the latency between each pair of clock domains in the trace, regardless of which clock domain is chosen as the "model" clock domain.

BUG=catapult:#2785

Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/cd1b7d7427c7961f15aeeb7024f70417505a04fd

[alexandermont, 2016-09-12 23:58:46]

alexandermont@chromium.org changed reviewers:
+ charliea@chromium.org

[charliea (OOO until 10-5), 2016-09-21 18:05:33]

https://codereview.chromium.org/2338583002/diff/80001/tracing/tracing/metrics...
File tracing/tracing/metrics/system_health/clock_sync_latency_metric.html
(right):

https://codereview.chromium.org/2338583002/diff/80001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/clock_sync_latency_metric.html:21: for
(var i = 1; i < domains.length; i++) {
I think the more idiomatic way to write these loops is to do:

for (var i = 0; i < domains.length; i++) {
  for (var j = i + 1; j < domains.length; i++) {
    // Do whatever specific to combination (i, j)
  }
}

https://codereview.chromium.org/2338583002/diff/80001/tracing/tracing/model/c...
File tracing/tracing/model/clock_sync_manager.html (right):

https://codereview.chromium.org/2338583002/diff/80001/tracing/tracing/model/c...
tracing/tracing/model/clock_sync_manager.html:150: getModelTimeTransformer:
function(domainId, targetDomainId) {
We don't need this function to accept a targetDomainId, do we? It doesn't seem
like it's used anywhere, and it just provides people with a footgun.

https://codereview.chromium.org/2338583002/diff/80001/tracing/tracing/model/c...
tracing/tracing/model/clock_sync_manager.html:158: getModelTimeTransformerError:
function(domainId, targetDomainId) {
Hmmm, this doesn't make any sense: getModelTimeTransformerError indicates that
the transformer is *between* the *model* domain and domainId. Providing a second
parameter without changing the function name doesn't make sense.

My vote is to change it to 

getTimeTransformerError: function(fromDomainId, toDomainId) {
  ...
}

When you do this, don't forget to update the jsdoc to something reasonable for
the new func name.

https://codereview.chromium.org/2338583002/diff/80001/tracing/tracing/model/c...
tracing/tracing/model/clock_sync_manager.html:162: getModelTimeTransformerRaw_:
function(domainId, targetDomainId) {
As mentioned above, having a "modelTimeTransformer..." function that accepts two
domains doesn't make any sense name-wise.

My vote is that we have two functions, getModelTimeTransformerRaw_ and
getTimeTransformerRaw_. getTransformerRaw_ accepts two domains, fromDomainId and
toDomainId. getModelTimeTransformerRaw_ only accepts one, and defers to
getTimeTransformerRaw_ after doing any model-domain-specific things.

I'm thinking something like:

getModelTimeTransformerRaw_: function(domainId) {
  this.onDomainSeen_(domainId);
  
  if (!this.modelDomainId_)
    this.selectModelDomainId_();

  return this.getTimeTransformerRaw_(domainId, this.modelDomainId_);
},

getTimeTransformerRaw_: function(fromDomainId, toDomainId) {
  this.onDomainSeen_(fromDomainId);
  this.onDomainSeen_(toDomainId);

  var transformer = 
    this.getTransformerBetween_(fromDomainId, toDomainId);
  if (!transformer) {
    ...
  }

  return transformer;
}

Note that, we have to put this.onDomainSeen_(domainId) before the model
selection, even though it happens again in getTimeTransformerRaw_, because which
domains have been seen might have an impact on model clock domain selection.

[charliea (OOO until 10-5), 2016-09-21 18:06:31]

Also, this is a terrible CL title and description. Could you improve it?

[alexandermont, 2016-09-21 18:31:31]

Description was changed from

==========
Fix bug in clock sync latency.

BUG=catapult:#2785
==========

to

==========
Make the clock sync metric report the latency between each pair of clock domains
in the trace, regardless of which clock domain is chosen as the "model" clock
domain.

BUG=catapult:#2785
==========

[alexandermont, 2016-09-21 19:26:40]

https://codereview.chromium.org/2338583002/diff/80001/tracing/tracing/metrics...
File tracing/tracing/metrics/system_health/clock_sync_latency_metric.html
(right):

https://codereview.chromium.org/2338583002/diff/80001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/clock_sync_latency_metric.html:21: for
(var i = 1; i < domains.length; i++) {
On 2016/09/21 at 18:05:33, charliea wrote:
> I think the more idiomatic way to write these loops is to do:
> 
> for (var i = 0; i < domains.length; i++) {
>   for (var j = i + 1; j < domains.length; i++) {
>     // Do whatever specific to combination (i, j)
>   }
> }

Done

https://codereview.chromium.org/2338583002/diff/80001/tracing/tracing/model/c...
File tracing/tracing/model/clock_sync_manager.html (right):

https://codereview.chromium.org/2338583002/diff/80001/tracing/tracing/model/c...
tracing/tracing/model/clock_sync_manager.html:150: getModelTimeTransformer:
function(domainId, targetDomainId) {
On 2016/09/21 at 18:05:33, charliea wrote:
> We don't need this function to accept a targetDomainId, do we? It doesn't seem
like it's used anywhere, and it just provides people with a footgun.

Done

https://codereview.chromium.org/2338583002/diff/80001/tracing/tracing/model/c...
tracing/tracing/model/clock_sync_manager.html:158: getModelTimeTransformerError:
function(domainId, targetDomainId) {
On 2016/09/21 at 18:05:33, charliea wrote:
> Hmmm, this doesn't make any sense: getModelTimeTransformerError indicates that
the transformer is *between* the *model* domain and domainId. Providing a second
parameter without changing the function name doesn't make sense.
> 
> My vote is to change it to 
> 
> getTimeTransformerError: function(fromDomainId, toDomainId) {
>   ...
> }
> 
> When you do this, don't forget to update the jsdoc to something reasonable for
the new func name.

Done

https://codereview.chromium.org/2338583002/diff/80001/tracing/tracing/model/c...
tracing/tracing/model/clock_sync_manager.html:162: getModelTimeTransformerRaw_:
function(domainId, targetDomainId) {
On 2016/09/21 at 18:05:33, charliea wrote:
> As mentioned above, having a "modelTimeTransformer..." function that accepts
two domains doesn't make any sense name-wise.
> 
> My vote is that we have two functions, getModelTimeTransformerRaw_ and
getTimeTransformerRaw_. getTransformerRaw_ accepts two domains, fromDomainId and
toDomainId. getModelTimeTransformerRaw_ only accepts one, and defers to
getTimeTransformerRaw_ after doing any model-domain-specific things.
> 
> I'm thinking something like:
> 
> getModelTimeTransformerRaw_: function(domainId) {
>   this.onDomainSeen_(domainId);
>   
>   if (!this.modelDomainId_)
>     this.selectModelDomainId_();
> 
>   return this.getTimeTransformerRaw_(domainId, this.modelDomainId_);
> },
> 
> getTimeTransformerRaw_: function(fromDomainId, toDomainId) {
>   this.onDomainSeen_(fromDomainId);
>   this.onDomainSeen_(toDomainId);
> 
>   var transformer = 
>     this.getTransformerBetween_(fromDomainId, toDomainId);
>   if (!transformer) {
>     ...
>   }
> 
>   return transformer;
> }
> 
> Note that, we have to put this.onDomainSeen_(domainId) before the model
selection, even though it happens again in getTimeTransformerRaw_, because which
domains have been seen might have an impact on model clock domain selection.

Done

[charliea (OOO until 10-5), 2016-09-21 19:47:50]

https://codereview.chromium.org/2338583002/diff/100001/tracing/tracing/metric...
File tracing/tracing/metrics/system_health/clock_sync_latency_metric.html
(right):

https://codereview.chromium.org/2338583002/diff/100001/tracing/tracing/metric...
tracing/tracing/metrics/system_health/clock_sync_latency_metric.html:25: var
hist = new tr.v.Histogram(
Note: I think that we probably want to turn off all aggregates besides _avg like
we did in other metrics.

https://codereview.chromium.org/2338583002/diff/100001/tracing/tracing/metric...
tracing/tracing/metrics/system_health/clock_sync_latency_metric.html:27:
tr.v.HistogramBinBoundaries.createExponential(1e-3, 1e3, 30));
Could you make the histogram bin boundaries a constant above this function, and
also add a comment with some rationalization about why those boundaries are
reasonable?

Something like:

// Clock sync latencies range from usually range from X to Y, so these 
// boundaries allow for extra room on both sides.
var CLOCK_SYNC_LATENCY_BOUNDARIES = tr.v.HistogramBinBoundaries(...);

Also note: I think that we want the bottom boundary to be 0, for the case where
two clock domains are actually the same or have a zero-error conversion, and the
upper boundary to be something higher than 1e3 milliseconds (only 1
millisecond). Maybe 1e5 (10s)?

https://codereview.chromium.org/2338583002/diff/100001/tracing/tracing/metric...
tracing/tracing/metrics/system_health/clock_sync_latency_metric.html:30:
'clock_sync_latency_' + domains[j].toLowerCase() + '_to_' +
given that, in getTimeTransformerError, j is the "to" domain and i is the "from"
domain, shouldn't this be domains[i] + '_to_' + domains[j], not the other way
around? This would make it so that it would always be
clock_sync_latency_<alphabetically_first>_to_<alphabetically_second>, which
seems to make more sense anyhow

https://codereview.chromium.org/2338583002/diff/100001/tracing/tracing/model/...
File tracing/tracing/model/clock_sync_manager.html (right):

https://codereview.chromium.org/2338583002/diff/100001/tracing/tracing/model/...
tracing/tracing/model/clock_sync_manager.html:161: * |fromDomainId|, returns a
timestamp in domain |toDomainId|.
nit: s/in domain |toDomainId|/in the domain with |toDomainId|

(for consistency with earlier in the sentence, and it's just better English)

https://codereview.chromium.org/2338583002/diff/100001/tracing/tracing/model/...
tracing/tracing/model/clock_sync_manager.html:163: getTimeTransformer:
function(fromDomainId, toDomainId) {
I still don't understand: why is this function necessary? It seems like the only
place where it's called is getModelTimeTransformer(). Given that we don't ever
want people transforming to any domain besides the model anyhow, I don't think
we should offer them that option.

[alexandermont, 2016-09-21 21:59:04]

https://codereview.chromium.org/2338583002/diff/100001/tracing/tracing/metric...
File tracing/tracing/metrics/system_health/clock_sync_latency_metric.html
(right):

https://codereview.chromium.org/2338583002/diff/100001/tracing/tracing/metric...
tracing/tracing/metrics/system_health/clock_sync_latency_metric.html:25: var
hist = new tr.v.Histogram(
On 2016/09/21 at 19:47:50, charliea wrote:
> Note: I think that we probably want to turn off all aggregates besides _avg
like we did in other metrics.

Done

https://codereview.chromium.org/2338583002/diff/100001/tracing/tracing/metric...
tracing/tracing/metrics/system_health/clock_sync_latency_metric.html:27:
tr.v.HistogramBinBoundaries.createExponential(1e-3, 1e3, 30));
On 2016/09/21 at 19:47:50, charliea wrote:
> Could you make the histogram bin boundaries a constant above this function,
and also add a comment with some rationalization about why those boundaries are
reasonable?
> 
> Something like:
> 
> // Clock sync latencies range from usually range from X to Y, so these 
> // boundaries allow for extra room on both sides.
> var CLOCK_SYNC_LATENCY_BOUNDARIES = tr.v.HistogramBinBoundaries(...);
> 
> Also note: I think that we want the bottom boundary to be 0, for the case
where two clock domains are actually the same or have a zero-error conversion,
and the upper boundary to be something higher than 1e3 milliseconds (only 1
millisecond). Maybe 1e5 (10s)?

Done

https://codereview.chromium.org/2338583002/diff/100001/tracing/tracing/metric...
tracing/tracing/metrics/system_health/clock_sync_latency_metric.html:30:
'clock_sync_latency_' + domains[j].toLowerCase() + '_to_' +
On 2016/09/21 at 19:47:50, charliea wrote:
> given that, in getTimeTransformerError, j is the "to" domain and i is the
"from" domain, shouldn't this be domains[i] + '_to_' + domains[j], not the other
way around? This would make it so that it would always be
clock_sync_latency_<alphabetically_first>_to_<alphabetically_second>, which
seems to make more sense anyhow

Done

https://codereview.chromium.org/2338583002/diff/100001/tracing/tracing/model/...
File tracing/tracing/model/clock_sync_manager.html (right):

https://codereview.chromium.org/2338583002/diff/100001/tracing/tracing/model/...
tracing/tracing/model/clock_sync_manager.html:163: getTimeTransformer:
function(fromDomainId, toDomainId) {
On 2016/09/21 at 19:47:50, charliea wrote:
> I still don't understand: why is this function necessary? It seems like the
only place where it's called is getModelTimeTransformer(). Given that we don't
ever want people transforming to any domain besides the model anyhow, I don't
think we should offer them that option.

Removed.

[charliea (OOO until 10-5), 2016-10-07 00:34:54]

lgtm

Sorry I didn't take a look at this sooner!

[alexandermont, 2016-10-07 20:36:40]

The CQ bit was checked by alexandermont@chromium.org

[alexandermont, 2016-10-07 20:36:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
charliea@chromium.org
Link to the patchset: https://codereview.chromium.org/2338583002/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-07 20:36:48]

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, 2016-10-07 21:09:10]

Description was changed from

==========
Make the clock sync metric report the latency between each pair of clock domains
in the trace, regardless of which clock domain is chosen as the "model" clock
domain.

BUG=catapult:#2785
==========

to

==========
Make the clock sync metric report the latency between each pair of clock domains
in the trace, regardless of which clock domain is chosen as the "model" clock
domain.

BUG=catapult:#2785

Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2016-10-07 21:09:11]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...