Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1937063002:
[NTP Snippets] Add metric for fetch error cases (Closed)
|
Created:
4 years, 7 months ago by mastiz Modified:
4 years, 7 months ago CC:
chromium-reviews, mcwilliams+watch_chromium.org, asvitkine+watch_chromium.org, dgn+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@record_article_fetch_errors_0 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[NTP Snippets] Add metrics for error cases
A new histogram is logged to capture fetch errors, namely net errors (negative) and HTTP errors (positive).
BUG=607524
Committed: https://crrev.com/1fb00d360ecad6f5372294b5d7ca70ff65c61845
Cr-Commit-Position: refs/heads/master@{#391367}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Added comment as suggested. #
Total comments: 1
Patch Set 3 : Adopted FetchHttpResponseOrErrorCode. #
Depends on Patchset: Messages
Total messages: 23 (8 generated)
Description was changed from ========== [NTP Snippets] Add metrics for error cases Two new histograms are logged to capture fetch errors, namely net and HTTP errors. The majority of requests are expected to succeed and thus log nothing. BUG=607524 ========== to ========== [NTP Snippets] Add metrics for error cases Two new histograms are logged to capture fetch errors, namely net and HTTP errors. The majority of requests are expected to succeed and thus log nothing. BUG=607524 ==========
mastiz@chromium.org changed reviewers: + treib@chromium.org
https://codereview.chromium.org/1937063002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1937063002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_fetcher.cc:127: -status.error()); I assume the minus is because the error codes tend to be negative - are we sure that's always the case? URLRequestStatus doesn't seem to say anything to that effect? https://codereview.chromium.org/1937063002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_fetcher.cc:133: source->GetResponseCode()); So this one is recorded on success too, which probably makes sense, but doesn't match the CL description.
Description was changed from ========== [NTP Snippets] Add metrics for error cases Two new histograms are logged to capture fetch errors, namely net and HTTP errors. The majority of requests are expected to succeed and thus log nothing. BUG=607524 ========== to ========== [NTP Snippets] Add metrics for error cases Two new histograms are logged to capture fetch errors, namely net and HTTP errors. BUG=607524 ==========
PTAL. https://codereview.chromium.org/1937063002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1937063002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_fetcher.cc:127: -status.error()); On 2016/05/02 14:41:22, Marc Treib wrote: > I assume the minus is because the error codes tend to be negative - are we sure > that's always the case? URLRequestStatus doesn't seem to say anything to that > effect? Although it's not explicitly documented, URLRequestStatus' constructor has a DCHECK that verifies it's zero or negative. In fact, there's various histograms that make this assumption. https://codereview.chromium.org/1937063002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_fetcher.cc:133: source->GetResponseCode()); On 2016/05/02 14:41:22, Marc Treib wrote: > So this one is recorded on success too, which probably makes sense, but doesn't > match the CL description. You're right, the description was outdated, thanks for pointing out.
lgtm https://codereview.chromium.org/1937063002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1937063002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_fetcher.cc:127: -status.error()); On 2016/05/02 15:11:44, mastiz wrote: > On 2016/05/02 14:41:22, Marc Treib wrote: > > I assume the minus is because the error codes tend to be negative - are we > sure > > that's always the case? URLRequestStatus doesn't seem to say anything to that > > effect? > > Although it's not explicitly documented, URLRequestStatus' constructor has a > DCHECK that verifies it's zero or negative. In fact, there's various histograms > that make this assumption. Alright then. Maybe worth a comment?
On 2016/05/02 15:20:56, Marc Treib wrote: > lgtm > > https://codereview.chromium.org/1937063002/diff/1/components/ntp_snippets/ntp... > File components/ntp_snippets/ntp_snippets_fetcher.cc (right): > > https://codereview.chromium.org/1937063002/diff/1/components/ntp_snippets/ntp... > components/ntp_snippets/ntp_snippets_fetcher.cc:127: -status.error()); > On 2016/05/02 15:11:44, mastiz wrote: > > On 2016/05/02 14:41:22, Marc Treib wrote: > > > I assume the minus is because the error codes tend to be negative - are we > > sure > > > that's always the case? URLRequestStatus doesn't seem to say anything to > that > > > effect? > > > > Although it's not explicitly documented, URLRequestStatus' constructor has a > > DCHECK that verifies it's zero or negative. In fact, there's various > histograms > > that make this assumption. > > Alright then. Maybe worth a comment? Done.
mastiz@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/1937063002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1937063002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:134: UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.FetchResponseCode", You can actually log these two together, see CombinedHttpResponseAndNetErrorCode enum in histograms.xml. Since error codes are negative (which are supported by sparse histograms) and response codes are positive, this works just fine.
PTAL.
Still LGTM!
lgtm
Please update the CL description however since it says two histograms.
Description was changed from ========== [NTP Snippets] Add metrics for error cases Two new histograms are logged to capture fetch errors, namely net and HTTP errors. BUG=607524 ========== to ========== [NTP Snippets] Add metrics for error cases A new histogram is logged to capture fetch errors, namely net errors (negative) and HTTP errors (positive). BUG=607524 ==========
The CQ bit was checked by mastiz@chromium.org
On 2016/05/03 14:55:36, Alexei Svitkine wrote: > Please update the CL description however since it says two histograms. Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1937063002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1937063002/40001
Message was sent while issue was closed.
Description was changed from ========== [NTP Snippets] Add metrics for error cases A new histogram is logged to capture fetch errors, namely net errors (negative) and HTTP errors (positive). BUG=607524 ========== to ========== [NTP Snippets] Add metrics for error cases A new histogram is logged to capture fetch errors, namely net errors (negative) and HTTP errors (positive). BUG=607524 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [NTP Snippets] Add metrics for error cases A new histogram is logged to capture fetch errors, namely net errors (negative) and HTTP errors (positive). BUG=607524 ========== to ========== [NTP Snippets] Add metrics for error cases A new histogram is logged to capture fetch errors, namely net errors (negative) and HTTP errors (positive). BUG=607524 Committed: https://crrev.com/1fb00d360ecad6f5372294b5d7ca70ff65c61845 Cr-Commit-Position: refs/heads/master@{#391367} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1fb00d360ecad6f5372294b5d7ca70ff65c61845 Cr-Commit-Position: refs/heads/master@{#391367} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
