Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2565173006:
Measure time between secure time queries (Closed)
DescriptionMeasure time between secure time queries
This CL adds an UMA histogram recording the delta between each timestamp
received via a secure time service query and the previous one. This
should give us an idea of how often users in various experimental groups
make secure time queries.
BUG=589700
Committed: https://crrev.com/108ee802fd2569b42ffebd6216a4263fab0e7f45
Cr-Commit-Position: refs/heads/master@{#438590}
Patch Set 1 #
Total comments: 7
Patch Set 2 : isherman comments #
Total comments: 12
Patch Set 3 : meacer comments #
Total comments: 2
Patch Set 4 : fix static consts #
Messages
Total messages: 29 (18 generated)
The CQ bit was checked by estark@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...
estark@chromium.org changed reviewers: + isherman@chromium.org, meacer@chromium.org
meacer, can you please review components/network_time? isherman, can you please review histograms.xml? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2565173006/diff/1/components/network_time/net... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/2565173006/diff/1/components/network_time/net... components/network_time/network_time_tracker.cc:546: base::TimeDelta::FromDays(7), 100); Hmm, do you really need 100 buckets, or would 50 suffice? (Actually, 1 hour to 7 days is a very small range, so probably even fewer than 50 buckets would suffice.) https://codereview.chromium.org/2565173006/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2565173006/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:38114: + and the previous one, to measure how often time queries are made. Please document that the first event is dropped, rather than handled specially in some other way.
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Thanks, isherman. https://codereview.chromium.org/2565173006/diff/1/components/network_time/net... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/2565173006/diff/1/components/network_time/net... components/network_time/network_time_tracker.cc:546: base::TimeDelta::FromDays(7), 100); On 2016/12/13 22:02:07, Ilya Sherman wrote: > Hmm, do you really need 100 buckets, or would 50 suffice? (Actually, 1 hour to > 7 days is a very small range, so probably even fewer than 50 buckets would > suffice.) Changed to 50. (I take it there's a cost to having more buckets?) https://codereview.chromium.org/2565173006/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2565173006/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:38114: + and the previous one, to measure how often time queries are made. On 2016/12/13 22:02:07, Ilya Sherman wrote: > Please document that the first event is dropped, rather than handled specially > in some other way. Done.
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.
Metrics LGTM, thanks. https://codereview.chromium.org/2565173006/diff/1/components/network_time/net... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/2565173006/diff/1/components/network_time/net... components/network_time/network_time_tracker.cc:546: base::TimeDelta::FromDays(7), 100); On 2016/12/13 22:09:36, estark wrote: > On 2016/12/13 22:02:07, Ilya Sherman wrote: > > Hmm, do you really need 100 buckets, or would 50 suffice? (Actually, 1 hour > to > > 7 days is a very small range, so probably even fewer than 50 buckets would > > suffice.) > > Changed to 50. (I take it there's a cost to having more buckets?) Yep, there's a cost on the client in terms of memory, and on the server in terms of processing time. It's not a big cost for any individual histogram; but we have a lot of histograms in Chromium, and it adds up =)
https://codereview.chromium.org/2565173006/diff/1/components/network_time/net... File components/network_time/network_time_tracker_unittest.cc (right): https://codereview.chromium.org/2565173006/diff/1/components/network_time/net... components/network_time/network_time_tracker_unittest.cc:870: const double kJsTimes[3] = {1481653709754, 1481653820879, 1481653880185}; You can make these static. https://codereview.chromium.org/2565173006/diff/20001/components/network_time... File components/network_time/network_time_tracker_unittest.cc (right): https://codereview.chromium.org/2565173006/diff/20001/components/network_time... components/network_time/network_time_tracker_unittest.cc:42: "NetworkTimeTracker.TimeBetweenFetchedTimes"; nit: Maybe rename to TimeBetweenFetches? Saying Time two times can be a bit confusing from time to time. https://codereview.chromium.org/2565173006/diff/20001/components/network_time... components/network_time/network_time_tracker_unittest.cc:820: // times. MultipleGoodTimeResponseHandler is like nit: times fits in the previous line, and perhaps the rest could be wrapped differently too. https://codereview.chromium.org/2565173006/diff/20001/components/network_time... components/network_time/network_time_tracker_unittest.cc:835: unsigned int num_responses = sizeof(kJsTimes) / sizeof(double); You can use arraysize(kJsTimes) instead (and maybe do it inside the if condition) https://codereview.chromium.org/2565173006/diff/20001/components/network_time... components/network_time/network_time_tracker_unittest.cc:850: if (i >= sizeof(kJsTimes) / sizeof(double)) arraysize https://codereview.chromium.org/2565173006/diff/20001/components/network_time... components/network_time/network_time_tracker_unittest.cc:870: const double kJsTimes[3] = {1481653709754, 1481653820879, 1481653880185}; static, this and the other consts. https://codereview.chromium.org/2565173006/diff/20001/components/network_time... components/network_time/network_time_tracker_unittest.cc:936: base::HistogramTester histograms2; It's not clear to me why we are using a second histogram here. Maybe document?
The CQ bit was checked by estark@chromium.org to run a CQ dry run
https://codereview.chromium.org/2565173006/diff/1/components/network_time/net... File components/network_time/network_time_tracker_unittest.cc (right): https://codereview.chromium.org/2565173006/diff/1/components/network_time/net... components/network_time/network_time_tracker_unittest.cc:870: const double kJsTimes[3] = {1481653709754, 1481653820879, 1481653880185}; On 2016/12/14 01:44:38, Mustafa Emre Acer wrote: > You can make these static. Done. https://codereview.chromium.org/2565173006/diff/20001/components/network_time... File components/network_time/network_time_tracker_unittest.cc (right): https://codereview.chromium.org/2565173006/diff/20001/components/network_time... components/network_time/network_time_tracker_unittest.cc:42: "NetworkTimeTracker.TimeBetweenFetchedTimes"; On 2016/12/14 01:44:38, Mustafa Emre Acer wrote: > nit: Maybe rename to TimeBetweenFetches? Saying Time two times can be a bit > confusing from time to time. Done. https://codereview.chromium.org/2565173006/diff/20001/components/network_time... components/network_time/network_time_tracker_unittest.cc:820: // times. MultipleGoodTimeResponseHandler is like On 2016/12/14 01:44:38, Mustafa Emre Acer wrote: > nit: times fits in the previous line, and perhaps the rest could be wrapped > differently too. Done. https://codereview.chromium.org/2565173006/diff/20001/components/network_time... components/network_time/network_time_tracker_unittest.cc:835: unsigned int num_responses = sizeof(kJsTimes) / sizeof(double); On 2016/12/14 01:44:38, Mustafa Emre Acer wrote: > You can use arraysize(kJsTimes) instead (and maybe do it inside the if > condition) Done. https://codereview.chromium.org/2565173006/diff/20001/components/network_time... components/network_time/network_time_tracker_unittest.cc:850: if (i >= sizeof(kJsTimes) / sizeof(double)) On 2016/12/14 01:44:38, Mustafa Emre Acer wrote: > arraysize Done. https://codereview.chromium.org/2565173006/diff/20001/components/network_time... components/network_time/network_time_tracker_unittest.cc:870: const double kJsTimes[3] = {1481653709754, 1481653820879, 1481653880185}; On 2016/12/14 01:44:38, Mustafa Emre Acer wrote: > static, this and the other consts. Done. (This got kinda weird: once I made it static, it requires an out-of-line definition to link properly. And it also had to be constexpr. I don't understand C++.) https://codereview.chromium.org/2565173006/diff/20001/components/network_time... components/network_time/network_time_tracker_unittest.cc:936: base::HistogramTester histograms2; On 2016/12/14 01:44:38, Mustafa Emre Acer wrote: > It's not clear to me why we are using a second histogram here. Maybe document? I was doing this to avoid conflating this sample with previous one (they happen to both go in the same bucket, but I didn't want to rely on that in the test, I just wanted to check that this sample goes in the correct bucket). But as I started to write a comment, I realized I don't think this third query is actually testing anything useful -- it doesn't add anything that the second query isn't already testing -- so I just removed it.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Lgtm https://codereview.chromium.org/2565173006/diff/40001/components/network_time... File components/network_time/network_time_tracker_unittest.cc (right): https://codereview.chromium.org/2565173006/diff/40001/components/network_time... components/network_time/network_time_tracker_unittest.cc:881: constexpr char const* MultipleGoodTimeResponseHandler::kTimeProofHeaders[]; Hmm, I think you can do away with the constexpr if you declare the variable inside the class but define it in here. Something like class MultipleGoodTimeResponseHandler { static const double kJsTimes; } const double MultipleGoodTimeResponseHandler::kJsTimes = {...} Not sure about constexpr either, but this is a common idiom for statics.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2565173006/diff/40001/components/network_time... File components/network_time/network_time_tracker_unittest.cc (right): https://codereview.chromium.org/2565173006/diff/40001/components/network_time... components/network_time/network_time_tracker_unittest.cc:881: constexpr char const* MultipleGoodTimeResponseHandler::kTimeProofHeaders[]; On 2016/12/14 02:36:14, Mustafa Emre Acer wrote: > Hmm, I think you can do away with the constexpr if you declare the variable > inside the class but define it in here. > Something like > > class MultipleGoodTimeResponseHandler { > static const double kJsTimes; > } > > const double MultipleGoodTimeResponseHandler::kJsTimes = {...} > > Not sure about constexpr either, but this is a common idiom for statics. Done, thanks.
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, meacer@chromium.org Link to the patchset: https://codereview.chromium.org/2565173006/#ps60001 (title: "fix static consts")
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": 60001, "attempt_start_ts": 1481733608848190,
"parent_rev": "58a047adeef20f3c22770116a3b4e3677499ae4d", "commit_rev":
"cbc4c3972b317b7933679a5e630085d129187e71"}
Message was sent while issue was closed.
Description was changed from ========== Measure time between secure time queries This CL adds an UMA histogram recording the delta between each timestamp received via a secure time service query and the previous one. This should give us an idea of how often users in various experimental groups make secure time queries. BUG=589700 ========== to ========== Measure time between secure time queries This CL adds an UMA histogram recording the delta between each timestamp received via a secure time service query and the previous one. This should give us an idea of how often users in various experimental groups make secure time queries. BUG=589700 Review-Url: https://codereview.chromium.org/2565173006 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Measure time between secure time queries This CL adds an UMA histogram recording the delta between each timestamp received via a secure time service query and the previous one. This should give us an idea of how often users in various experimental groups make secure time queries. BUG=589700 Review-Url: https://codereview.chromium.org/2565173006 ========== to ========== Measure time between secure time queries This CL adds an UMA histogram recording the delta between each timestamp received via a secure time service query and the previous one. This should give us an idea of how often users in various experimental groups make secure time queries. BUG=589700 Committed: https://crrev.com/108ee802fd2569b42ffebd6216a4263fab0e7f45 Cr-Commit-Position: refs/heads/master@{#438590} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/108ee802fd2569b42ffebd6216a4263fab0e7f45 Cr-Commit-Position: refs/heads/master@{#438590} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
