Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2989143002:
Plumb SparseDiagnostics by Name (Closed)
DescriptionPlumb SparseDiagnostics by Name
This CL adds the creation of Sparse Diagnostics in add_histograms by using
a set of reserved names.
It also adds some of the logic so that when shared diagnostics of histograms
are imported, their name is checked against the reserved set, instead of
only using their type.
This addresses step 5 of the list of tasks to be done in bug 3507.
BUG=catapult:#3718, catapult:#3507
Review-Url: https://codereview.chromium.org/2989143002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/68c8e443039c742d3685516647cfef77def67d27
Review-Url: https://codereview.chromium.org/2989143002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/0efcc192805c88a3ab6347f26245e6d293d09b5f
Patch Set 1 #Patch Set 2 : Use reserved infos instead of hard coded names #
Total comments: 7
Patch Set 3 : Address Ben's comments #
Total comments: 5
Patch Set 4 : Remove None from possible name of Diagnostic #
Total comments: 1
Patch Set 5 : Raise Exception When Histograms Differ #
Total comments: 1
Patch Set 6 : Add Names all times #
Total comments: 1
Patch Set 7 : Add TODO #Patch Set 8 : Rebase onto master #
Messages
Total messages: 33 (12 generated)
Description was changed from ========== Plumb SparseDiagnostics Through the reserved names BUG=catapult:# ========== to ========== Plumb SparseDiagnostics Through the reserved names BUG=catapult:#3718 ==========
Description was changed from ========== Plumb SparseDiagnostics Through the reserved names BUG=catapult:#3718 ========== to ========== Plumb SparseDiagnostics by Name This CL adds the creation of Sparse Diagnostics in add_histograms by using a set of reserved names. It also adds some of the logic so that when shared diagnostics of histograms are imported, their name is checked against the reserved set, instead of only using their type. This addresses step 5 of the list of tasks to be done in bug 3507. BUG=catapult:#3718, #3507 ==========
phsilva@google.com changed reviewers: + benjhayden@chromium.org, eakuefner@chromium.org, simonhatch@chromium.org
Hi all, I've written this CL regarding using names (not just types) to create Sparse Diagnostics, which was previously used for DeviceInfo and Ownership (which are now groups of GenericSets instead). Let me know what you think. Thanks. https://codereview.chromium.org/2989143002/diff/20001/tracing/tracing/value/h... File tracing/tracing/value/histogram_set.py (right): https://codereview.chromium.org/2989143002/diff/20001/tracing/tracing/value/h... tracing/tracing/value/histogram_set.py:81: if (all_diagnostics.DIAGNOSTICS_BY_NAME.get(d.get('type')) or I added this check for import dictionaries because even though it was not directly related to plumbing sparse diagnostics, it made sense to be added now that the DIAGNOSTICS_BY_NAME field exists and having ImportDicts can be helpful for tests making use of the new field. Let me know if it makes more sense to send this in as a separate CL.
Thanks! https://codereview.chromium.org/2989143002/diff/20001/dashboard/dashboard/add... File dashboard/dashboard/add_histograms.py (right): https://codereview.chromium.org/2989143002/diff/20001/dashboard/dashboard/add... dashboard/dashboard/add_histograms.py:24: [reserved_infos.ARCHITECTURES.name, reserved_infos.BUG_COMPONENTS.name, Can you split these to 1 per line? https://codereview.chromium.org/2989143002/diff/20001/dashboard/dashboard/add... dashboard/dashboard/add_histograms.py:83: for hist in histograms: I think this loop should be merged with the previous loop. The goal is to create one SparseDiagnostic entity per shared diagnostic whose name is in SUITE_LEVEL_SPARSE_DIAGNOSTIC_NAMES. The tricky part is finding the name of each shared diagnostic. It looks like this loop currently creates multiple SparseDiagnostic entities per shared diagnostic. Ideally, we shouldn't handle any more histograms or diagnostics than absolutely necessary, so we should stop looking for names once we've found the names of all of the shared_diagnostics. Would something like this work? shared_diagnostics = set(histograms.shared_diagnostics) for hist in histograms: for name, diagnostic in hist.diagnostics.iteritems(): if diagnostic not in shared_diagnostics: continue shared_diagnostics.remove(diagnostic) if not shared_diagnostics: break if name not in SUITE_LEVEL_SPARSE_DIAGNOSTIC_NAMES and type(diag) not in SUITE_LEVEL_SPARSE_DIAGNOSTIC_TYPES: continue suite_level_sparse_diagnostic_entities.append( histogram.SparseDiagnostic( id=diag.guid, data=diag.AsDict(), test=suite_key, start_revision=revision, end_revision=sys.maxint, name=diagnostic_name)) if not shared_diagnostics: break https://codereview.chromium.org/2989143002/diff/20001/tracing/tracing/value/h... File tracing/tracing/value/histogram_set.py (right): https://codereview.chromium.org/2989143002/diff/20001/tracing/tracing/value/h... tracing/tracing/value/histogram_set.py:81: if (all_diagnostics.DIAGNOSTICS_BY_NAME.get(d.get('type')) or On 2017/07/31 at 19:24:03, phsilva wrote: > I added this check for import dictionaries because even though it was not directly related to plumbing sparse diagnostics, it made sense to be added now that the DIAGNOSTICS_BY_NAME field exists and having ImportDicts can be helpful for tests making use of the new field. > > Let me know if it makes more sense to send this in as a separate CL. This doesn't look necessary.
https://codereview.chromium.org/2989143002/diff/20001/dashboard/dashboard/add... File dashboard/dashboard/add_histograms.py (right): https://codereview.chromium.org/2989143002/diff/20001/dashboard/dashboard/add... dashboard/dashboard/add_histograms.py:83: for hist in histograms: Can you explain this loop to me, why are you looking for suite level diagnostics in individual histograms?
Description was changed from ========== Plumb SparseDiagnostics by Name This CL adds the creation of Sparse Diagnostics in add_histograms by using a set of reserved names. It also adds some of the logic so that when shared diagnostics of histograms are imported, their name is checked against the reserved set, instead of only using their type. This addresses step 5 of the list of tasks to be done in bug 3507. BUG=catapult:#3718, #3507 ========== to ========== Plumb SparseDiagnostics by Name This CL adds the creation of Sparse Diagnostics in add_histograms by using a set of reserved names. It also adds some of the logic so that when shared diagnostics of histograms are imported, their name is checked against the reserved set, instead of only using their type. This addresses step 5 of the list of tasks to be done in bug 3507. BUG=catapult:#3718, catapult:#3507 ==========
Thanks for the comments, Ben. I've addressed them in a new patch. On 2017/07/31 at 20:44:33, benjhayden wrote: > https://codereview.chromium.org/2989143002/diff/20001/dashboard/dashboard/add... > File dashboard/dashboard/add_histograms.py (right): > > https://codereview.chromium.org/2989143002/diff/20001/dashboard/dashboard/add... > dashboard/dashboard/add_histograms.py:24: [reserved_infos.ARCHITECTURES.name, reserved_infos.BUG_COMPONENTS.name, > Can you split these to 1 per line? > Done. > https://codereview.chromium.org/2989143002/diff/20001/dashboard/dashboard/add... > dashboard/dashboard/add_histograms.py:83: for hist in histograms: > I think this loop should be merged with the previous loop. > The goal is to create one SparseDiagnostic entity per shared diagnostic whose name is in SUITE_LEVEL_SPARSE_DIAGNOSTIC_NAMES. > The tricky part is finding the name of each shared diagnostic. > It looks like this loop currently creates multiple SparseDiagnostic entities per shared diagnostic. Ideally, we shouldn't handle any more histograms or diagnostics than absolutely necessary, so we should stop looking for names once we've found the names of all of the shared_diagnostics. > You're right that it would have caused a bug and added multiple diagnostics. I fixed it by adding a set and now we can only add a diagnostic with each name once. I was wondering if instead we should add them together? So if histogram1 uses diagnostic1 for 'owners' and then histogram2 uses diagnostic2 for 'owners' then the SparseDiagnostic created is the result of adding one to another instead of just the one. I've merged the two loops, at first I thought it made sense to keep them separate since iterated over the HistogramSet's diagnostics while the other iterated over histograms. With the new implementation we will catch all the shared diagnostics that are present in the histogram level, but ignore if they are not (eg: a HistogramSet with a shared diagnostic, but no histograms). > > https://codereview.chromium.org/2989143002/diff/20001/tracing/tracing/value/h... > File tracing/tracing/value/histogram_set.py (right): > > https://codereview.chromium.org/2989143002/diff/20001/tracing/tracing/value/h... > tracing/tracing/value/histogram_set.py:81: if (all_diagnostics.DIAGNOSTICS_BY_NAME.get(d.get('type')) or > > This doesn't look necessary. I removed that part.
On 2017/07/31 at 21:06:17, simonhatch wrote: > https://codereview.chromium.org/2989143002/diff/20001/dashboard/dashboard/add... > File dashboard/dashboard/add_histograms.py (right): > > https://codereview.chromium.org/2989143002/diff/20001/dashboard/dashboard/add... > dashboard/dashboard/add_histograms.py:83: for hist in histograms: > Can you explain this loop to me, why are you looking for suite level diagnostics in individual histograms? I needed to do it at the histogram level because the relationship between names and diagnostics is not available elsewhere. Diagnostics don't "know" their names so the names are not available at the HistogramSet.shared_diagnostics level.
https://codereview.chromium.org/2989143002/diff/40001/dashboard/dashboard/add... File dashboard/dashboard/add_histograms.py (right): https://codereview.chromium.org/2989143002/diff/40001/dashboard/dashboard/add... dashboard/dashboard/add_histograms.py:87: name not in diagnostic_names_added): This assumes that there's not more than 1 OWNERS/etc per upload. If that assumption turns out to be false, then this will fail to store subsequent OWNERS/etc, which will cause errors when we inevitably lookup those diagnostics by the guids in the histograms. Should this throw an exception if that assumption turns out to be false? https://codereview.chromium.org/2989143002/diff/40001/dashboard/dashboard/add... dashboard/dashboard/add_histograms.py:97: name=diagnostic_name)) I think we should avoid storing SparseDiagnostics with name=None. That seems like it would cause errors down the road.
https://codereview.chromium.org/2989143002/diff/20001/dashboard/dashboard/add... File dashboard/dashboard/add_histograms.py (right): https://codereview.chromium.org/2989143002/diff/20001/dashboard/dashboard/add... dashboard/dashboard/add_histograms.py:83: for hist in histograms: On 2017/07/31 at 21:06:16, shatch wrote: > Can you explain this loop to me, why are you looking for suite level diagnostics in individual histograms? I had to look them up there because the names to diagnostic relationship is only expressed at the histogram level. Diagnostics "don't know" their names so they are not available at the HistogramSet.shared_diagnostics level. https://codereview.chromium.org/2989143002/diff/40001/dashboard/dashboard/add... File dashboard/dashboard/add_histograms.py (right): https://codereview.chromium.org/2989143002/diff/40001/dashboard/dashboard/add... dashboard/dashboard/add_histograms.py:87: name not in diagnostic_names_added): On 2017/07/31 at 22:10:40, benjhayden wrote: > This assumes that there's not more than 1 OWNERS/etc per upload. > If that assumption turns out to be false, then this will fail to store subsequent OWNERS/etc, which will cause errors when we inevitably lookup those diagnostics by the guids in the histograms. > Should this throw an exception if that assumption turns out to be false? I'm not sure what should be expected. I imagine the case above would mostly happen when 2 histogram sets get merged. I think raising an exception is a good alternative, as merging the diagnostics would be. https://codereview.chromium.org/2989143002/diff/40001/dashboard/dashboard/add... dashboard/dashboard/add_histograms.py:97: name=diagnostic_name)) On 2017/07/31 at 22:10:40, benjhayden wrote: > I think we should avoid storing SparseDiagnostics with name=None. That seems like it would cause errors down the road. Thanks, I've changed it so that the name is only added later if it should be.
lgtm once Ben is happy with it https://codereview.chromium.org/2989143002/diff/20001/dashboard/dashboard/add... File dashboard/dashboard/add_histograms.py (right): https://codereview.chromium.org/2989143002/diff/20001/dashboard/dashboard/add... dashboard/dashboard/add_histograms.py:83: for hist in histograms: On 2017/07/31 22:23:21, phsilva wrote: > On 2017/07/31 at 21:06:16, shatch wrote: > > Can you explain this loop to me, why are you looking for suite level > diagnostics in individual histograms? > > I had to look them up there because the names to diagnostic relationship is only > expressed at the histogram level. Diagnostics "don't know" their names so they > are not available at the HistogramSet.shared_diagnostics level. Ah ok, thanks! https://codereview.chromium.org/2989143002/diff/40001/dashboard/dashboard/add... File dashboard/dashboard/add_histograms.py (right): https://codereview.chromium.org/2989143002/diff/40001/dashboard/dashboard/add... dashboard/dashboard/add_histograms.py:87: name not in diagnostic_names_added): On 2017/07/31 22:23:21, phsilva wrote: > On 2017/07/31 at 22:10:40, benjhayden wrote: > > This assumes that there's not more than 1 OWNERS/etc per upload. > > If that assumption turns out to be false, then this will fail to store > subsequent OWNERS/etc, which will cause errors when we inevitably lookup those > diagnostics by the guids in the histograms. > > Should this throw an exception if that assumption turns out to be false? > > I'm not sure what should be expected. I imagine the case above would mostly > happen when 2 histogram sets get merged. I think raising an exception is a good > alternative, as merging the diagnostics would be. +1 to exception Kinda feel like if the dashboard is expecting something that's not strictly enforced by HistogramSet, just error out. https://codereview.chromium.org/2989143002/diff/60001/dashboard/dashboard/add... File dashboard/dashboard/add_histograms.py (right): https://codereview.chromium.org/2989143002/diff/60001/dashboard/dashboard/add... dashboard/dashboard/add_histograms.py:99: suite_level_sparse_diagnostic_entities[-1].name = diagnostic_name Don't you already pass the name in to the constructor?
Thanks for the review Simon. I've submitted a patch now that checks that the histograms "agree" on the diagnostic going by the same name. If they diverge, it will throw an error (added a test to reflect that too). I spoke with Ben offline and he agrees that this approach (raise exception instead of merging the two) is the better one. On 2017/08/01 at 15:28:01, simonhatch wrote: > https://codereview.chromium.org/2989143002/diff/60001/dashboard/dashboard/add... > File dashboard/dashboard/add_histograms.py (right): > > https://codereview.chromium.org/2989143002/diff/60001/dashboard/dashboard/add... > dashboard/dashboard/add_histograms.py:99: suite_level_sparse_diagnostic_entities[-1].name = diagnostic_name > Don't you already pass the name in to the constructor? A small bug still posted the name as "None" for diagnostics chosen by type, fixed now.
https://codereview.chromium.org/2989143002/diff/80001/dashboard/dashboard/add... File dashboard/dashboard/add_histograms.py (right): https://codereview.chromium.org/2989143002/diff/80001/dashboard/dashboard/add... dashboard/dashboard/add_histograms.py:99: suite_level_sparse_diagnostic_entities[-1].name = name Why not set name for all SparseDiagnostics?
On 2017/08/01 at 21:46:18, benjhayden wrote: > https://codereview.chromium.org/2989143002/diff/80001/dashboard/dashboard/add... > File dashboard/dashboard/add_histograms.py (right): > > https://codereview.chromium.org/2989143002/diff/80001/dashboard/dashboard/add... > dashboard/dashboard/add_histograms.py:99: suite_level_sparse_diagnostic_entities[-1].name = name > Why not set name for all SparseDiagnostics? I'm trying here to mimic the previous implementation specifically for the diagnostics that are being selected by type. They don't have an assigned name the way the ones selected by reserved infos do. Here their name will be their type so I'm not entirely sure they would create a conflict (say if you had 2 GenericSet diagnostics that are *not* any of the ones in the reserved info). Overall it was just an attempt to keep the functionality exactly the same for the diagnostics that haven't been changed. Do you think it's better if we just use the type as name for the "legacy" diagnostics, and raise exceptions if two of the same type are used for the same histogram set?
On 2017/08/02 00:09:03, phsilva wrote: > On 2017/08/01 at 21:46:18, benjhayden wrote: > > > https://codereview.chromium.org/2989143002/diff/80001/dashboard/dashboard/add... > > File dashboard/dashboard/add_histograms.py (right): > > > > > https://codereview.chromium.org/2989143002/diff/80001/dashboard/dashboard/add... > > dashboard/dashboard/add_histograms.py:99: > suite_level_sparse_diagnostic_entities[-1].name = name > > Why not set name for all SparseDiagnostics? > > I'm trying here to mimic the previous implementation specifically for the > diagnostics that are being selected by type. They don't have an assigned name > the way the ones selected by reserved infos do. Here their name will be their > type so I'm not entirely sure they would create a conflict (say if you had 2 > GenericSet diagnostics that are *not* any of the ones in the reserved info). > Overall it was just an attempt to keep the functionality exactly the same for > the diagnostics that haven't been changed. > > Do you think it's better if we just use the type as name for the "legacy" > diagnostics, and raise exceptions if two of the same type are used for the same > histogram set? My understanding is that once all this work is done, there won't be any "legacy" diagnostics. You shouldn't worry too much about preserving existing functionality, since if there are even diagnostics in the datastore right now, they're from test uploads and not real data.
lgtm
On 2017/08/02 at 12:56:16, simonhatch wrote: > My understanding is that once all this work is done, there won't be any "legacy" diagnostics. You shouldn't worry too much about preserving existing functionality, since if there are even diagnostics in the datastore right now, they're from test uploads and not real data. Ah I see. I'll just use their names as well here then. We should then expect exceptions if two diagnostics selected by their types conflict between two histograms.
On 2017/08/02 at 17:56:22, phsilva wrote: > On 2017/08/02 at 12:56:16, simonhatch wrote: > > My understanding is that once all this work is done, there won't be any "legacy" diagnostics. You shouldn't worry too much about preserving existing functionality, since if there are even diagnostics in the datastore right now, they're from test uploads and not real data. > > Ah I see. I'll just use their names as well here then. We should then expect exceptions if two diagnostics selected by their types conflict between two histograms. I've uploaded a patch that reflect these changes.
lgtm https://codereview.chromium.org/2989143002/diff/100001/dashboard/dashboard/ad... File dashboard/dashboard/add_histograms.py (right): https://codereview.chromium.org/2989143002/diff/100001/dashboard/dashboard/ad... dashboard/dashboard/add_histograms.py:32: SUITE_LEVEL_SPARSE_DIAGNOSTIC_TYPES = set( Please add a TODO to get rid of this (#3507).
The CQ bit was checked by phsilva@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from simonhatch@chromium.org, benjhayden@chromium.org, eakuefner@chromium.org Link to the patchset: https://codereview.chromium.org/2989143002/#ps120001 (title: "Add TODO")
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": 120001, "attempt_start_ts": 1502232259409090,
"parent_rev": "b0531f7a731c861783a4c71cb47302bf7164a3fe", "commit_rev":
"68c8e443039c742d3685516647cfef77def67d27"}
Message was sent while issue was closed.
Description was changed from ========== Plumb SparseDiagnostics by Name This CL adds the creation of Sparse Diagnostics in add_histograms by using a set of reserved names. It also adds some of the logic so that when shared diagnostics of histograms are imported, their name is checked against the reserved set, instead of only using their type. This addresses step 5 of the list of tasks to be done in bug 3507. BUG=catapult:#3718, catapult:#3507 ========== to ========== Plumb SparseDiagnostics by Name This CL adds the creation of Sparse Diagnostics in add_histograms by using a set of reserved names. It also adds some of the logic so that when shared diagnostics of histograms are imported, their name is checked against the reserved set, instead of only using their type. This addresses step 5 of the list of tasks to be done in bug 3507. BUG=catapult:#3718, catapult:#3507 Review-Url: https://codereview.chromium.org/2989143002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2997643002/ by phsilva@google.com. The reason for reverting is: This CL landed while adding tests that used functionality that was deprecated by a separate CL.
The CQ bit was checked by phsilva@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org, benjhayden@chromium.org, simonhatch@chromium.org Link to the patchset: https://codereview.chromium.org/2989143002/#ps140001 (title: "Rebase onto master")
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": 1502298309492360,
"parent_rev": "ffd86f1e6c9de3b455b68af1d6dc1e15fefdfe5a", "commit_rev":
"0efcc192805c88a3ab6347f26245e6d293d09b5f"}
Message was sent while issue was closed.
Description was changed from ========== Plumb SparseDiagnostics by Name This CL adds the creation of Sparse Diagnostics in add_histograms by using a set of reserved names. It also adds some of the logic so that when shared diagnostics of histograms are imported, their name is checked against the reserved set, instead of only using their type. This addresses step 5 of the list of tasks to be done in bug 3507. BUG=catapult:#3718, catapult:#3507 Review-Url: https://codereview.chromium.org/2989143002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ========== to ========== Plumb SparseDiagnostics by Name This CL adds the creation of Sparse Diagnostics in add_histograms by using a set of reserved names. It also adds some of the logic so that when shared diagnostics of histograms are imported, their name is checked against the reserved set, instead of only using their type. This addresses step 5 of the list of tasks to be done in bug 3507. BUG=catapult:#3718, catapult:#3507 Review-Url: https://codereview.chromium.org/2989143002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... Review-Url: https://codereview.chromium.org/2989143002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
