Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2862743002:
Add a metric to record http response code for download requests. (Closed)
DescriptionAdd a metric to record http response code for download requests.
Currently we notice an issue according to metric data that parallel
download feature has a higher SERVER_FAIL interruption type.
we would like to know more details about the http response code from
the server to analyze the cause. This metric only applies to download
requests and will also be helpful to track general download system health.
BUG=718465
Review-Url: https://codereview.chromium.org/2862743002
Cr-Commit-Position: refs/heads/master@{#469888}
Committed: https://chromium.googlesource.com/chromium/src/+/503e20c58c8fdae480d5e7bbc538365f17e32fa1
Patch Set 1 #
Total comments: 3
Patch Set 2 : Add histogram enum. #
Total comments: 2
Patch Set 3 : Fix an issue for tests, some tests don't have response headers. #
Messages
Total messages: 49 (33 generated)
Description was changed from ========== Add a metric to record http response code for download requests. Currently we notice an issue according to metric data that parallel download feature has a higher SERVER_FAIL interruption type. we would like to know more details about the http response code from the server to analyze the cause. This metric only applies to download requests. This will probably need to be merged to M59 beta. BUG=717719 ========== to ========== Add a metric to record http response code for download requests. Currently we notice an issue according to metric data that parallel download feature has a higher SERVER_FAIL interruption type. we would like to know more details about the http response code from the server to analyze the cause. This metric only applies to download requests. BUG=717719 ==========
Description was changed from ========== Add a metric to record http response code for download requests. Currently we notice an issue according to metric data that parallel download feature has a higher SERVER_FAIL interruption type. we would like to know more details about the http response code from the server to analyze the cause. This metric only applies to download requests. BUG=717719 ========== to ========== Add a metric to record http response code for download requests. Currently we notice an issue according to metric data that parallel download feature has a higher SERVER_FAIL interruption type. we would like to know more details about the http response code from the server to analyze the cause. This metric only applies to download requests and will be helpful to track general download system health. BUG=717719 ==========
Description was changed from ========== Add a metric to record http response code for download requests. Currently we notice an issue according to metric data that parallel download feature has a higher SERVER_FAIL interruption type. we would like to know more details about the http response code from the server to analyze the cause. This metric only applies to download requests and will be helpful to track general download system health. BUG=717719 ========== to ========== Add a metric to record http response code for download requests. Currently we notice an issue according to metric data that parallel download feature has a higher SERVER_FAIL interruption type. we would like to know more details about the http response code from the server to analyze the cause. This metric only applies to download requests and will also be helpful to track general download system health. BUG=717719 ==========
xingliu@chromium.org changed reviewers: + dtrainor@chromium.org, qinmin@chromium.org
Hi, PTAL, thanks.
xingliu@chromium.org changed reviewers: + isherman@chromium.org
+isherman@ for histogram review, thanks.
metrics LGTM % a question: https://codereview.chromium.org/2862743002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2862743002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:13934: +<histogram name="Download.HttpResponseCode"> Is there an enum that's appropriate to associate with this histogram?
lgtm % Ilya's comments.
Description was changed from ========== Add a metric to record http response code for download requests. Currently we notice an issue according to metric data that parallel download feature has a higher SERVER_FAIL interruption type. we would like to know more details about the http response code from the server to analyze the cause. This metric only applies to download requests and will also be helpful to track general download system health. BUG=717719 ========== to ========== Add a metric to record http response code for download requests. Currently we notice an issue according to metric data that parallel download feature has a higher SERVER_FAIL interruption type. we would like to know more details about the http response code from the server to analyze the cause. This metric only applies to download requests and will also be helpful to track general download system health. BUG=718465 ==========
Thanks for the review, address comments. https://codereview.chromium.org/2862743002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2862743002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:13934: +<histogram name="Download.HttpResponseCode"> On 2017/05/04 06:06:50, Ilya Sherman wrote: > Is there an enum that's appropriate to associate with this histogram? No, this metric logs all the http response code. "Net.HttpResponseCode" also do this, but it collects the response code for all the network quests, where we need the code for download requests.
https://codereview.chromium.org/2862743002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2862743002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:13934: +<histogram name="Download.HttpResponseCode"> On 2017/05/04 16:52:41, xingliu wrote: > On 2017/05/04 06:06:50, Ilya Sherman wrote: > > Is there an enum that's appropriate to associate with this histogram? > > No, this metric logs all the http response code. "Net.HttpResponseCode" also do > this, but it collects the response code for all the network quests, where we > need the code for download requests. > It looks like the HttpResponseCode enum is appropriate for recording HTTP response codes. Am I misunderstanding? Perhaps to clarify: I'm simply suggesting adding the attribute enum="HttpResponseCode" to this histogram. I'm not suggesting making any changes to the C++ code.
On 2017/05/04 19:39:24, Ilya Sherman wrote: > https://codereview.chromium.org/2862743002/diff/1/tools/metrics/histograms/hi... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2862743002/diff/1/tools/metrics/histograms/hi... > tools/metrics/histograms/histograms.xml:13934: +<histogram > name="Download.HttpResponseCode"> > On 2017/05/04 16:52:41, xingliu wrote: > > On 2017/05/04 06:06:50, Ilya Sherman wrote: > > > Is there an enum that's appropriate to associate with this histogram? > > > > No, this metric logs all the http response code. "Net.HttpResponseCode" also > do > > this, but it collects the response code for all the network quests, where we > > need the code for download requests. > > > > It looks like the HttpResponseCode enum is appropriate for recording HTTP > response codes. Am I misunderstanding? > > Perhaps to clarify: I'm simply suggesting adding the attribute > enum="HttpResponseCode" to this histogram. I'm not suggesting making any > changes to the C++ code. Oh, I see, fixed. Sorry that I didn't notice there is already an enum defined in histogram.xml.
Metrics LGTM, thanks.
The CQ bit was checked by xingliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2862743002/#ps20001 (title: "Add histogram enum.")
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 xingliu@chromium.org
https://codereview.chromium.org/2862743002/diff/20001/content/browser/downloa... File content/browser/download/download_request_core.cc (right): https://codereview.chromium.org/2862743002/diff/20001/content/browser/downloa... content/browser/download/download_request_core.cc:254: RecordDownloadHttpResponseCode( I think you should do this in OnResponseCompleted rather than here
https://codereview.chromium.org/2862743002/diff/20001/content/browser/downloa... File content/browser/download/download_request_core.cc (right): https://codereview.chromium.org/2862743002/diff/20001/content/browser/downloa... content/browser/download/download_request_core.cc:254: RecordDownloadHttpResponseCode( On 2017/05/04 22:30:58, qinmin wrote: > I think you should do this in OnResponseCompleted rather than here Never mind, SERVER_FAILED was only reported in HandleSuccessfulServerResponse, but I think we will be more interested in all the interruption reasons, with the majority coming from OnResponseCompleted().
On 2017/05/04 22:47:31, qinmin wrote: > https://codereview.chromium.org/2862743002/diff/20001/content/browser/downloa... > File content/browser/download/download_request_core.cc (right): > > https://codereview.chromium.org/2862743002/diff/20001/content/browser/downloa... > content/browser/download/download_request_core.cc:254: > RecordDownloadHttpResponseCode( > On 2017/05/04 22:30:58, qinmin wrote: > > I think you should do this in OnResponseCompleted rather than here > > Never mind, SERVER_FAILED was only reported in HandleSuccessfulServerResponse, > but I think we will be more interested in all the interruption reasons, with the > majority coming from OnResponseCompleted(). We have reported all reasons in DownloadItemImpl. I think our goal is to track finer granularity for the interrupt reasons. So the interrupt reason has 3 sources, #1. http response code, #2. net error(socket error mostly), #3. others, like user action. This CL tracks all #1, no matter how download interrupt reason changed. I will add some more interrupt reason for some net::error and one http code, only for handling these cases in code. wdyt?
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/05/04 23:05:04, xingliu wrote: > On 2017/05/04 22:47:31, qinmin wrote: > > > https://codereview.chromium.org/2862743002/diff/20001/content/browser/downloa... > > File content/browser/download/download_request_core.cc (right): > > > > > https://codereview.chromium.org/2862743002/diff/20001/content/browser/downloa... > > content/browser/download/download_request_core.cc:254: > > RecordDownloadHttpResponseCode( > > On 2017/05/04 22:30:58, qinmin wrote: > > > I think you should do this in OnResponseCompleted rather than here > > > > Never mind, SERVER_FAILED was only reported in HandleSuccessfulServerResponse, > > but I think we will be more interested in all the interruption reasons, with > the > > majority coming from OnResponseCompleted(). > > We have reported all reasons in DownloadItemImpl. I think our goal is to track > finer granularity for the interrupt reasons. > > So the interrupt reason has 3 sources, #1. http response code, #2. net > error(socket error mostly), #3. others, like user action. > > This CL tracks all #1, no matter how download interrupt reason changed. > > I will add some more interrupt reason for some net::error and one http code, > only for handling these cases in code. > > wdyt? sounds good to me
lgtm
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xingliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, qinmin@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2862743002/#ps40001 (title: "Fix an issue for tests, some tests don't have response headers.")
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": 40001, "attempt_start_ts": 1494100361069230,
"parent_rev": "3ba7b83974443ba8324d704117743c7ae463d4ed", "commit_rev":
"503e20c58c8fdae480d5e7bbc538365f17e32fa1"}
Message was sent while issue was closed.
Description was changed from ========== Add a metric to record http response code for download requests. Currently we notice an issue according to metric data that parallel download feature has a higher SERVER_FAIL interruption type. we would like to know more details about the http response code from the server to analyze the cause. This metric only applies to download requests and will also be helpful to track general download system health. BUG=718465 ========== to ========== Add a metric to record http response code for download requests. Currently we notice an issue according to metric data that parallel download feature has a higher SERVER_FAIL interruption type. we would like to know more details about the http response code from the server to analyze the cause. This metric only applies to download requests and will also be helpful to track general download system health. BUG=718465 Review-Url: https://codereview.chromium.org/2862743002 Cr-Commit-Position: refs/heads/master@{#469888} Committed: https://chromium.googlesource.com/chromium/src/+/503e20c58c8fdae480d5e7bbc538... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/503e20c58c8fdae480d5e7bbc538... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
