Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2832173002:
NTP: Fix TileType metrics on desktop (Closed)
|
Created:
3 years, 8 months ago by mastiz Modified:
3 years, 8 months ago CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, asvitkine+watch_chromium.org, donnd+watch_chromium.org, jfweitz+watch_chromium.org, David Black, arv+watch_chromium.org, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNTP: Fix TileType metrics on desktop
Follow-up to https://codereview.chromium.org/2796643002 where the
metrics were added but an inconsistency was introduced between the C++
enum and the Javascript counterpart. This caused the wrong buckets being
counted for NewTabPage.TileType (ThumbnailLocal instead Thumbnail;
ThumbnailServer instead of ThumbnailFailed). The affected buckets
were already deprecated (DeprecatedThumbnailLocal and
DeprecatedThumbnailServer) and hence, luckily, there are no conflated
buckets.
Besides, histograms.xml was missing the new suffixes for
NewTabPageIconTypes, used to provide desktop-specific breakdowns for
NewTabPage.MostVisited and NewTabPage.SuggestionsImpression.
BUG=714095, 703165
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2832173002
Cr-Commit-Position: refs/heads/master@{#467287}
Committed: https://chromium.googlesource.com/chromium/src/+/79eaaf7eefb10a3006da5d1a56007e8c6f72ffe1
Patch Set 1 #
Total comments: 1
Messages
Total messages: 26 (11 generated)
Description was changed from ========== NTP: Fix TileType metrics on desktop Follow-up to https://codereview.chromium.org/2796643002 where the metrics were added but an inconsistency was introduced between the C++ enum and the Javascript counterpart. This caused the wrong buckets being counted for NewTabPage.TileType (ThumbnailLocal instead Thumbnail; ThumbnailServer instead of ThumbnailFailed). Besides, histograms.xml was missing the new suffixes for NewTabPageIconTypes, used to provide desktop-specific breakdowns for NewTabPage.MostVisited and NewTabPage.SuggestionsImpression. BUG=703165 ========== to ========== NTP: Fix TileType metrics on desktop Follow-up to https://codereview.chromium.org/2796643002 where the metrics were added but an inconsistency was introduced between the C++ enum and the Javascript counterpart. This caused the wrong buckets being counted for NewTabPage.TileType (ThumbnailLocal instead Thumbnail; ThumbnailServer instead of ThumbnailFailed). Besides, histograms.xml was missing the new suffixes for NewTabPageIconTypes, used to provide desktop-specific breakdowns for NewTabPage.MostVisited and NewTabPage.SuggestionsImpression. BUG=703165 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
mastiz@chromium.org changed reviewers: + sfiera@chromium.org
LGTM Seems our tests weren't good enough to catch this. Have you… * …verified the fix by hand? * …filed a bug against treib for better tests? :) When we talked before, you believed it wasn't worth merging this fix to M59. That may be, but can you create a separate bug for it in case we decide otherwise? It would be OK by me to reuse the CL title/description.
Description was changed from ========== NTP: Fix TileType metrics on desktop Follow-up to https://codereview.chromium.org/2796643002 where the metrics were added but an inconsistency was introduced between the C++ enum and the Javascript counterpart. This caused the wrong buckets being counted for NewTabPage.TileType (ThumbnailLocal instead Thumbnail; ThumbnailServer instead of ThumbnailFailed). Besides, histograms.xml was missing the new suffixes for NewTabPageIconTypes, used to provide desktop-specific breakdowns for NewTabPage.MostVisited and NewTabPage.SuggestionsImpression. BUG=703165 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== NTP: Fix TileType metrics on desktop Follow-up to https://codereview.chromium.org/2796643002 where the metrics were added but an inconsistency was introduced between the C++ enum and the Javascript counterpart. This caused the wrong buckets being counted for NewTabPage.TileType (ThumbnailLocal instead Thumbnail; ThumbnailServer instead of ThumbnailFailed). Besides, histograms.xml was missing the new suffixes for NewTabPageIconTypes, used to provide desktop-specific breakdowns for NewTabPage.MostVisited and NewTabPage.SuggestionsImpression. BUG=714095,703165 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2017/04/21 11:06:30, sfiera wrote: > LGTM > > Seems our tests weren't good enough to catch this. Have you… > * …verified the fix by hand? Yes. > * …filed a bug against treib for better tests? :) Done, https://bugs.chromium.org/p/chromium/issues/detail?id=714096 (although not referenced in code in the form of TODO). > > When we talked before, you believed it wasn't worth merging this fix to M59. > That may be, but can you create a separate bug for it in case we decide > otherwise? It would be OK by me to reuse the CL title/description. Done, https://bugs.chromium.org/p/chromium/issues/detail?id=714095.
mastiz@chromium.org changed reviewers: + isherman@chromium.org, jeremycho@chromium.org
jeremycho@chromium.org: Please review changes in most_visited_single.js isherman@chromium.org: Please review changes in histograms.xml
https://codereview.chromium.org/2832173002/diff/1/chrome/browser/resources/lo... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/2832173002/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/most_visited_single.js:55: THUMBNAIL_FAILED: 8, I'm confused about why there is a gap in this enum. It sounds like there's a Java-side enum as well, and the two need to be kept in sync? That sounds really fragile. If my understanding is correct, please move all of the enumerated constants into C++, and expose them to Java. (I know that I've seen this done elsewhere in the codebase, though I don't have a reference handy, unfortunately.) Also, if my understanding is correct, then the histograms recording these metrics are going to have conflated data for some buckets. The best practice in this case is to rename the histograms, so that the active histograms have internally consistent semantics.
On 2017/04/21 19:58:38, Ilya Sherman wrote: > https://codereview.chromium.org/2832173002/diff/1/chrome/browser/resources/lo... > File chrome/browser/resources/local_ntp/most_visited_single.js (right): > > https://codereview.chromium.org/2832173002/diff/1/chrome/browser/resources/lo... > chrome/browser/resources/local_ntp/most_visited_single.js:55: THUMBNAIL_FAILED: > 8, > I'm confused about why there is a gap in this enum. It sounds like there's a > Java-side enum as well, and the two need to be kept in sync? That sounds really > fragile. If my understanding is correct, please move all of the enumerated > constants into C++, and expose them to Java. (I know that I've seen this done > elsewhere in the codebase, though I don't have a reference handy, > unfortunately.) There seems to be some confusion Java<->Javascript. The enum is indeed shared between C++ and Java, but AFAIK there's no analogous approach to Javascript, hence the fragile duplication. If you know otherwise, please let me know. > > Also, if my understanding is correct, then the histograms recording these > metrics are going to have conflated data for some buckets. The best practice in > this case is to rename the histograms, so that the active histograms have > internally consistent semantics. Another alternative is to deprecate histogram buckets, which is what we've done with this histogram in the past. Note that, luckily, there are no conflated buckets, since the bug (being fixed here) causes Chrome to contribute to buckets that happen to be deprecated (and hence not populated, modulo this bug).
On 2017/04/24 07:50:53, mastiz wrote: > On 2017/04/21 19:58:38, Ilya Sherman wrote: > > > https://codereview.chromium.org/2832173002/diff/1/chrome/browser/resources/lo... > > File chrome/browser/resources/local_ntp/most_visited_single.js (right): > > > > > https://codereview.chromium.org/2832173002/diff/1/chrome/browser/resources/lo... > > chrome/browser/resources/local_ntp/most_visited_single.js:55: > THUMBNAIL_FAILED: > > 8, > > I'm confused about why there is a gap in this enum. It sounds like there's a > > Java-side enum as well, and the two need to be kept in sync? That sounds > really > > fragile. If my understanding is correct, please move all of the enumerated > > constants into C++, and expose them to Java. (I know that I've seen this done > > elsewhere in the codebase, though I don't have a reference handy, > > unfortunately.) > > There seems to be some confusion Java<->Javascript. The enum is indeed shared > between C++ and Java, but AFAIK there's no analogous approach to Javascript, > hence the fragile duplication. If you know otherwise, please let me know. Yes, sorry, I must have misread the context. There is probably an API for exposing constants to JavaScript, right? Would it be reasonable to provide accessors for the enum constants that the JS side needs? Perhaps you could provide them all packaged together in a single object, so that you don't need to define a wide API for this. > > Also, if my understanding is correct, then the histograms recording these > > metrics are going to have conflated data for some buckets. The best practice > in > > this case is to rename the histograms, so that the active histograms have > > internally consistent semantics. > > Another alternative is to deprecate histogram buckets, which is what we've done > with this histogram in the past. Note that, luckily, there are no conflated > buckets, since the bug (being fixed here) causes Chrome to contribute to buckets > that happen to be deprecated (and hence not populated, modulo this bug). Yes, deprecating histogram buckets is okay too, as long as you don't mind the noise in the histogram.
On 2017/04/24 19:31:50, Ilya Sherman wrote: > On 2017/04/24 07:50:53, mastiz wrote: > > On 2017/04/21 19:58:38, Ilya Sherman wrote: > > > > > > https://codereview.chromium.org/2832173002/diff/1/chrome/browser/resources/lo... > > > File chrome/browser/resources/local_ntp/most_visited_single.js (right): > > > > > > > > > https://codereview.chromium.org/2832173002/diff/1/chrome/browser/resources/lo... > > > chrome/browser/resources/local_ntp/most_visited_single.js:55: > > THUMBNAIL_FAILED: > > > 8, > > > I'm confused about why there is a gap in this enum. It sounds like there's > a > > > Java-side enum as well, and the two need to be kept in sync? That sounds > > really > > > fragile. If my understanding is correct, please move all of the enumerated > > > constants into C++, and expose them to Java. (I know that I've seen this > done > > > elsewhere in the codebase, though I don't have a reference handy, > > > unfortunately.) > > > > There seems to be some confusion Java<->Javascript. The enum is indeed shared > > between C++ and Java, but AFAIK there's no analogous approach to Javascript, > > hence the fragile duplication. If you know otherwise, please let me know. > > Yes, sorry, I must have misread the context. There is probably an API for > exposing constants to JavaScript, right? Would it be reasonable to provide > accessors for the enum constants that the JS side needs? Perhaps you could > provide them all packaged together in a single object, so that you don't need to > define a wide API for this. Sorry but I asked around and nobody knows of a good solution to achieve your suggestion. If you have some pointers, I can try to dig deeper, but it might even make sense to do that in a follow-up CL, this being a fix. I did file a bug to improve test coverage such that similar issues in the future are caught. > > > > Also, if my understanding is correct, then the histograms recording these > > > metrics are going to have conflated data for some buckets. The best > practice > > in > > > this case is to rename the histograms, so that the active histograms have > > > internally consistent semantics. > > > > Another alternative is to deprecate histogram buckets, which is what we've > done > > with this histogram in the past. Note that, luckily, there are no conflated > > buckets, since the bug (being fixed here) causes Chrome to contribute to > buckets > > that happen to be deprecated (and hence not populated, modulo this bug). > > Yes, deprecating histogram buckets is okay too, as long as you don't mind the > noise in the histogram.
I'm also not coming up with a great way to share an enum between JS and C++. Alas. The metrics change LGTM as long as the only affected buckets were already deprecated.
Description was changed from ========== NTP: Fix TileType metrics on desktop Follow-up to https://codereview.chromium.org/2796643002 where the metrics were added but an inconsistency was introduced between the C++ enum and the Javascript counterpart. This caused the wrong buckets being counted for NewTabPage.TileType (ThumbnailLocal instead Thumbnail; ThumbnailServer instead of ThumbnailFailed). Besides, histograms.xml was missing the new suffixes for NewTabPageIconTypes, used to provide desktop-specific breakdowns for NewTabPage.MostVisited and NewTabPage.SuggestionsImpression. BUG=714095,703165 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== NTP: Fix TileType metrics on desktop Follow-up to https://codereview.chromium.org/2796643002 where the metrics were added but an inconsistency was introduced between the C++ enum and the Javascript counterpart. This caused the wrong buckets being counted for NewTabPage.TileType (ThumbnailLocal instead Thumbnail; ThumbnailServer instead of ThumbnailFailed). The affected buckets were already deprecated (DeprecatedThumbnailLocal and DeprecatedThumbnailServer) and hence, luckily, there are no conflated buckets. Besides, histograms.xml was missing the new suffixes for NewTabPageIconTypes, used to provide desktop-specific breakdowns for NewTabPage.MostVisited and NewTabPage.SuggestionsImpression. BUG=714095,703165 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2017/04/25 23:49:13, Ilya Sherman wrote: > I'm also not coming up with a great way to share an enum between JS and C++. > Alas. > > The metrics change LGTM as long as the only affected buckets were already > deprecated. Confirmed, that's the case. I updated the CL description to emphasize this, thanks!
The CQ bit was checked by mastiz@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
treib@chromium.org changed reviewers: + treib@chromium.org
Drive-by owners LGTM. Thanks for fixing this!
The CQ bit was checked by mastiz@chromium.org
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": 1, "attempt_start_ts": 1493202373200500, "parent_rev":
"2edac8fe131cb58ae2bc37c007cc0b3c56f6fc0e", "commit_rev":
"79eaaf7eefb10a3006da5d1a56007e8c6f72ffe1"}
Message was sent while issue was closed.
Description was changed from ========== NTP: Fix TileType metrics on desktop Follow-up to https://codereview.chromium.org/2796643002 where the metrics were added but an inconsistency was introduced between the C++ enum and the Javascript counterpart. This caused the wrong buckets being counted for NewTabPage.TileType (ThumbnailLocal instead Thumbnail; ThumbnailServer instead of ThumbnailFailed). The affected buckets were already deprecated (DeprecatedThumbnailLocal and DeprecatedThumbnailServer) and hence, luckily, there are no conflated buckets. Besides, histograms.xml was missing the new suffixes for NewTabPageIconTypes, used to provide desktop-specific breakdowns for NewTabPage.MostVisited and NewTabPage.SuggestionsImpression. BUG=714095,703165 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== NTP: Fix TileType metrics on desktop Follow-up to https://codereview.chromium.org/2796643002 where the metrics were added but an inconsistency was introduced between the C++ enum and the Javascript counterpart. This caused the wrong buckets being counted for NewTabPage.TileType (ThumbnailLocal instead Thumbnail; ThumbnailServer instead of ThumbnailFailed). The affected buckets were already deprecated (DeprecatedThumbnailLocal and DeprecatedThumbnailServer) and hence, luckily, there are no conflated buckets. Besides, histograms.xml was missing the new suffixes for NewTabPageIconTypes, used to provide desktop-specific breakdowns for NewTabPage.MostVisited and NewTabPage.SuggestionsImpression. BUG=714095,703165 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2832173002 Cr-Commit-Position: refs/heads/master@{#467287} Committed: https://chromium.googlesource.com/chromium/src/+/79eaaf7eefb10a3006da5d1a5600... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/79eaaf7eefb10a3006da5d1a5600... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
