Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(139)

Issue 12314053: Log the freshness of the Variations seed in a histogram. (Closed)

Created:
7 years, 10 months ago by SteveT
Modified:
7 years, 10 months ago
CC:
chromium-reviews, MAD, Ilya Sherman, jar (doing other things)
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

Log the freshness of the Variations seed in a histogram. We do this by storing a pref of the last successful (200 or 304) fetch and comparing it to the next one. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184452

Patch Set 1 #

Total comments: 6

Patch Set 2 : use UMA_HISTOGRAMS_CUSTOM_TIMES #

Patch Set 3 : use custom_counts #

Total comments: 6

Patch Set 4 : compare to startup #

Total comments: 11

Patch Set 5 : asvit nit #

Total comments: 2

Patch Set 6 : handle 200s if valid #

Total comments: 2

Patch Set 7 : Move log call #

Patch Set 8 : local_state_ crash fix #

Patch Set 9 : check comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+37 lines, -2 lines) Patch
M chrome/browser/metrics/variations/variations_service.h View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/metrics/variations/variations_service.cc View 1 2 3 4 5 6 7 8 5 chunks +29 lines, -2 lines 0 comments Download
M chrome/common/pref_names.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/pref_names.cc View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 28 (0 generated)
SteveT
PTAL. I am mostly concerned about how sane UMA_HISTOGRAMS_COUNTS_10000 is.
7 years, 10 months ago (2013-02-21 22:51:02 UTC) #1
Ilya Sherman
https://codereview.chromium.org/12314053/diff/1/chrome/browser/metrics/variations/variations_service.cc File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/1/chrome/browser/metrics/variations/variations_service.cc#newcode304 chrome/browser/metrics/variations/variations_service.cc:304: UMA_HISTOGRAM_COUNTS_10000("Variations.SeedFreshness", You sure you don't want UMA_HISTOGRAM_CUSTOM_TIMES?
7 years, 10 months ago (2013-02-22 00:31:28 UTC) #2
SteveT
Question for you! https://codereview.chromium.org/12314053/diff/1/chrome/browser/metrics/variations/variations_service.cc File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/1/chrome/browser/metrics/variations/variations_service.cc#newcode304 chrome/browser/metrics/variations/variations_service.cc:304: UMA_HISTOGRAM_COUNTS_10000("Variations.SeedFreshness", Hmm, it's a little hard ...
7 years, 10 months ago (2013-02-22 00:38:37 UTC) #3
Ilya Sherman
https://codereview.chromium.org/12314053/diff/1/chrome/browser/metrics/variations/variations_service.cc File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/1/chrome/browser/metrics/variations/variations_service.cc#newcode304 chrome/browser/metrics/variations/variations_service.cc:304: UMA_HISTOGRAM_COUNTS_10000("Variations.SeedFreshness", On 2013/02/22 00:38:37, SteveT wrote: > Hmm, it's ...
7 years, 10 months ago (2013-02-22 00:45:50 UTC) #4
SteveT
Question for you in the first patch regarding the second patch. :) https://codereview.chromium.org/12314053/diff/1/chrome/browser/metrics/variations/variations_service.cc File chrome/browser/metrics/variations/variations_service.cc ...
7 years, 10 months ago (2013-02-22 01:01:02 UTC) #5
Ilya Sherman
https://codereview.chromium.org/12314053/diff/1/chrome/browser/metrics/variations/variations_service.cc File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/1/chrome/browser/metrics/variations/variations_service.cc#newcode304 chrome/browser/metrics/variations/variations_service.cc:304: UMA_HISTOGRAM_COUNTS_10000("Variations.SeedFreshness", On 2013/02/22 01:01:02, SteveT wrote: > So in ...
7 years, 10 months ago (2013-02-22 01:12:07 UTC) #6
SteveT
Cool, thanks. PTAL. https://codereview.chromium.org/12314053/diff/1/chrome/browser/metrics/variations/variations_service.cc File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/1/chrome/browser/metrics/variations/variations_service.cc#newcode304 chrome/browser/metrics/variations/variations_service.cc:304: UMA_HISTOGRAM_COUNTS_10000("Variations.SeedFreshness", Yeah, if the dashboard had ...
7 years, 10 months ago (2013-02-22 01:26:40 UTC) #7
Ilya Sherman
LGTM, thanks. https://codereview.chromium.org/12314053/diff/4003/chrome/browser/metrics/variations/variations_service.cc File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/4003/chrome/browser/metrics/variations/variations_service.cc#newcode295 chrome/browser/metrics/variations/variations_service.cc:295: int64 internal_now = base::Time::Now().ToInternalValue(); nit: I'd recommend ...
7 years, 10 months ago (2013-02-22 02:05:04 UTC) #8
Alexei Svitkine (slow)
https://codereview.chromium.org/12314053/diff/4003/chrome/browser/metrics/variations/variations_service.cc File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/4003/chrome/browser/metrics/variations/variations_service.cc#newcode304 chrome/browser/metrics/variations/variations_service.cc:304: UMA_HISTOGRAM_CUSTOM_COUNTS("Variations.SeedFreshness", delta.InMinutes(), Doing this here will not give us ...
7 years, 10 months ago (2013-02-22 14:13:10 UTC) #9
SteveT
Back to Alexei (Ilya's comments not addressed yet). https://codereview.chromium.org/12314053/diff/4003/chrome/browser/metrics/variations/variations_service.cc File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/4003/chrome/browser/metrics/variations/variations_service.cc#newcode304 chrome/browser/metrics/variations/variations_service.cc:304: UMA_HISTOGRAM_CUSTOM_COUNTS("Variations.SeedFreshness", ...
7 years, 10 months ago (2013-02-22 15:17:54 UTC) #10
Alexei Svitkine (slow)
On 2013/02/22 15:17:54, SteveT wrote: > Back to Alexei (Ilya's comments not addressed yet). > ...
7 years, 10 months ago (2013-02-22 15:25:39 UTC) #11
SteveT
On 2013/02/22 15:25:39, Alexei Svitkine wrote: > On 2013/02/22 15:17:54, SteveT wrote: > > Back ...
7 years, 10 months ago (2013-02-22 15:33:30 UTC) #12
Alexei Svitkine (slow)
On 2013/02/22 15:33:30, SteveT wrote: > On 2013/02/22 15:25:39, Alexei Svitkine wrote: > > On ...
7 years, 10 months ago (2013-02-22 15:38:25 UTC) #13
SteveT
Made the suggested changes (and incorporated Ilya's suggestions). Back to you. https://codereview.chromium.org/12314053/diff/4003/chrome/browser/metrics/variations/variations_service.cc File chrome/browser/metrics/variations/variations_service.cc (right): ...
7 years, 10 months ago (2013-02-22 16:14:35 UTC) #14
Alexei Svitkine (slow)
https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/variations/variations_service.cc File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/variations/variations_service.cc#newcode182 chrome/browser/metrics/variations/variations_service.cc:182: base::Time now = base::Time::Now(); Move this into the if ...
7 years, 10 months ago (2013-02-22 16:26:25 UTC) #15
SteveT
Back to you. Ilya: Question for you inline re: Histograms. https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/variations/variations_service.cc File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/variations/variations_service.cc#newcode182 ...
7 years, 10 months ago (2013-02-22 18:12:03 UTC) #16
Alexei Svitkine (slow)
Looking good, pending on Ilya's clarification about the histogram. One comment below, though. https://codereview.chromium.org/12314053/diff/5/chrome/browser/metrics/variations/variations_service.cc File ...
7 years, 10 months ago (2013-02-22 18:46:51 UTC) #17
SteveT
Back to you. What do you think? https://codereview.chromium.org/12314053/diff/5/chrome/browser/metrics/variations/variations_service.cc File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/5/chrome/browser/metrics/variations/variations_service.cc#newcode307 chrome/browser/metrics/variations/variations_service.cc:307: local_state_->SetInt64(prefs::kVariationsLastFetchTime, What ...
7 years, 10 months ago (2013-02-22 20:15:36 UTC) #18
Alexei Svitkine (slow)
LGTM with a suggestion that can avoid the additional if statement. Please wait for confirmation ...
7 years, 10 months ago (2013-02-22 20:18:01 UTC) #19
SteveT
Okay cool. Just waiting on Ilya, then. https://codereview.chromium.org/12314053/diff/13004/chrome/browser/metrics/variations/variations_service.cc File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/13004/chrome/browser/metrics/variations/variations_service.cc#newcode315 chrome/browser/metrics/variations/variations_service.cc:315: UMA_HISTOGRAM_MEDIUM_TIMES("Variations.FetchNotModifiedLatency", latency); ...
7 years, 10 months ago (2013-02-22 21:03:19 UTC) #20
Ilya Sherman
https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/variations/variations_service.cc File chrome/browser/metrics/variations/variations_service.cc (right): https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/variations/variations_service.cc#newcode186 chrome/browser/metrics/variations/variations_service.cc:186: DCHECK_GE(now.ToInternalValue(), last_fetch_time_internal); On 2013/02/22 18:12:03, SteveT wrote: > Removed ...
7 years, 10 months ago (2013-02-22 23:26:16 UTC) #21
SteveT
On 2013/02/22 23:26:16, Ilya Sherman wrote: > https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/variations/variations_service.cc > File chrome/browser/metrics/variations/variations_service.cc (right): > > https://codereview.chromium.org/12314053/diff/10004/chrome/browser/metrics/variations/variations_service.cc#newcode186 ...
7 years, 10 months ago (2013-02-22 23:33:12 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevet@chromium.org/12314053/6023
7 years, 10 months ago (2013-02-22 23:35:03 UTC) #23
commit-bot: I haz the power
Retried try job too often on linux_chromeos for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chromeos&number=86918
7 years, 10 months ago (2013-02-23 00:34:26 UTC) #24
Alexei Svitkine (slow)
One thing I realised this isn't handling is the first-run case via master_prefs, per: https://chromiumcodereview.appspot.com/12223062 ...
7 years, 10 months ago (2013-02-23 15:42:35 UTC) #25
SteveT
On 2013/02/23 15:42:35, Alexei Svitkine wrote: > One thing I realised this isn't handling is ...
7 years, 10 months ago (2013-02-25 15:09:32 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevet@chromium.org/12314053/9013
7 years, 10 months ago (2013-02-25 16:06:07 UTC) #27
commit-bot: I haz the power
7 years, 10 months ago (2013-02-25 18:49:28 UTC) #28
Message was sent while issue was closed.
Change committed as 184452

Powered by Google App Engine
This is Rietveld 408576698