Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2133963002:
Implement Composition.buildFromEvents() (Closed)
|
Created:
4 years, 5 months ago by benjhayden Modified:
4 years, 5 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. |
DescriptionImplement Composition.buildFromEvents()
Currently, there is no example of how to use Composition.
This CL implements a static method that demonstrates how to build a Composition
from an EventSet, with an example caller in long_tasks_metric using
UserFriendlyCategoryDriver.
Getting this to look nice required:
making Chart margins easier to deal with so that embedders could customize them,
fixing a style bug in tr-ui-b-table (vertical-align: text-top != top),
fixing a style bug in value-set-table (charts assume text-align: left),
updating composition-span to use the new Chart APIs,
adding NumericBuilder.createExponential() to mirror createLinear().
Screenshot (google-only): https://screenshot.googleplex.com/dWZz92Mxm7X.png
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/891a87f026ab8a6b19d39f4158957fe2a9f3a0b5
Patch Set 1 : fix composition-span #
Total comments: 8
Patch Set 2 : fix a bunch of crap #Patch Set 3 : . #
Total comments: 19
Patch Set 4 : . #Messages
Total messages: 42 (25 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
benjhayden@chromium.org changed reviewers: + eakuefner@chromium.org, nednguyen@google.com
PTAL :-) I'll try to get a screenshot and integrate this into Ned's TTI metric on Monday.
Description was changed from ========== Implement Composition.buildUserFriendlyCategories() Currently, there is no example of how to use Composition. This CL implements a static method that demonstrates how to build a Composition using UserFriendlyCategoryDriver. ========== to ========== Implement Composition.build() Currently, there is no example of how to use Composition. This CL implements a static method that demonstrates how to build a Composition from an EventSet, with an example caller in long_tasks_metric using UserFriendlyCategoryDriver. ==========
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Description was changed from ========== Implement Composition.build() Currently, there is no example of how to use Composition. This CL implements a static method that demonstrates how to build a Composition from an EventSet, with an example caller in long_tasks_metric using UserFriendlyCategoryDriver. ========== to ========== Implement Composition.build() Currently, there is no example of how to use Composition. This CL implements a static method that demonstrates how to build a Composition from an EventSet, with an example caller in long_tasks_metric using UserFriendlyCategoryDriver. Screenshot (google-only, feat. old busted chart legends): https://screenshot.googleplex.com/MNsj7E7dTer.png See also new hotness chart legends: https://codereview.chromium.org/2124113002 ==========
Patchset #1 (id:120001) has been deleted
https://codereview.chromium.org/2133963002/diff/140001/tracing/tracing/metric... File tracing/tracing/metrics/system_health/long_tasks_metric.html (right): https://codereview.chromium.org/2133963002/diff/140001/tracing/tracing/metric... tracing/tracing/metrics/system_health/long_tasks_metric.html:8: <link rel="import" nit: there isn't an 80char limit in HTML, so just put this on one line. https://codereview.chromium.org/2133963002/diff/140001/tracing/tracing/metric... tracing/tracing/metrics/system_health/long_tasks_metric.html:73: var SLICE_NUMERIC_BUILDER = tr.v.NumericBuilder.createExponential( Why exponential? Comment perhaps? https://codereview.chromium.org/2133963002/diff/140001/tracing/tracing/value/... File tracing/tracing/value/diagnostics/composition.html (right): https://codereview.chromium.org/2133963002/diff/140001/tracing/tracing/value/... tracing/tracing/value/diagnostics/composition.html:36: Composition.build = function( I'm a little confused -- why do you not simply put this code in the constructor?
https://codereview.chromium.org/2133963002/diff/140001/tracing/tracing/value/... File tracing/tracing/value/diagnostics/composition.html (right): https://codereview.chromium.org/2133963002/diff/140001/tracing/tracing/value/... tracing/tracing/value/diagnostics/composition.html:25: * to their Value's diagnostics. Clarify that Compositions can be built over sets of other kinds of things, such as processes, threads, v8 samples, etc.
Spoke to Ben offline about my questions; lgtm
https://codereview.chromium.org/2133963002/diff/140001/tracing/tracing/value/... File tracing/tracing/value/diagnostics/composition.html (right): https://codereview.chromium.org/2133963002/diff/140001/tracing/tracing/value/... tracing/tracing/value/diagnostics/composition.html:27: * @param {!tr.v.ValueSet} values Why having values in the param and not just let the callsite add this to value set themselves? https://codereview.chromium.org/2133963002/diff/140001/tracing/tracing/value/... tracing/tracing/value/diagnostics/composition.html:28: * @param {string} namePrefix I think we don't need this namePrefix since all of these values are generated as a composition for existing value.
Description was changed from ========== Implement Composition.build() Currently, there is no example of how to use Composition. This CL implements a static method that demonstrates how to build a Composition from an EventSet, with an example caller in long_tasks_metric using UserFriendlyCategoryDriver. Screenshot (google-only, feat. old busted chart legends): https://screenshot.googleplex.com/MNsj7E7dTer.png See also new hotness chart legends: https://codereview.chromium.org/2124113002 ========== to ========== Implement Composition.buildFromEvents() Currently, there is no example of how to use Composition. This CL implements a static method that demonstrates how to build a Composition from an EventSet, with an example caller in long_tasks_metric using UserFriendlyCategoryDriver. Screenshot (google-only, feat. old busted chart legends): https://screenshot.googleplex.com/MNsj7E7dTer.png See also new hotness chart legends: https://codereview.chromium.org/2124113002 ==========
Description was changed from ========== Implement Composition.buildFromEvents() Currently, there is no example of how to use Composition. This CL implements a static method that demonstrates how to build a Composition from an EventSet, with an example caller in long_tasks_metric using UserFriendlyCategoryDriver. Screenshot (google-only, feat. old busted chart legends): https://screenshot.googleplex.com/MNsj7E7dTer.png See also new hotness chart legends: https://codereview.chromium.org/2124113002 ========== to ========== Implement Composition.buildFromEvents() Currently, there is no example of how to use Composition. This CL implements a static method that demonstrates how to build a Composition from an EventSet, with an example caller in long_tasks_metric using UserFriendlyCategoryDriver. Screenshot (google-only): https://screenshot.googleplex.com/dWZz92Mxm7X.png ==========
Patchset #3 (id:180001) has been deleted
Patchset #2 (id:160001) has been deleted
Ned: PTAL. Sorry I had to fix a bunch of random crap to get it to work. https://codereview.chromium.org/2133963002/diff/140001/tracing/tracing/value/... File tracing/tracing/value/diagnostics/composition.html (right): https://codereview.chromium.org/2133963002/diff/140001/tracing/tracing/value/... tracing/tracing/value/diagnostics/composition.html:27: * @param {!tr.v.ValueSet} values On 2016/07/12 at 17:46:24, nednguyen wrote: > Why having values in the param and not just let the callsite add this to value set themselves? It seemed like it would be burdensome and error-prone to require callers to iterate over the returned Composition and manually add the composing NumericValues to the ValueSet. https://codereview.chromium.org/2133963002/diff/140001/tracing/tracing/value/... tracing/tracing/value/diagnostics/composition.html:28: * @param {string} namePrefix On 2016/07/12 at 17:46:24, nednguyen wrote: > I think we don't need this namePrefix since all of these values are generated as a composition for existing value. I suppose this function could take the parent Value and the name of the new Composition diagnostic, and automatically add the new Composition diagnostic to the parent Value's diagnostics, and automatically name the composing NumericValues after the parent Value, but that seems like an unnecessary restriction. Some metrics might want to use different naming schemes, different name separators (space vs colon vs dash). Explicit is better than implicit. General is better than special, though too general requires another layer of tools.
There are some miscellaneous cleanups that are also part of the latest patch set that Ben has addressed with me in person, and I think they're all okay in this CL.
Description was changed from ========== Implement Composition.buildFromEvents() Currently, there is no example of how to use Composition. This CL implements a static method that demonstrates how to build a Composition from an EventSet, with an example caller in long_tasks_metric using UserFriendlyCategoryDriver. Screenshot (google-only): https://screenshot.googleplex.com/dWZz92Mxm7X.png ========== to ========== Implement Composition.buildFromEvents() Currently, there is no example of how to use Composition. This CL implements a static method that demonstrates how to build a Composition from an EventSet, with an example caller in long_tasks_metric using UserFriendlyCategoryDriver. Getting this to look nice required: making Chart margins easier to deal with so that embedders could customize them, fixing a style bug in tr-ui-b-table (vertical-align: text-top != top), Screenshot (google-only): https://screenshot.googleplex.com/dWZz92Mxm7X.png ==========
Description was changed from ========== Implement Composition.buildFromEvents() Currently, there is no example of how to use Composition. This CL implements a static method that demonstrates how to build a Composition from an EventSet, with an example caller in long_tasks_metric using UserFriendlyCategoryDriver. Getting this to look nice required: making Chart margins easier to deal with so that embedders could customize them, fixing a style bug in tr-ui-b-table (vertical-align: text-top != top), Screenshot (google-only): https://screenshot.googleplex.com/dWZz92Mxm7X.png ========== to ========== Implement Composition.buildFromEvents() Currently, there is no example of how to use Composition. This CL implements a static method that demonstrates how to build a Composition from an EventSet, with an example caller in long_tasks_metric using UserFriendlyCategoryDriver. Getting this to look nice required: making Chart margins easier to deal with so that embedders could customize them, fixing a style bug in tr-ui-b-table (vertical-align: text-top != top), fixing a style bug in value-set-table (charts assume text-align: left), Screenshot (google-only): https://screenshot.googleplex.com/dWZz92Mxm7X.png ==========
Description was changed from ========== Implement Composition.buildFromEvents() Currently, there is no example of how to use Composition. This CL implements a static method that demonstrates how to build a Composition from an EventSet, with an example caller in long_tasks_metric using UserFriendlyCategoryDriver. Getting this to look nice required: making Chart margins easier to deal with so that embedders could customize them, fixing a style bug in tr-ui-b-table (vertical-align: text-top != top), fixing a style bug in value-set-table (charts assume text-align: left), Screenshot (google-only): https://screenshot.googleplex.com/dWZz92Mxm7X.png ========== to ========== Implement Composition.buildFromEvents() Currently, there is no example of how to use Composition. This CL implements a static method that demonstrates how to build a Composition from an EventSet, with an example caller in long_tasks_metric using UserFriendlyCategoryDriver. Getting this to look nice required: making Chart margins easier to deal with so that embedders could customize them, fixing a style bug in tr-ui-b-table (vertical-align: text-top != top), fixing a style bug in value-set-table (charts assume text-align: left), updating composition-span to use the new Chart APIs, Screenshot (google-only): https://screenshot.googleplex.com/dWZz92Mxm7X.png ==========
Description was changed from ========== Implement Composition.buildFromEvents() Currently, there is no example of how to use Composition. This CL implements a static method that demonstrates how to build a Composition from an EventSet, with an example caller in long_tasks_metric using UserFriendlyCategoryDriver. Getting this to look nice required: making Chart margins easier to deal with so that embedders could customize them, fixing a style bug in tr-ui-b-table (vertical-align: text-top != top), fixing a style bug in value-set-table (charts assume text-align: left), updating composition-span to use the new Chart APIs, Screenshot (google-only): https://screenshot.googleplex.com/dWZz92Mxm7X.png ========== to ========== Implement Composition.buildFromEvents() Currently, there is no example of how to use Composition. This CL implements a static method that demonstrates how to build a Composition from an EventSet, with an example caller in long_tasks_metric using UserFriendlyCategoryDriver. Getting this to look nice required: making Chart margins easier to deal with so that embedders could customize them, fixing a style bug in tr-ui-b-table (vertical-align: text-top != top), fixing a style bug in value-set-table (charts assume text-align: left), updating composition-span to use the new Chart APIs, adding NumericBuilder.createExponential() to mirror createLinear(), Screenshot (google-only): https://screenshot.googleplex.com/dWZz92Mxm7X.png ==========
benjhayden@chromium.org changed reviewers: + charliea@chromium.org
+Charliea, FYI for a bunch of cleanups. Sorry for short notice, Charlie and Ned, but I'm going to have to commit this tomorrow. I still have a bunch of things that I need to burn down on the roadmap.
Description was changed from ========== Implement Composition.buildFromEvents() Currently, there is no example of how to use Composition. This CL implements a static method that demonstrates how to build a Composition from an EventSet, with an example caller in long_tasks_metric using UserFriendlyCategoryDriver. Getting this to look nice required: making Chart margins easier to deal with so that embedders could customize them, fixing a style bug in tr-ui-b-table (vertical-align: text-top != top), fixing a style bug in value-set-table (charts assume text-align: left), updating composition-span to use the new Chart APIs, adding NumericBuilder.createExponential() to mirror createLinear(), Screenshot (google-only): https://screenshot.googleplex.com/dWZz92Mxm7X.png ========== to ========== Implement Composition.buildFromEvents() Currently, there is no example of how to use Composition. This CL implements a static method that demonstrates how to build a Composition from an EventSet, with an example caller in long_tasks_metric using UserFriendlyCategoryDriver. Getting this to look nice required: making Chart margins easier to deal with so that embedders could customize them, fixing a style bug in tr-ui-b-table (vertical-align: text-top != top), fixing a style bug in value-set-table (charts assume text-align: left), updating composition-span to use the new Chart APIs, adding NumericBuilder.createExponential() to mirror createLinear(). Screenshot (google-only): https://screenshot.googleplex.com/dWZz92Mxm7X.png ==========
lgtm with some comments https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... File tracing/tracing/value/diagnostics/composition.html (right): https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... tracing/tracing/value/diagnostics/composition.html:33: * @param {string} namePrefix I still think this namePrefix is redundant. If people want this, they can just modify their categoryForEvent(e) function so that it returns <namePrefix> + category(e) instead https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... tracing/tracing/value/diagnostics/composition.html:41: Composition.buildFromEvents = function( s/buildFromEvents/buildFromEventCategories ? I don't have a strong opinion on this naming though, so you can just keep the name the same.
Thanks! https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... File tracing/tracing/value/diagnostics/composition.html (right): https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... tracing/tracing/value/diagnostics/composition.html:33: * @param {string} namePrefix On 2016/07/13 at 02:17:32, nednguyen wrote: > I still think this namePrefix is redundant. If people want this, they can just modify their categoryForEvent(e) function so that it returns <namePrefix> + category(e) instead The category isn't only used for the composing Value name, it's also used as the Composition's key to the Value, which is displayed as a link in the chart-base-legend-key. So categoryForEvent returns something like 'style' or 'script', namePrefix is something like 'long tasks ', so the composing value names are 'long tasks style', etc., but it would be redundant to display 'long tasks style', 'long tasks script', 'long tasks paint' in the legend-keys, not to mention they'd all be elided at 'long tasks...'. Alternatively, if the Value names are just the eventCategory strings with no prefix, then there may be multiple Values named 'script', but one of them might be for long tasks, another for loads, etc. but the Value names should indicate that distinction. Hence namePrefix. Does that help?
Thanks! https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... File tracing/tracing/value/diagnostics/composition.html (right): https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... tracing/tracing/value/diagnostics/composition.html:33: * @param {string} namePrefix On 2016/07/13 at 02:17:32, nednguyen wrote: > I still think this namePrefix is redundant. If people want this, they can just modify their categoryForEvent(e) function so that it returns <namePrefix> + category(e) instead The category isn't only used for the composing Value name, it's also used as the Composition's key to the Value, which is displayed as a link in the chart-base-legend-key. So categoryForEvent returns something like 'style' or 'script', namePrefix is something like 'long tasks ', so the composing value names are 'long tasks style', etc., but it would be redundant to display 'long tasks style', 'long tasks script', 'long tasks paint' in the legend-keys, not to mention they'd all be elided at 'long tasks...'. Alternatively, if the Value names are just the eventCategory strings with no prefix, then there may be multiple Values named 'script', but one of them might be for long tasks, another for loads, etc. but the Value names should indicate that distinction. Hence namePrefix. Does that help?
Thanks for your explanation, it makes sense. lgtm https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... File tracing/tracing/value/diagnostics/composition.html (right): https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... tracing/tracing/value/diagnostics/composition.html:33: * @param {string} namePrefix On 2016/07/13 04:41:28, benjhayden_chromium wrote: > On 2016/07/13 at 02:17:32, nednguyen wrote: > > I still think this namePrefix is redundant. If people want this, they can just > modify their categoryForEvent(e) function so that it returns <namePrefix> + > category(e) instead > > The category isn't only used for the composing Value name, it's also used as the > Composition's key to the Value, which is displayed as a link in the > chart-base-legend-key. > So categoryForEvent returns something like 'style' or 'script', namePrefix is > something like 'long tasks ', so the composing value names are 'long tasks > style', etc., but it would be redundant to display 'long tasks style', 'long > tasks script', 'long tasks paint' in the legend-keys, not to mention they'd all > be elided at 'long tasks...'. > > Alternatively, if the Value names are just the eventCategory strings with no > prefix, then there may be multiple Values named 'script', but one of them might > be for long tasks, another for loads, etc. but the Value names should indicate > that distinction. Hence namePrefix. > > Does that help? Hmhh, this makes sense but now I think about it, having value name & composition legend being two totally different entities might cause unnecessary confusion for people when it shows the composition's legend is "a b c" but when they click on "a b c", it directs them to value named "d e f". Can we somehow enforce the legend name must be a suffix of value name? Probably in a different CL though.
https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/metric... File tracing/tracing/metrics/system_health/long_tasks_metric.html (right): https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/metric... tracing/tracing/metrics/system_health/long_tasks_metric.html:8: <link rel="import" nit: no need to break <link> tags apart over multiple lines https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/ui/bas... File tracing/tracing/ui/base/chart_base.html (right): https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/ui/bas... tracing/tracing/ui/base/chart_base.html:138: * /deep/ .chart-base .legend body { The /deep/ selector is deprecated. Do you know of a way to get around this? https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/ui/bas... File tracing/tracing/ui/base/chart_base_2d.html (right): https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/ui/bas... tracing/tracing/ui/base/chart_base_2d.html:188: tr.b.requestAnimationFrame(function() { You certainly don't have to fix it here (this CL makes things strictly better wrt. RAF), but I'm not sure I understand why we're using RAF here. This code looks suspiciously like someone was looking for a reason to try it out. https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... File tracing/tracing/value/diagnostics/composition.html (right): https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... tracing/tracing/value/diagnostics/composition.html:14: /** @constructor */ nit: might be useful in here to add that you should see buildFromEvents for actually building a composition. https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... tracing/tracing/value/diagnostics/composition.html:27: * Composition encapsulates a generic relationship between NumericValues that Generally, the jsdoc describing what a type is goes along with the constructor. Could you move the "Composition encapsulates a..." part of this comment up there? https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... tracing/tracing/value/diagnostics/composition.html:28: * can hold for groups of other things besides Events, such as memory I don't really have much context about Compositions here, but I'm not sure this doc is parsable by people like me that lack context. I can't quite grok what "a generic relationship between NumericValues that can hold for groups of other things besides Events" means. Is it like a bunch of NumericValues that are all associated somehow? Also, in this sentence, it says that it's a relationship between groups of other things besides events, but in the next sentence, it says that "composition over groups of events is expected to be the most common way of building Compositions". Superficially, those two things seem at odds with each other, but I'm pretty sure it's just because that lack of context. Could you maybe give a more concrete example in the doc about how compositions might be used to do something useful? https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... tracing/tracing/value/diagnostics/composition.html:46: eventCallback = function(event) { nit: can be (event) => event.cpuSelfTime https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... File tracing/tracing/value/diagnostics/composition_test.html (right): https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... tracing/tracing/value/diagnostics/composition_test.html:8: <link rel="import" nit: don't have to split over multiple lines https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... tracing/tracing/value/diagnostics/composition_test.html:26: var events = new tr.model.EventSet(); Can just do: var events = new tr.model.EventSet( { guid: 9, title: 'EvaluateScript', cpuSelfTime: 1, stableId: '11.11' }, { guid: 10, title: 'EvaluateScript', cpuSelfTime: 2, stableId: '22.22' }, ... }); https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... tracing/tracing/value/diagnostics/composition_test.html:59: assert.lengthOf(scriptValue.numeric.centralBins[1].diagnostics, 1); I think you might just be able to do: assert.includesDeepMembers( scriptValue.numeric.centralBins, [ { diagnostics: [events[0]] }, { diagnostics: [events[1]] } ]); Or, if you care about order: assert.deepEqual( scriptValue.numeric.centralBins[1], { diagnostics: [events[0] }); assert.deepEqual( scriptValue.numeric.centralBins[2], { diagnostics: [events[1] }); My high-level point is try to use chai's higher-level assertions (http://chaijs.com/api/assert/#method_includedeepmembers) to avoid lots of lengthOfs followed by strictEqual. https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... File tracing/tracing/value/ui/composition_span.html (right): https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... tracing/tracing/value/ui/composition_span.html:52: delta[name] = value; Can you add a note about why delta[name] = value; this.chart_.customizeLegendTargets(delta); delta[name] = true; is necessary? Without knowing anything else, it looks pretty strange.
Thanks! https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/ui/bas... File tracing/tracing/ui/base/chart_base.html (right): https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/ui/bas... tracing/tracing/ui/base/chart_base.html:138: * /deep/ .chart-base .legend body { On 2016/07/13 at 14:30:06, charliea wrote: > The /deep/ selector is deprecated. Do you know of a way to get around this? Maybe it's time for ChartBase to become a real polymer element? Another CL, for sure. https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... File tracing/tracing/value/diagnostics/composition.html (right): https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... tracing/tracing/value/diagnostics/composition.html:33: * @param {string} namePrefix On 2016/07/13 at 11:34:02, nednguyen wrote: > On 2016/07/13 04:41:28, benjhayden_chromium wrote: > > On 2016/07/13 at 02:17:32, nednguyen wrote: > > > I still think this namePrefix is redundant. If people want this, they can just > > modify their categoryForEvent(e) function so that it returns <namePrefix> + > > category(e) instead > > > > The category isn't only used for the composing Value name, it's also used as the > > Composition's key to the Value, which is displayed as a link in the > > chart-base-legend-key. > > So categoryForEvent returns something like 'style' or 'script', namePrefix is > > something like 'long tasks ', so the composing value names are 'long tasks > > style', etc., but it would be redundant to display 'long tasks style', 'long > > tasks script', 'long tasks paint' in the legend-keys, not to mention they'd all > > be elided at 'long tasks...'. > > > > Alternatively, if the Value names are just the eventCategory strings with no > > prefix, then there may be multiple Values named 'script', but one of them might > > be for long tasks, another for loads, etc. but the Value names should indicate > > that distinction. Hence namePrefix. > > > > Does that help? > > Hmhh, this makes sense but now I think about it, having value name & composition legend being two totally different entities might cause unnecessary confusion for people when it shows the composition's legend is "a b c" but when they click on "a b c", it directs them to value named "d e f". > > Can we somehow enforce the legend name must be a suffix of value name? Probably in a different CL though. Sorry, I'm doing mega-CLs this week. :-) This is a 2-line check-and-throw in Composition.set(), but it required changing the default value of ChromeUserFriendlyCategoryDriver.fromEvent() from undefined to 'unknown', FYI. https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... File tracing/tracing/value/diagnostics/composition_test.html (right): https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... tracing/tracing/value/diagnostics/composition_test.html:59: assert.lengthOf(scriptValue.numeric.centralBins[1].diagnostics, 1); On 2016/07/13 at 14:30:06, charliea wrote: > I think you might just be able to do: > > assert.includesDeepMembers( > scriptValue.numeric.centralBins, > [ > { diagnostics: [events[0]] }, > { diagnostics: [events[1]] } > ]); > > Or, if you care about order: > > assert.deepEqual( > scriptValue.numeric.centralBins[1], > { diagnostics: [events[0] }); > > assert.deepEqual( > scriptValue.numeric.centralBins[2], > { diagnostics: [events[1] }); > > My high-level point is try to use chai's higher-level assertions (http://chaijs.com/api/assert/#method_includedeepmembers) to avoid lots of lengthOfs followed by strictEqual. I find deepEqual's error message useless. lengthOf and specific strictEquals are more verbose, but much easier to diagnose. https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... File tracing/tracing/value/ui/composition_span.html (right): https://codereview.chromium.org/2133963002/diff/220001/tracing/tracing/value/... tracing/tracing/value/ui/composition_span.html:52: delta[name] = value; On 2016/07/13 at 14:30:06, charliea wrote: > Can you add a note about why > > delta[name] = value; > this.chart_.customizeLegendTargets(delta); > delta[name] = true; > > is necessary? Without knowing anything else, it looks pretty strange. I changed these to direct calls to getDataSeries(name).target/optional.
Patchset #4 (id:240001) has been deleted
Patchset #4 (id:260001) has been deleted
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, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2133963002/#ps280001 (title: ".")
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 ========== Implement Composition.buildFromEvents() Currently, there is no example of how to use Composition. This CL implements a static method that demonstrates how to build a Composition from an EventSet, with an example caller in long_tasks_metric using UserFriendlyCategoryDriver. Getting this to look nice required: making Chart margins easier to deal with so that embedders could customize them, fixing a style bug in tr-ui-b-table (vertical-align: text-top != top), fixing a style bug in value-set-table (charts assume text-align: left), updating composition-span to use the new Chart APIs, adding NumericBuilder.createExponential() to mirror createLinear(). Screenshot (google-only): https://screenshot.googleplex.com/dWZz92Mxm7X.png ========== to ========== Implement Composition.buildFromEvents() Currently, there is no example of how to use Composition. This CL implements a static method that demonstrates how to build a Composition from an EventSet, with an example caller in long_tasks_metric using UserFriendlyCategoryDriver. Getting this to look nice required: making Chart margins easier to deal with so that embedders could customize them, fixing a style bug in tr-ui-b-table (vertical-align: text-top != top), fixing a style bug in value-set-table (charts assume text-align: left), updating composition-span to use the new Chart APIs, adding NumericBuilder.createExponential() to mirror createLinear(). Screenshot (google-only): https://screenshot.googleplex.com/dWZz92Mxm7X.png Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:280001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Message was sent while issue was closed.
CQ bit was unchecked. |
