Add Sync.Attachments.DownloadTotalTime.

Add Sync.Attachments.DownloadTotalTime.

Track the total time a download takes, including network and server
overhead.

BUG=436134
Committed: https://crrev.com/80a68ab79fe0258212c88eb5154907be93ff4901
Cr-Commit-Position: refs/heads/master@{#308146}

[maxbogue, 2014-12-11 20:31:14]

maxbogue@chromium.org changed reviewers:
+ asvitkine@chromium.org, maniscalco@chromium.org, pavely@chromium.org

[maxbogue, 2014-12-11 20:31:14]

PTAL! I'm currently only logging the time for HTTP_OK responses, though I'm not
entirely sure that's correct.

[Alexei Svitkine (slow), 2014-12-11 22:01:56]

How about just storing TimeTicks::Now() on the DownloadState - seems
simpler and
more efficient than creating a new object?

The benefit of ElapsedTimer is it's easy to use on the stack. Whereas here
you
don't get that benefit.

On Thu, Dec 11, 2014 at 3:31 PM, <maxbogue@chromium.org> wrote:

> Reviewers: asvitkine (OOO Dec13 - Jan4), maniscalco, pavely,
>
> Message:
> PTAL! I'm currently only logging the time for HTTP_OK responses, though
> I'm not
> entirely sure that's correct.
>
> Description:
> Add Sync.Attachments.DownloadTotalTime.
>
> Track the total time a download takes, including network and server
> overhead.
>
> BUG=436134
>
> Please review this at https://codereview.chromium.org/796953002/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+15, -0 lines):
>   M sync/internal_api/attachments/attachment_downloader_impl.cc
>   M tools/metrics/histograms/histograms.xml
>
>
> Index: sync/internal_api/attachments/attachment_downloader_impl.cc
> diff --git a/sync/internal_api/attachments/attachment_downloader_impl.cc
> b/sync/internal_api/attachments/attachment_downloader_impl.cc
> index 33ac885a8f200be3647a38d1f63a7c24555f200d..
> cb699f92dbe77e1b7dcdfdecb50eb67f8b057fa4 100644
> --- a/sync/internal_api/attachments/attachment_downloader_impl.cc
> +++ b/sync/internal_api/attachments/attachment_downloader_impl.cc
> @@ -8,8 +8,10 @@
>  #include "base/bind.h"
>  #include "base/message_loop/message_loop.h"
>  #include "base/metrics/histogram.h"
> +#include "base/metrics/histogram_macros.h"
>  #include "base/metrics/sparse_histogram.h"
>  #include "base/sys_byteorder.h"
> +#include "base/timer/elapsed_timer.h"
>  #include "net/base/load_flags.h"
>  #include "net/http/http_response_headers.h"
>  #include "net/http/http_status_code.h"
> @@ -35,6 +37,7 @@ struct AttachmentDownloaderImpl::DownloadState {
>    std::string access_token;
>    scoped_ptr<net::URLFetcher> url_fetcher;
>    std::vector<DownloadCallback> user_callbacks;
> +  scoped_ptr<base::ElapsedTimer> timer;
>  };
>
>  AttachmentDownloaderImpl::DownloadState::DownloadState(
> @@ -109,6 +112,8 @@ void AttachmentDownloaderImpl::OnGetTokenSuccess(
>      download_state->access_token = access_token;
>      download_state->url_fetcher =
>          CreateFetcher(download_state->attachment_url,
> access_token).Pass();
> +    scoped_ptr<base::ElapsedTimer> timer(new base::ElapsedTimer());
> +    download_state->timer = timer.Pass();
>      download_state->url_fetcher->Start();
>    }
>    requests_waiting_for_access_token_.clear();
> @@ -159,6 +164,9 @@ void AttachmentDownloaderImpl::OnURLFetchComplete(
>      source->GetResponseAsString(&data_as_string);
>      attachment_data = base::RefCountedString::
> TakeString(&data_as_string);
>
> +    UMA_HISTOGRAM_LONG_TIMES("Sync.Attachments.DownloadTotalTime",
> +                             download_state.timer->Elapsed());
> +
>      attachment_crc32c = ComputeCrc32c(attachment_data);
>      uint32_t crc32c_from_headers = 0;
>      if (ExtractCrc32c(source->GetResponseHeaders(),
> &crc32c_from_headers) &&
> Index: tools/metrics/histograms/histograms.xml
> diff --git a/tools/metrics/histograms/histograms.xml
> b/tools/metrics/histograms/histograms.xml
> index e036ff65e8913ad338ca5e362fa76b0a292d2bcb..
> e9e2b3401ade03e684f1d9855cae5d344c490b24 100644
> --- a/tools/metrics/histograms/histograms.xml
> +++ b/tools/metrics/histograms/histograms.xml
> @@ -35977,6 +35977,13 @@ Therefore, the affected-histogram name has to
> have at least one dot in it.
>    <summary>Response or error codes from downloading sync
> attachments.</summary>
>  </histogram>
>
> +<histogram name="Sync.Attachments.DownloadTotalTime"
> units="milliseconds">
> +  <owner>maxbogue@chromium.org</owner>
> +  <summary>
> +    The total time a download takes including request and server overhead.
> +  </summary>
> +</histogram>
> +
>  <histogram name="Sync.Attachments.StoreInitResult"
>      enum="SyncAttachmentStoreResult">
>    <owner>maxbogue@chromium.org</owner>
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[maxbogue, 2014-12-12 00:02:07]

On 2014/12/11 22:01:56, asvitkine (OOO Dec13 - Jan4) wrote:
> How about just storing TimeTicks::Now() on the DownloadState - seems
> simpler and
> more efficient than creating a new object?
> 
> The benefit of ElapsedTimer is it's easy to use on the stack. Whereas here
> you
> don't get that benefit.

Done. Nick: I think this is much simpler. No memory management and TimeTicks
provides the guarantee you were after anyways.

[Alexei Svitkine (slow), 2014-12-12 16:19:03]

lgtm

[pavely, 2014-12-12 18:33:49]

lgtm

[maxbogue, 2014-12-12 18:41:42]

The CQ bit was checked by maxbogue@chromium.org

[commit-bot: I haz the power, 2014-12-12 18:42:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/796953002/40001

[commit-bot: I haz the power, 2014-12-12 20:01:21]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-12 20:02:07]

Patchset 3 (id:??) landed as
https://crrev.com/80a68ab79fe0258212c88eb5154907be93ff4901
Cr-Commit-Position: refs/heads/master@{#308146}