Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2046553003:
Add RelatedEventSet Diagnostic (Closed)
|
Created:
4 years, 6 months ago by benjhayden Modified:
4 years, 6 months ago CC:
catapult-reviews_chromium.org, charliea (OOO until 10-5), tracing-review_chromium.org Base URL:
https://github.com/catapult-project/catapult.git@master Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
DescriptionAdd RelatedEventSet Diagnostic
Currently, metrics can define diagnostics that encode relationships between
Values and other Values.
Some metrics might want to encode relationships between Values and Events in
traces.
This CL adds a new type of diagnostic that wraps an EventSet.
Events are serialized by their stableIds, which can be resolved using the new
Model.getEventByStableId(stableId) method.
An event-set-span is left for another CL.
Turning Numeric (histogram) srcInfos into diagnostics is left for another CL.
BUG=catapult:#2180
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/f1e892068d99ea3ddac0ccf0ade4857be70a5a1b
Patch Set 1 : . #
Total comments: 1
Patch Set 2 : RelatedEventSet #
Total comments: 17
Patch Set 3 : comments #Patch Set 4 : comments #Patch Set 5 : blind rewrite #Patch Set 6 : rebase #
Total comments: 1
Patch Set 7 : comments, rebase #
Messages
Total messages: 35 (10 generated)
Description was changed from ========== Add EventSet Diagnostic Currently, metrics can define diagnostics that encode relationships between Values and other Values. Some metrics might want to encode relationships between Values and Events in traces. This CL adds a new type of diagnostic that wraps an EventSet. BUG=catapult:#2180 ========== to ========== Add EventSet Diagnostic Currently, metrics can define diagnostics that encode relationships between Values and other Values. Some metrics might want to encode relationships between Values and Events in traces. This CL adds a new type of diagnostic that wraps an EventSet. An event-set-span is left for another CL. Turning Numeric (histogram) srcInfos into diagnostics is left for another CL. BUG=catapult:#2180 ==========
Description was changed from ========== Add EventSet Diagnostic Currently, metrics can define diagnostics that encode relationships between Values and other Values. Some metrics might want to encode relationships between Values and Events in traces. This CL adds a new type of diagnostic that wraps an EventSet. An event-set-span is left for another CL. Turning Numeric (histogram) srcInfos into diagnostics is left for another CL. BUG=catapult:#2180 ========== to ========== Add EventSet Diagnostic Currently, metrics can define diagnostics that encode relationships between Values and other Values. Some metrics might want to encode relationships between Values and Events in traces. This CL adds a new type of diagnostic that wraps an EventSet. Events are serialized by their stableIds, which can be resolved using the new Model.getEventByStableId(stableId) method. An event-set-span is left for another CL. Turning Numeric (histogram) srcInfos into diagnostics is left for another CL. BUG=catapult:#2180 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
benjhayden@chromium.org changed reviewers: + eakuefner@chromium.org, nednguyen@google.com
PTAL
https://codereview.chromium.org/2046553003/diff/40001/tracing/tracing/value/d... File tracing/tracing/value/diagnostics/event_set.html (right): https://codereview.chromium.org/2046553003/diff/40001/tracing/tracing/value/d... tracing/tracing/value/diagnostics/event_set.html:34: function EventSet(opt_events) { We already have tracing/tracing/model/event_set.html. Should we merge the two?
On 2016/06/06 at 22:50:47, nednguyen wrote: > https://codereview.chromium.org/2046553003/diff/40001/tracing/tracing/value/d... > File tracing/tracing/value/diagnostics/event_set.html (right): > > https://codereview.chromium.org/2046553003/diff/40001/tracing/tracing/value/d... > tracing/tracing/value/diagnostics/event_set.html:34: function EventSet(opt_events) { > We already have tracing/tracing/model/event_set.html. Should we merge the two? It seems to me that diagnostics logic should stay in value/diagnostics/ and not mingle with abstractions in model/. tr.v.d.EventSet is basically a mix-in that mixes a tr.model.EventSet with the specific kind of serialization particular to diagnostics. I understand that having two classes with the same name but in different namespaces can be considered confusing, but merging diagnostic serialization into tr.model.EventSet smells funny to me. Did you have any concerns besides the similar names? Did you want to merge all values and diagnostics into model/?
On 2016/06/06 23:09:36, benjhayden_chromium wrote: > On 2016/06/06 at 22:50:47, nednguyen wrote: > > > https://codereview.chromium.org/2046553003/diff/40001/tracing/tracing/value/d... > > File tracing/tracing/value/diagnostics/event_set.html (right): > > > > > https://codereview.chromium.org/2046553003/diff/40001/tracing/tracing/value/d... > > tracing/tracing/value/diagnostics/event_set.html:34: function > EventSet(opt_events) { > > We already have tracing/tracing/model/event_set.html. Should we merge the two? > > It seems to me that diagnostics logic should stay in value/diagnostics/ and not > mingle with abstractions in model/. > tr.v.d.EventSet is basically a mix-in that mixes a tr.model.EventSet with the > specific kind of serialization particular to diagnostics. > I understand that having two classes with the same name but in different > namespaces can be considered confusing, but merging diagnostic serialization > into tr.model.EventSet smells funny to me. > Did you have any concerns besides the similar names? > Did you want to merge all values and diagnostics into model/? I see. What about rename this to RelatedEvents?
On 2016/06/06 23:12:23, nednguyen wrote: > On 2016/06/06 23:09:36, benjhayden_chromium wrote: > > On 2016/06/06 at 22:50:47, nednguyen wrote: > > > > > > https://codereview.chromium.org/2046553003/diff/40001/tracing/tracing/value/d... > > > File tracing/tracing/value/diagnostics/event_set.html (right): > > > > > > > > > https://codereview.chromium.org/2046553003/diff/40001/tracing/tracing/value/d... > > > tracing/tracing/value/diagnostics/event_set.html:34: function > > EventSet(opt_events) { > > > We already have tracing/tracing/model/event_set.html. Should we merge the > two? > > > > It seems to me that diagnostics logic should stay in value/diagnostics/ and > not > > mingle with abstractions in model/. > > tr.v.d.EventSet is basically a mix-in that mixes a tr.model.EventSet with the > > specific kind of serialization particular to diagnostics. > > I understand that having two classes with the same name but in different > > namespaces can be considered confusing, but merging diagnostic serialization > > into tr.model.EventSet smells funny to me. > > Did you have any concerns besides the similar names? > > Did you want to merge all values and diagnostics into model/? > > I see. What about rename this to RelatedEvents? *RelatedEventsSet
Description was changed from ========== Add EventSet Diagnostic Currently, metrics can define diagnostics that encode relationships between Values and other Values. Some metrics might want to encode relationships between Values and Events in traces. This CL adds a new type of diagnostic that wraps an EventSet. Events are serialized by their stableIds, which can be resolved using the new Model.getEventByStableId(stableId) method. An event-set-span is left for another CL. Turning Numeric (histogram) srcInfos into diagnostics is left for another CL. BUG=catapult:#2180 ========== to ========== Add RelatedEventSet Diagnostic Currently, metrics can define diagnostics that encode relationships between Values and other Values. Some metrics might want to encode relationships between Values and Events in traces. This CL adds a new type of diagnostic that wraps an EventSet. Events are serialized by their stableIds, which can be resolved using the new Model.getEventByStableId(stableId) method. An event-set-span is left for another CL. Turning Numeric (histogram) srcInfos into diagnostics is left for another CL. BUG=catapult:#2180 ==========
PTAL :-)
https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... File tracing/tracing/value/diagnostics/related_event_set.html (right): https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... tracing/tracing/value/diagnostics/related_event_set.html:38: RelatedEventSet.prototype = { this class seems to only have 3 methods: resolve, fromDict & asDict. Shouldn't it need to expose the internal events set to be more useful? https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... tracing/tracing/value/diagnostics/related_event_set.html:68: this.events = resolvedEvents; hmhh, this.events is not strongly typed? I thought our js code base is C++ish? https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... tracing/tracing/value/diagnostics/related_event_set.html:71: _asDictInto: function(d) { who call this & why is this private? Also I think the convention for naming private method in tracing/ code base is asDictInfo_ https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... tracing/tracing/value/diagnostics/related_event_set.html:91: EventRef: EventRef nits: hide EventRef since it doesn't need to be used outside of this https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... File tracing/tracing/value/diagnostics/related_event_set_test.html (right): https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... tracing/tracing/value/diagnostics/related_event_set_test.html:30: console.debug(d.events[0]); Remove debug print
https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... File tracing/tracing/value/diagnostics/related_event_set.html (right): https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... tracing/tracing/value/diagnostics/related_event_set.html:38: RelatedEventSet.prototype = { On 2016/06/07 at 00:38:50, nednguyen wrote: > this class seems to only have 3 methods: resolve, fromDict & asDict. Shouldn't it need to expose the internal events set to be more useful? The constructor sets a field named 'events' that contains the EventSet. The field is named 'events' without underscores. Field names without underscores indicate that the field is public. If the field name had an underscore, then it would be private, and callers could not access it without a getter. But the field is public, so callers can access it even though there is no getter. The constructor sets the EventSet field named 'events', which is public, right? I know I'm crazy, but I think I'm still pretty sure of that? :-) https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... tracing/tracing/value/diagnostics/related_event_set.html:68: this.events = resolvedEvents; On 2016/06/07 at 00:38:50, nednguyen wrote: > hmhh, this.events is not strongly typed? I thought our js code base is C++ish? Why do you think it isn't strongly typed? It's always an EventSet. https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... tracing/tracing/value/diagnostics/related_event_set.html:71: _asDictInto: function(d) { On 2016/06/07 at 00:38:50, nednguyen wrote: > who call this & why is this private? You reviewed the Diagnostic CL last week: https://codereview.chromium.org/2000063006 :-) > > Also I think the convention for naming private method in tracing/ code base is asDictInfo_ I copied this from Value. Feel free to change it in another CL.
https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... File tracing/tracing/value/diagnostics/related_event_set.html (right): https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... tracing/tracing/value/diagnostics/related_event_set.html:38: RelatedEventSet.prototype = { On 2016/06/07 01:21:33, benjhayden_chromium wrote: > On 2016/06/07 at 00:38:50, nednguyen wrote: > > this class seems to only have 3 methods: resolve, fromDict & asDict. Shouldn't > it need to expose the internal events set to be more useful? > > The constructor sets a field named 'events' that contains the EventSet. The > field is named 'events' without underscores. Field names without underscores > indicate that the field is public. If the field name had an underscore, then it > would be private, and callers could not access it without a getter. But the > field is public, so callers can access it even though there is no getter. > > The constructor sets the EventSet field named 'events', which is public, right? > I know I'm crazy, but I think I'm still pretty sure of that? :-) I think the convention in tracing code base is to make all members private, then for ones you want to publicize, you add the getters. https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... tracing/tracing/value/diagnostics/related_event_set.html:68: this.events = resolvedEvents; On 2016/06/07 01:21:33, benjhayden_chromium wrote: > On 2016/06/07 at 00:38:50, nednguyen wrote: > > hmhh, this.events is not strongly typed? I thought our js code base is C++ish? > > Why do you think it isn't strongly typed? It's always an EventSet. I meant it contains a mixed of Event & EventRef types.
nduca@chromium.org changed reviewers: + nduca@chromium.org
what specific use case?
Sorry, my comment was vague. Can you please make sure Ethan supports this CL?
On 2016/06/07 at 15:20:18, nduca wrote: > what specific use case? This blocks turning Numeric srcInfos into strongly-typed Diagnostics. Metrics frequently add samples to histograms where each sample is derived from an event. Some metrics are already serializing stableIds in srcInfos; this CL is just one step towards formalizing that. There's a step in the debugging process when a user is looking at a histogram in the metrics side panel or results2.html or the dashboard, and wants to find the events that fall into particular bins in the context of their traces. It's the deep-trace-link concept from perf-insights.
On 2016/06/07 at 15:46:27, nduca wrote: > Sorry, my comment was vague. Can you please make sure Ethan supports this CL? Yes, he is a Reviewer. He was off yesterday, but I will wait for his lg.
https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... File tracing/tracing/value/diagnostics/related_event_set.html (right): https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... tracing/tracing/value/diagnostics/related_event_set.html:38: RelatedEventSet.prototype = { On 2016/06/07 at 01:31:15, nednguyen wrote: > On 2016/06/07 01:21:33, benjhayden_chromium wrote: > > On 2016/06/07 at 00:38:50, nednguyen wrote: > > > this class seems to only have 3 methods: resolve, fromDict & asDict. Shouldn't > > it need to expose the internal events set to be more useful? > > > > The constructor sets a field named 'events' that contains the EventSet. The > > field is named 'events' without underscores. Field names without underscores > > indicate that the field is public. If the field name had an underscore, then it > > would be private, and callers could not access it without a getter. But the > > field is public, so callers can access it even though there is no getter. > > > > The constructor sets the EventSet field named 'events', which is public, right? > > I know I'm crazy, but I think I'm still pretty sure of that? :-) > > I think the convention in tracing code base is to make all members private, then for ones you want to publicize, you add the getters. That is not a hard-and-fast rule. Private members are a common pattern, but public members are also quite common. The reason that I made this |events| public is so that RelatedEventSet would not need to duplicate the entire EventSet API. RelatedEventSet's usage of its EventSet is not so sensitive that it needs protection. Callers can generally do anything they want to |events|. Is there a particular reason that you think that this particular field should be private? https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... tracing/tracing/value/diagnostics/related_event_set.html:68: this.events = resolvedEvents; On 2016/06/07 at 01:31:15, nednguyen wrote: > On 2016/06/07 01:21:33, benjhayden_chromium wrote: > > On 2016/06/07 at 00:38:50, nednguyen wrote: > > > hmhh, this.events is not strongly typed? I thought our js code base is C++ish? > > > > Why do you think it isn't strongly typed? It's always an EventSet. > > I meant it contains a mixed of Event & EventRef types. In C++, there are FakeThings and MockThings that are still Things. Is there a specific problem? Is there an alternative?
https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... File tracing/tracing/value/diagnostics/related_event_set.html (right): https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... tracing/tracing/value/diagnostics/related_event_set.html:38: RelatedEventSet.prototype = { We typically do the private + getter, actually. The only time we do public methods is when there is a performance reason to do so, because getters are non-free. Which is the case in the slices system for instance. You could ask charlie to add this to the conventions doc. https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... tracing/tracing/value/diagnostics/related_event_set.html:68: this.events = resolvedEvents; You're putting EventRefs into an EventSet? That seems ... scary? https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... tracing/tracing/value/diagnostics/related_event_set.html:71: _asDictInto: function(d) { I think you should just do it the right way here. Reviewers do their best but they are imperfect, and we should always strive to make our code as good as we can.
+Charlie, see below. https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... File tracing/tracing/value/diagnostics/related_event_set.html (right): https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... tracing/tracing/value/diagnostics/related_event_set.html:38: RelatedEventSet.prototype = { On 2016/06/08 at 18:51:34, nduca wrote: > We typically do the private + getter, actually. The only time we do public methods is when there is a performance reason to do so, because getters are non-free. Which is the case in the slices system for instance. You could ask charlie to add this to the conventions doc. Charlie, can we do this? Is the conventions doc Nat's talking about the style guide? https://github.com/catapult-project/catapult/blob/master/docs/style-guide.md https://codereview.chromium.org/2046553003/diff/60001/tracing/tracing/value/d... tracing/tracing/value/diagnostics/related_event_set.html:71: _asDictInto: function(d) { On 2016/06/08 at 18:51:34, nduca wrote: > I think you should just do it the right way here. Reviewers do their best but they are imperfect, and we should always strive to make our code as good as we can. +1 to doing it right here, that's to say putting the underscores after rather than before. We should fix the remaining preceding underscores in a different CL. Also, just to be entirely clear, Info is a typo in Ned's comment, it's "Into" because you're inserting extra fields into the dict. I wrote a bit on https://github.com/catapult-project/catapult/issues/1961 about this -- we should encode the convention. Also, Charlie, this is another example where we could probably use an eslint rule to knock this problem dead, at least in JS source code.
PTAL
I think I rewrote this to avoid the things that you didn't like. It would be awesome if I could get some constructive criticism. I'm happy to do whatever you want, though ideally I'd like to understand why you want something. After this CL, I can change Numeric srcInfos into strongly-typed diagnostics, which will then enable some neat user experiences in the side panel, results2.html, and dashboard, similar to what we saw with perf-insights. Thanks! :-)
I'm happy with this patch, but I just want a little bit more clarity on why we need EventRef. Ben, let's talk tomorrow and if you can draw some pictures for me, I'll stamp this.
On 2016/06/13 at 23:01:01, eakuefner wrote:
> I'm happy with this patch, but I just want a little bit more clarity on why we
need EventRef. Ben, let's talk tomorrow and if you can draw some pictures for
me, I'll stamp this.
EventRef is the same concept as ValueRef in RelatedValueSet/Map. They're
placeholders, in case the referenced Value/Event isn't available in memory to
point to directly.
It's what Nat was talking to you, Ned, and me about in my office a week or two
ago about avoiding loading all of the entire ValueSets from all time for every
timeseries chart.
ValueRef and EventRef placeholders allow a Related{Value,Event}{Set,Map}
diagnostic to be deserialized without requiring the full {ValueSet,Model} to
have been loaded in memory.
Users can leave them as placeholders until, for example, the user clicks a link
to load the related Value or Model.
I might not have formalized ValueRef/EventRef as named classes (I might have
left them anonymous javascript objects) except that Nat had already given us the
name "ValueRef", which is a perfectly good name.
I'm not certain that ValueRef and EventRef serialize the right fields from their
respective Values/Events. We might eventually want to serialize more information
for each ValueRef and EventRef. After summarization, we might want to serialize
the value name in ValueRef to facilitate the dashboard. We might want to
serialize event args for EventRef, or args for only some Event types, in order
to facilitate even more advanced data exploration UXs on the dashboard. Imagine
having a {single,multi}-event-sub-view on the dashboard that responds when you
brush a histogram. Or we might decide that EventRefs really don't need any more
information than the stableId.
Does that help?
lgtm with nits https://codereview.chromium.org/2046553003/diff/140001/tracing/tracing/value/... File tracing/tracing/value/diagnostics/diagnostic_map.html (right): https://codereview.chromium.org/2046553003/diff/140001/tracing/tracing/value/... tracing/tracing/value/diagnostics/diagnostic_map.html:10: <link rel="import" href="/tracing/value/diagnostics/related_event_set.html"> Add some comment explain why these imports are needed.
lgtm
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/2046553003/#ps160001 (title: "comments, rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2046553003/160001
Message was sent while issue was closed.
Description was changed from ========== Add RelatedEventSet Diagnostic Currently, metrics can define diagnostics that encode relationships between Values and other Values. Some metrics might want to encode relationships between Values and Events in traces. This CL adds a new type of diagnostic that wraps an EventSet. Events are serialized by their stableIds, which can be resolved using the new Model.getEventByStableId(stableId) method. An event-set-span is left for another CL. Turning Numeric (histogram) srcInfos into diagnostics is left for another CL. BUG=catapult:#2180 ========== to ========== Add RelatedEventSet Diagnostic Currently, metrics can define diagnostics that encode relationships between Values and other Values. Some metrics might want to encode relationships between Values and Events in traces. This CL adds a new type of diagnostic that wraps an EventSet. Events are serialized by their stableIds, which can be resolved using the new Model.getEventByStableId(stableId) method. An event-set-span is left for another CL. Turning Numeric (histogram) srcInfos into diagnostics is left for another CL. BUG=catapult:#2180 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
