Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2272203002:
Migrate tracing_metric from ScalarNumeric to Histograms (Closed)
|
Created:
4 years, 4 months ago by benjhayden Modified:
4 years, 3 months ago CC:
catapult-reviews_chromium.org, tracing-review_chromium.org Base URL:
https://github.com/catapult-project/catapult.git@master Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
DescriptionMigrate tracing_metric from ScalarNumeric to Histograms
BUG=catapult:#2711
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/a71c94da3936bd306ddab4feb0902c496ab176a4
Patch Set 1 #
Total comments: 5
Patch Set 2 : exponential count builder #Patch Set 3 : ssid #
Messages
Total messages: 20 (6 generated)
benjhayden@chromium.org changed reviewers: + eakuefner@chromium.org, wangzhen127@gmail.com
PTAL :-)
zhenw@chromium.org changed reviewers: + zhenw@chromium.org - wangzhen127@gmail.com
Is there any pointer to why we would want to make this migration?
On 2016/08/24 at 22:41:53, zhenw wrote: > Is there any pointer to why we would want to make this migration? The bug links to the Value system hierarchy collapse design sketch: https://docs.google.com/document/d/10r7us9S-WcnT9Ke469wR8fBffTvwV6tF7zsPRTWZ4... Here's a quick summary. results2.html currently merges values in order to reduce the firehose of results. Soon, telemetry will be merging values and feeding the merged values to the dashboard in a process that resembles the TBMv1's summarization. Histograms support merging intuitively. There is an algorithm for merging ScalarNumerics into a Histogram, but it is fragile and unpredictable. Metrics are also burdened by too many different concepts, so we are collapsing the Value hierarchy and hiding Scalars from metrics.
Using histogram for just one sample seems overkill to me. If one metric author starts to write metrics and want to collect just one sample, it is not easy for him/her to think about using histogram. Having said that, if this migration is required by other design decisions, I am ok with that.
On 2016/08/24 at 22:56:16, zhenw wrote: > Using histogram for just one sample seems overkill to me. If one metric author starts to write metrics and want to collect just one sample, it is not easy for him/her to think about using histogram. > > Having said that, if this migration is required by other design decisions, I am ok with that. Zhen, we need to migrate to histograms for a variety of technical reasons that Ben describes, but I don't think this is overkill from the standpoint of metric authors either. True, it's not easy for metric authors to think about histograms up front, but we should view it as statistically necessary. Given that the implication of any TBMv2 metric is that we intend to use it to collect vast amounts of data (in the style of UMA, which also requires its users to use histograms), we think it makes sense for metric authors to think carefully about the shape of the data they intend to collect up front. Ben, lgtm from my perspective.
> Zhen, we need to migrate to histograms for a variety of technical reasons that > Ben describes, but I don't think this is overkill from the standpoint of metric > authors either. True, it's not easy for metric authors to think about histograms > up front, but we should view it as statistically necessary. Given that the > implication of any TBMv2 metric is that we intend to use it to collect vast > amounts of data (in the style of UMA, which also requires its users to use > histograms), we think it makes sense for metric authors to think carefully about > the shape of the data they intend to collect up front. Makes sense to me. I have some questions on the histogram configuration. https://codereview.chromium.org/2272203002/diff/1/tracing/tracing/metrics/tra... File tracing/tracing/metrics/tracing_metric.html (right): https://codereview.chromium.org/2272203002/diff/1/tracing/tracing/metrics/tra... tracing/tracing/metrics/tracing_metric.html:22: tr.b.Range.fromExplicitRange(1e-2, 1e5), 30); ssid, can you take a look to see if this setting is reasonable to you? https://codereview.chromium.org/2272203002/diff/1/tracing/tracing/metrics/tra... tracing/tracing/metrics/tracing_metric.html:30: tr.b.Range.fromExplicitRange(0, 1000), 40); 1000 is not enough, e.g, the following has close to 70,000. I would suggest using at least 100,000 as the upper bound. https://chromeperf.appspot.com/report?sid=927f8f18a8a6e7747c4c4a2d63980e257c2... And why are there 40 bins and the other 2 are 30 bins?
As you can see, I have not thought thoroughly about the shape of these metrics. I'm relying on you to help me figure out appropriate ranges, linear vs exponential, and bin counts. I wouldn't worry too much about getting them right on the first try, however. I don't think that it will be traumatic to change the bin configurations if they don't look right. Currently, the dashboard only receives the statistics, which are computed independently of the bin configuration. We will want to have thought about bin configurations more carefully by the time the dashboard deals with histograms directly, because we can't merge histograms with different bin configurations, although we could merge only their statistics if necessary for, e.g. bisect. I think that the biggest consequence of bin configurations currently is how it looks in bar charts in results2.html, which is currently broken for exponential histograms anyway. So just take your best guess and let's run with it for now. :-) https://codereview.chromium.org/2272203002/diff/1/tracing/tracing/metrics/tra... File tracing/tracing/metrics/tracing_metric.html (right): https://codereview.chromium.org/2272203002/diff/1/tracing/tracing/metrics/tra... tracing/tracing/metrics/tracing_metric.html:30: tr.b.Range.fromExplicitRange(0, 1000), 40); On 2016/08/25 at 21:19:46, Zhen Wang wrote: > 1000 is not enough, e.g, the following has close to 70,000. I would suggest using at least 100,000 as the upper bound. > https://chromeperf.appspot.com/report?sid=927f8f18a8a6e7747c4c4a2d63980e257c2... Thanks! > > And why are there 40 bins and the other 2 are 30 bins? Now that I know that this count can regularly reach 5 digits, I think that an exponential histogram would be more appropriate than linear, and, since exponential histograms are in some sense more efficient, we can probably reduce the number of bins to match the other histograms?
https://codereview.chromium.org/2272203002/diff/1/tracing/tracing/metrics/tra... File tracing/tracing/metrics/tracing_metric.html (right): https://codereview.chromium.org/2272203002/diff/1/tracing/tracing/metrics/tra... tracing/tracing/metrics/tracing_metric.html:30: tr.b.Range.fromExplicitRange(0, 1000), 40); On 2016/08/26 04:53:41, benjhayden wrote: > On 2016/08/25 at 21:19:46, Zhen Wang wrote: > > 1000 is not enough, e.g, the following has close to 70,000. I would suggest > using at least 100,000 as the upper bound. > > > https://chromeperf.appspot.com/report?sid=927f8f18a8a6e7747c4c4a2d63980e257c2... > > Thanks! > > > > > And why are there 40 bins and the other 2 are 30 bins? > > Now that I know that this count can regularly reach 5 digits, I think that an > exponential histogram would be more appropriate than linear, and, since > exponential histograms are in some sense more efficient, we can probably reduce > the number of bins to match the other histograms? sgtm
benjhayden@chromium.org changed reviewers: + ssid@chromium.org
ssid PTAL :-)
lgtm
lgtm, sorry for delay. https://codereview.chromium.org/2272203002/diff/1/tracing/tracing/metrics/tra... File tracing/tracing/metrics/tracing_metric.html (right): https://codereview.chromium.org/2272203002/diff/1/tracing/tracing/metrics/tra... tracing/tracing/metrics/tracing_metric.html:22: tr.b.Range.fromExplicitRange(1e-2, 1e5), 30); On 2016/08/25 21:19:46, Zhen Wang wrote: > ssid, can you take a look to see if this setting is reasonable to you? I think the lower could be 1e-3. IIUC, 1e-2 means 0.01. The trace events can sometimes measure duration upto 0.001ms. Not saying these numbers are useful to track, but just for the sake of correctness.
On 2016/08/26 at 22:32:13, ssid wrote: > lgtm, sorry for delay. > > https://codereview.chromium.org/2272203002/diff/1/tracing/tracing/metrics/tra... > File tracing/tracing/metrics/tracing_metric.html (right): > > https://codereview.chromium.org/2272203002/diff/1/tracing/tracing/metrics/tra... > tracing/tracing/metrics/tracing_metric.html:22: tr.b.Range.fromExplicitRange(1e-2, 1e5), 30); > On 2016/08/25 21:19:46, Zhen Wang wrote: > > ssid, can you take a look to see if this setting is reasonable to you? > > I think the lower could be 1e-3. IIUC, 1e-2 means 0.01. > The trace events can sometimes measure duration upto 0.001ms. Not saying these numbers are useful to track, but just for the sake of correctness. Thanks!
The CQ bit was checked by benjhayden@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org, zhenw@chromium.org, ssid@chromium.org Link to the patchset: https://codereview.chromium.org/2272203002/#ps40001 (title: "ssid")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Migrate tracing_metric from ScalarNumeric to Histograms BUG=catapult:#2711 ========== to ========== Migrate tracing_metric from ScalarNumeric to Histograms BUG=catapult:#2711 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
