Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2707783004:
[remote suggestions] Add a new histogram for recording the time since a snippet was fetched. (Closed)
|
Created:
3 years, 10 months ago by markusheintz_ Modified:
3 years, 10 months ago CC:
chromium-reviews, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[remote suggestions] Add a new histogram for recording the time since a snippet was fetched.
This is the missing piece to https://codereview.chromium.org/2686063003/
BUG=690514
Review-Url: https://codereview.chromium.org/2707783004
Cr-Commit-Position: refs/heads/master@{#452465}
Committed: https://chromium.googlesource.com/chromium/src/+/2075239e1a890ce7bd32ae54ca5575c145528649
Patch Set 1 #Patch Set 2 : '"' #
Total comments: 11
Patch Set 3 : Address comments #
Total comments: 4
Patch Set 4 : Address comments round two #Patch Set 5 : Change histogram units to minutes #Patch Set 6 : Address comments of comments #
Total comments: 5
Patch Set 7 : final change #Patch Set 8 : Address final comments #Patch Set 9 : rebase on tot #
Messages
Total messages: 35 (9 generated)
Description was changed from ========== [remote suggestions] Add a new historgram for recoding the time since a snippet was fetched. BUG=690514 ========== to ========== [remote suggestions] Add a new historgram for recoding the time since a snippet was fetched. This is the missing piece to https://codereview.chromium.org/2686063003/ BUG=690514 ==========
markusheintz@chromium.org changed reviewers: + jkrcal@chromium.org, treib@chromium.org
Jan and Marc could you please take a look. Sorry for the delay
Description was changed from ========== [remote suggestions] Add a new historgram for recoding the time since a snippet was fetched. This is the missing piece to https://codereview.chromium.org/2686063003/ BUG=690514 ========== to ========== [remote suggestions] Add a new histogram for recording the time since a snippet was fetched. This is the missing piece to https://codereview.chromium.org/2686063003/ BUG=690514 ==========
https://codereview.chromium.org/2707783004/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2707783004/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:257: UMA_HISTOGRAM_CUSTOM_TIMES( I am wondering, do we still need the old histogram? I do not see the situation when we would need it.
https://codereview.chromium.org/2707783004/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2707783004/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:64: const char kHistogramContentSuggestionsTimeSinceSuggestionFetched[] = optional nit: You could remove "ContentSuggestions" from the variable name, also from the one above. All the other histograms don't have it, and it's obvious from context anyway. https://codereview.chromium.org/2707783004/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:245: /*bucket_count=*/100); Should we record this only for ARTICLES? If not, should it be a per-category histogram, and/or should we only record it if we actually have a fetch date? https://codereview.chromium.org/2707783004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2707783004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39848: + units="ms"> Is this the correct unit? Will the dashboard always show milliseconds?
Address comments
https://codereview.chromium.org/2707783004/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2707783004/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:64: const char kHistogramContentSuggestionsTimeSinceSuggestionFetched[] = On 2017/02/21 09:14:09, Marc Treib wrote: > optional nit: You could remove "ContentSuggestions" from the variable name, also > from the one above. All the other histograms don't have it, and it's obvious > from context anyway. Done. https://codereview.chromium.org/2707783004/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:245: /*bucket_count=*/100); On 2017/02/21 09:14:09, Marc Treib wrote: > Should we record this only for ARTICLES? > If not, should it be a per-category histogram, and/or should we only record it > if we actually have a fetch date? Done. No the fetch date for non Article suggestions should always be 0. https://codereview.chromium.org/2707783004/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:257: UMA_HISTOGRAM_CUSTOM_TIMES( On 2017/02/20 18:38:37, jkrcal wrote: > I am wondering, do we still need the old histogram? > I do not see the situation when we would need it. I removed the code. https://codereview.chromium.org/2707783004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2707783004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39848: + units="ms"> On 2017/02/21 09:14:09, Marc Treib wrote: > Is this the correct unit? Will the dashboard always show milliseconds? yes. It seems to be the common unit. But I can see that we could use minutes for example. This would make reading the histogram more pleasant. I guess hours is too coarse grained. Even though it may still work. Shall we change it to min?
https://codereview.chromium.org/2707783004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2707783004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39848: + units="ms"> On 2017/02/21 11:31:50, markusheintz_ wrote: > On 2017/02/21 09:14:09, Marc Treib wrote: > > Is this the correct unit? Will the dashboard always show milliseconds? > > yes. It seems to be the common unit. > > But I can see that we could use minutes for example. This would make reading the > histogram more pleasant. I guess hours is too coarse grained. Even though it may > still work. > > Shall we change it to min? I *think* this is only used for the label in the dashboards - AFAIK the dashboard won't actually convert to the correct unit. We should check with the UMA folks though; IMO seconds or minutes would be much easier to read, if it's possible to get them. Anyway, not a big deal. https://codereview.chromium.org/2707783004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2707783004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39837: -<histogram name="NewTabPage.ContentSuggestions.TimeSinceLastBackgroundFetch" We never remove entries from here - we keep them, but add an <obsolete> tag. https://codereview.chromium.org/2707783004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2707783004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39842: + Recorded when the user looks at content suggestions on the NTP. Please mention that it's only recorded for ARTICLE suggestions.
Address comments round two
https://codereview.chromium.org/2707783004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2707783004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39848: + units="ms"> On 2017/02/21 11:53:48, Marc Treib wrote: > On 2017/02/21 11:31:50, markusheintz_ wrote: > > On 2017/02/21 09:14:09, Marc Treib wrote: > > > Is this the correct unit? Will the dashboard always show milliseconds? > > > > yes. It seems to be the common unit. > > > > But I can see that we could use minutes for example. This would make reading > the > > histogram more pleasant. I guess hours is too coarse grained. Even though it > may > > still work. > > > > Shall we change it to min? > > I *think* this is only used for the label in the dashboards - AFAIK the > dashboard won't actually convert to the correct unit. We should check with the > UMA folks though; IMO seconds or minutes would be much easier to read, if it's > possible to get them. > Anyway, not a big deal. The example for the MACRO usage refers to ms as the unit to use (https://cs.corp.google.com/piper///depot/google3/third_party/chromium_headles...) But I guess we can put in whatever we want. I'll try to clarify https://codereview.chromium.org/2707783004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2707783004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39837: -<histogram name="NewTabPage.ContentSuggestions.TimeSinceLastBackgroundFetch" On 2017/02/21 11:53:48, Marc Treib wrote: > We never remove entries from here - we keep them, but add an <obsolete> tag. Done. https://codereview.chromium.org/2707783004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2707783004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39842: + Recorded when the user looks at content suggestions on the NTP. On 2017/02/21 11:53:48, Marc Treib wrote: > Please mention that it's only recorded for ARTICLE suggestions. Done.
Change histogram units to minutes
https://codereview.chromium.org/2707783004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2707783004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39848: + units="ms"> On 2017/02/21 15:28:57, markusheintz_ wrote: > On 2017/02/21 11:53:48, Marc Treib wrote: > > On 2017/02/21 11:31:50, markusheintz_ wrote: > > > On 2017/02/21 09:14:09, Marc Treib wrote: > > > > Is this the correct unit? Will the dashboard always show milliseconds? > > > > > > yes. It seems to be the common unit. > > > > > > But I can see that we could use minutes for example. This would make reading > > the > > > histogram more pleasant. I guess hours is too coarse grained. Even though it > > may > > > still work. > > > > > > Shall we change it to min? > > > > I *think* this is only used for the label in the dashboards - AFAIK the > > dashboard won't actually convert to the correct unit. We should check with the > > UMA folks though; IMO seconds or minutes would be much easier to read, if it's > > possible to get them. > > Anyway, not a big deal. > > > The example for the MACRO usage refers to ms as the unit to use > (https://cs.corp.google.com/piper///depot/google3/third_party/chromium_headles...) > > But I guess we can put in whatever we want. I'll try to clarify PTAL
lgtm I am not sure about using the time macro with minutes (the macro docs should get updated if this is okay) but let's see what metrics folks say to it. I am also puzzled how this even compiles. The macro UMA_HISTOGRAM_CUSTOM_TIMES expects the params sample, min, max to be TimeDeltas, we provide ints (and always provided in other places). I see no explicit public constructor of TimeDelta from int. What am I missing?
On 2017/02/22 07:48:34, jkrcal wrote: > lgtm > > I am not sure about using the time macro with minutes (the macro docs should get > updated if this is okay) but let's see what metrics folks say to it. > > I am also puzzled how this even compiles. The macro UMA_HISTOGRAM_CUSTOM_TIMES > expects the params sample, min, max to be TimeDeltas, we provide ints (and > always provided in other places). I see no explicit public constructor of > TimeDelta from int. What am I missing? I'm not using the time macro any more. I'm using a count macro now. That's why it compiles ;-)
On 2017/02/22 08:47:40, markusheintz_ wrote: > On 2017/02/22 07:48:34, jkrcal wrote: > > lgtm > > > > I am not sure about using the time macro with minutes (the macro docs should > get > > updated if this is okay) but let's see what metrics folks say to it. > > > > I am also puzzled how this even compiles. The macro UMA_HISTOGRAM_CUSTOM_TIMES > > expects the params sample, min, max to be TimeDeltas, we provide ints (and > > always provided in other places). I see no explicit public constructor of > > TimeDelta from int. What am I missing? > > I'm not using the time macro any more. I'm using a count macro now. That's why > it compiles ;-) Er.. why? If we're recording times, IMO we should use the standard macros for times. And if the dashboard doesn't display the data nicely (which IIUC we're not even sure about), we should nag the dashboard owners to improve that :)
On 2017/02/22 09:23:00, Marc Treib wrote: > On 2017/02/22 08:47:40, markusheintz_ wrote: > > On 2017/02/22 07:48:34, jkrcal wrote: > > > lgtm > > > > > > I am not sure about using the time macro with minutes (the macro docs should > > get > > > updated if this is okay) but let's see what metrics folks say to it. > > > > > > I am also puzzled how this even compiles. The macro > UMA_HISTOGRAM_CUSTOM_TIMES > > > expects the params sample, min, max to be TimeDeltas, we provide ints (and > > > always provided in other places). I see no explicit public constructor of > > > TimeDelta from int. What am I missing? > > > > I'm not using the time macro any more. I'm using a count macro now. That's why > > it compiles ;-) > > Er.. why? If we're recording times, IMO we should use the standard macros for > times. > And if the dashboard doesn't display the data nicely (which IIUC we're not even > sure about), we should nag the dashboard owners to improve that :) It seems kike there has been some misunderstanding. Resolved offline. Changing back to using the CUSTOM_TIMES macro
Address comments of comments
Thanks! LGTM with some last nits. Sorry about the misunderstanding! I'll try to make my comments clearer next time :( https://codereview.chromium.org/2707783004/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2707783004/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_metrics.cc:242: base::TimeDelta::FromDays(0), base::TimeDelta::FromDays(7), I think the min value can't be 0. FromSeconds(1)? https://codereview.chromium.org/2707783004/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2707783004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:39851: + units="seconds"> Please check with histogram owners what units are supported here - we might have to use "ms" for now. https://codereview.chromium.org/2707783004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:39856: + is only recorded for snippets of the Articles for you section. nit: s/snippets/suggestions/
final change
https://codereview.chromium.org/2707783004/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2707783004/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_metrics.cc:242: base::TimeDelta::FromDays(0), base::TimeDelta::FromDays(7), On 2017/02/22 13:38:11, Marc Treib wrote: > I think the min value can't be 0. FromSeconds(1)? done. already uploaded. sry I forgot https://codereview.chromium.org/2707783004/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2707783004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:39851: + units="seconds"> On 2017/02/22 13:38:11, Marc Treib wrote: > Please check with histogram owners what units are supported here - we might have > to use "ms" for now. Done. https://codereview.chromium.org/2707783004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:39856: + is only recorded for snippets of the Articles for you section. On 2017/02/22 13:38:11, Marc Treib wrote: > nit: s/snippets/suggestions/ Done.
https://codereview.chromium.org/2707783004/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2707783004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:39851: + units="seconds"> On 2017/02/22 13:38:11, Marc Treib wrote: > Please check with histogram owners what units are supported here - we might have > to use "ms" for now. Done. https://codereview.chromium.org/2707783004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:39856: + is only recorded for snippets of the Articles for you section. On 2017/02/22 13:38:11, Marc Treib wrote: > nit: s/snippets/suggestions/ Done.
Address final comments
markusheintz@chromium.org changed reviewers: + mkwst@chromium.org
On 2017/02/22 13:42:33, markusheintz_ wrote: > Address final comments Mike could you review the histogram changes in this CL? Thanks a lot.
markusheintz@chromium.org changed reviewers: + holte@chromium.org - mkwst@chromium.org
On 2017/02/22 13:43:47, markusheintz_ wrote: > On 2017/02/22 13:42:33, markusheintz_ wrote: > > Address final comments > > Mike could you review the histogram changes in this CL? Thanks a lot. Replacing Mike with Steven. Steven could you review the histograms.xml changes? Thanks a lot
lgtm
rebase on tot
The CQ bit was checked by markusheintz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org, treib@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2707783004/#ps160001 (title: "rebase on tot")
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": 160001, "attempt_start_ts": 1487846961607310,
"parent_rev": "8d90ac250bb2ac5aaa876b04300c8edce0a7b824", "commit_rev":
"2075239e1a890ce7bd32ae54ca5575c145528649"}
Message was sent while issue was closed.
Description was changed from ========== [remote suggestions] Add a new histogram for recording the time since a snippet was fetched. This is the missing piece to https://codereview.chromium.org/2686063003/ BUG=690514 ========== to ========== [remote suggestions] Add a new histogram for recording the time since a snippet was fetched. This is the missing piece to https://codereview.chromium.org/2686063003/ BUG=690514 Review-Url: https://codereview.chromium.org/2707783004 Cr-Commit-Position: refs/heads/master@{#452465} Committed: https://chromium.googlesource.com/chromium/src/+/2075239e1a890ce7bd32ae54ca55... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/2075239e1a890ce7bd32ae54ca55... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
