Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2658143002:
Create a trace import duration metric (Closed)
|
Created:
3 years, 11 months ago by charliea (OOO until 10-5) Modified:
3 years, 10 months ago Reviewers:
benjhayden CC:
catapult-reviews_chromium.org, eakuefner, tracing-review_chromium.org Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
DescriptionCreate a trace import duration metric
This metric will allow us to use TBMv2 to monitor how the trace import
duration changes over time.
BUG=chromium:665550
Review-Url: https://codereview.chromium.org/2658143002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/bbfc356b5b6ec0200b27805acef8486938522968
Patch Set 1 #
Total comments: 10
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #Patch Set 4 : Synced to head #Patch Set 5 : Fixed tests #
Messages
Total messages: 29 (16 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
charliea@chromium.org changed reviewers: + benjhayden@chromium.org
Description was changed from ========== Create a trace import duration metric. This metric will allow us to use TBMv2 to monitor how the trace import duration changes over time. BUG=chromium:665550 ========== to ========== Create a trace import duration metric This metric will allow us to use TBMv2 to monitor how the trace import duration changes over time. BUG=chromium:665550 ==========
https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/trace_import_duration_metric.html (right): https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/trace_import_duration_metric.html:17: 1e-3, // Use a lower bound of 1 microsecond. Is 1us vs 1ms that interesting? I would be very surprised if importing any trace ever took less than 1ms. https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/trace_import_duration_metric.html:19: 9); 9 is an odd number of bins. Why not 10 or 20 or 30? https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/trace_import_duration_metric.html:47: traceImportDurationMetric Can you add a trailing comma for consistency? https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/trace_import_duration_metric_test.html (right): https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/trace_import_duration_metric_test.html:34: let valueSet = new tr.v.HistogramSet(); The style-guide says that let is still pending discussion. https://github.com/catapult-project/catapult/blob/master/docs/style-guide.md https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/trace_import_duration_metric_test.html:37: assert.equal(getMetricValueAvg(valueSet, 'trace_import_duration'), 10); Can you use tr.b.getOnlyElement(valueSet.getValuesNamed('...')).average instead of a custom function? And assert.strictEqual() instead of equal()?
On 2017/01/27 04:31:16, benjhayden wrote: > https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... > File tracing/tracing/metrics/trace_import_duration_metric.html (right): > > https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... > tracing/tracing/metrics/trace_import_duration_metric.html:17: 1e-3, // Use a > lower bound of 1 microsecond. > Is 1us vs 1ms that interesting? > I would be very surprised if importing any trace ever took less than 1ms. > > https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... > tracing/tracing/metrics/trace_import_duration_metric.html:19: 9); > 9 is an odd number of bins. Why not 10 or 20 or 30? > > https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... > tracing/tracing/metrics/trace_import_duration_metric.html:47: > traceImportDurationMetric > Can you add a trailing comma for consistency? > > https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... > File tracing/tracing/metrics/trace_import_duration_metric_test.html (right): > > https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... > tracing/tracing/metrics/trace_import_duration_metric_test.html:34: let valueSet > = new tr.v.HistogramSet(); > The style-guide says that let is still pending discussion. > https://github.com/catapult-project/catapult/blob/master/docs/style-guide.md > > https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... > tracing/tracing/metrics/trace_import_duration_metric_test.html:37: > assert.equal(getMetricValueAvg(valueSet, 'trace_import_duration'), 10); > Can you use tr.b.getOnlyElement(valueSet.getValuesNamed('...')).average instead > of a custom function? > > And assert.strictEqual() instead of equal()? Hmhh, why not group this with tracing_metrics which already collect trace sizes?
On 2017/01/27 at 13:32:51, nednguyen wrote: > On 2017/01/27 04:31:16, benjhayden wrote: > > https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... > > File tracing/tracing/metrics/trace_import_duration_metric.html (right): > > > > https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... > > tracing/tracing/metrics/trace_import_duration_metric.html:17: 1e-3, // Use a > > lower bound of 1 microsecond. > > Is 1us vs 1ms that interesting? > > I would be very surprised if importing any trace ever took less than 1ms. > > > > https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... > > tracing/tracing/metrics/trace_import_duration_metric.html:19: 9); > > 9 is an odd number of bins. Why not 10 or 20 or 30? > > > > https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... > > tracing/tracing/metrics/trace_import_duration_metric.html:47: > > traceImportDurationMetric > > Can you add a trailing comma for consistency? > > > > https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... > > File tracing/tracing/metrics/trace_import_duration_metric_test.html (right): > > > > https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... > > tracing/tracing/metrics/trace_import_duration_metric_test.html:34: let valueSet > > = new tr.v.HistogramSet(); > > The style-guide says that let is still pending discussion. > > https://github.com/catapult-project/catapult/blob/master/docs/style-guide.md > > > > https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... > > tracing/tracing/metrics/trace_import_duration_metric_test.html:37: > > assert.equal(getMetricValueAvg(valueSet, 'trace_import_duration'), 10); > > Can you use tr.b.getOnlyElement(valueSet.getValuesNamed('...')).average instead > > of a custom function? > > > > And assert.strictEqual() instead of equal()? > > Hmhh, why not group this with tracing_metrics which already collect trace sizes? The tracingMetric seems to compute information about tracing agents and traces, not about the trace import process. What would y'all think of naming this new metric traceImportMetric in trace_import_metric.html? Maybe it could eventually measure the amounts of browser/vinn memory required to import traces as well as how long various steps take?
I agree that, as it stands, the tracing_metric deals mostly with measuring the *recording* of traces rather than the *processing* of traces. I have no strong opinions on whether it should also include metrics about trace importing.
On 2017/01/31 at 05:51:10, charliea wrote: > I agree that, as it stands, the tracing_metric deals mostly with measuring the *recording* of traces rather than the *processing* of traces. I have no strong opinions on whether it should also include metrics about trace importing. I think that recording traces and importing traces are different enough processes that it doesn't make sense to talk about them at the same time or in the same ways. I may be wrong. :-) I also think that where this metric lives doesn't necessarily need to block this CL. The code can always move from one file to another, and, if the metric is referenced in by a benchmark in another repo, then a 3-sided patch can be used to update the reference. In chromium, window.performance.memory.usedJsHeapSize might be an interesting measurement for traceImportMetric to record. I don't know how to access that information from a metric in vinn, but the 'v8' module in node exposes 'used_heap_size'.
Patchset #2 (id:60001) has been deleted
As Ned suggested, I went ahead and just added this to tracing_metric given how easy it is to do that. If, in the future, we find that this file has grown too much, we can always split off the metric at a low cost. https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/trace_import_duration_metric.html (right): https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/trace_import_duration_metric.html:17: 1e-3, // Use a lower bound of 1 microsecond. On 2017/01/27 04:31:16, benjhayden wrote: > Is 1us vs 1ms that interesting? > I would be very surprised if importing any trace ever took less than 1ms. The shortest I've ever seen is something like half a millisecond for an almost-empty trace, so yea, this should probably be increased. https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/trace_import_duration_metric.html:19: 9); On 2017/01/27 04:31:16, benjhayden wrote: > 9 is an odd number of bins. Why not 10 or 20 or 30? Given that it was an exponential bin boundary, I just went with 1 bin for each order of magnitude. I went ahead and changed it to 10. https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/trace_import_duration_metric.html:47: traceImportDurationMetric On 2017/01/27 04:31:16, benjhayden wrote: > Can you add a trailing comma for consistency? Done. https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/trace_import_duration_metric_test.html (right): https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/trace_import_duration_metric_test.html:34: let valueSet = new tr.v.HistogramSet(); On 2017/01/27 04:31:16, benjhayden wrote: > The style-guide says that let is still pending discussion. > https://github.com/catapult-project/catapult/blob/master/docs/style-guide.md Good point. I asked Ethan, and he fixed the merge conflict and submitted this morning in https://codereview.chromium.org/2621643003/ https://codereview.chromium.org/2658143002/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/trace_import_duration_metric_test.html:37: assert.equal(getMetricValueAvg(valueSet, 'trace_import_duration'), 10); On 2017/01/27 04:31:16, benjhayden wrote: > Can you use tr.b.getOnlyElement(valueSet.getValuesNamed('...')).average instead > of a custom function? > > And assert.strictEqual() instead of equal()? As discussed offline, we can use getHistogramNamed('trace_import_duration').average now. https://codereview.chromium.org/2658143002/diff/80001/tracing/tracing/metrics... File tracing/tracing/metrics/tracing_metric.html (right): https://codereview.chromium.org/2658143002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:101: addTimeDurationHistogram( As described above, I erred on the side of consistency with the rest of the codebase with the metric name rather than consistency with the other metrics in this file, which should almost certainly be renamed. https://codereview.chromium.org/2658143002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:104: if (!model.stats.hasEventSizesinBytes) return; This brings the metric more in line with other metrics, which just don't add their histograms if the required data isn't available. Without this change, we could never actually turn on the tracing metric for a benchmark because hasEventSizesInBytes can only be turned on for interactive traces (I think).
The CQ bit was checked by charliea@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Catapult Android Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20An...) Catapult Linux Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Li...) Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)
1 nit in the test then lgtm https://codereview.chromium.org/2658143002/diff/80001/tracing/tracing/metrics... File tracing/tracing/metrics/tracing_metric.html (right): https://codereview.chromium.org/2658143002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:97: // (e.g. 'trace_size_growth_rate' with a unit of bytes/second, not 'Max event This comment is fine as is, but I wanted to make a note that it might require more discussion before fixing: All of the current units are simple: ms, B, W, J. Do we really need to define complex units like bytes/second? Should it be bytes/ms for consistency? Or should ms be changed to seconds? How would conversion work for complex units? Would we need to be able to convert to e.g. TB/year? Do we need to define a calculus of complex Units to express B/s, J/s? Again, no need to change the comment, just thoughts to keep in mind. https://codereview.chromium.org/2658143002/diff/80001/tracing/tracing/metrics... File tracing/tracing/metrics/tracing_metric_test.html (right): https://codereview.chromium.org/2658143002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric_test.html:175: assert.equal( strictEqual?
https://codereview.chromium.org/2658143002/diff/80001/tracing/tracing/metrics... File tracing/tracing/metrics/tracing_metric.html (right): https://codereview.chromium.org/2658143002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:97: // (e.g. 'trace_size_growth_rate' with a unit of bytes/second, not 'Max event On 2017/02/17 16:48:55, benjhayden wrote: > This comment is fine as is, but I wanted to make a note that it might require > more discussion before fixing: > All of the current units are simple: ms, B, W, J. > Do we really need to define complex units like bytes/second? > Should it be bytes/ms for consistency? Or should ms be changed to seconds? > How would conversion work for complex units? Would we need to be able to convert > to e.g. TB/year? > Do we need to define a calculus of complex Units to express B/s, J/s? > > Again, no need to change the comment, just thoughts to keep in mind. Hmmm, yea, you're completely right. I hadn't really given much thought about how this would complicate our existing unit system. And what if the numerator is something that we don't have a unit for, like "garbage collections" if we're measuring GCs/second? It seems like keeping this as "bytes" and just renaming the metric to something like "trace_size_growth_per_second" is a better idea. As for the millisecond/second thing, I think there are definitely a lot of ideas that are more easily expressed in seconds than milliseconds (e.g. power is energy per second, and bytes/second for trace size growth makes more second to me than bytes/millisecond). We can sort that out when we implement it, though. https://codereview.chromium.org/2658143002/diff/80001/tracing/tracing/metrics... File tracing/tracing/metrics/tracing_metric_test.html (right): https://codereview.chromium.org/2658143002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric_test.html:175: assert.equal( On 2017/02/17 16:48:55, benjhayden wrote: > strictEqual? Done.
The CQ bit was checked by charliea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from benjhayden@chromium.org Link to the patchset: https://codereview.chromium.org/2658143002/#ps120001 (title: "Synced to head")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)
The CQ bit was checked by charliea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from benjhayden@chromium.org Link to the patchset: https://codereview.chromium.org/2658143002/#ps140001 (title: "Fixed tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487776400782500,
"parent_rev": "0cc3f7ab57ee2fab3930f80fa6c74bab20838728", "commit_rev":
"bbfc356b5b6ec0200b27805acef8486938522968"}
Message was sent while issue was closed.
Description was changed from ========== Create a trace import duration metric This metric will allow us to use TBMv2 to monitor how the trace import duration changes over time. BUG=chromium:665550 ========== to ========== Create a trace import duration metric This metric will allow us to use TBMv2 to monitor how the trace import duration changes over time. BUG=chromium:665550 Review-Url: https://codereview.chromium.org/2658143002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
