Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2153123002:
Certificate Transparency: Collect metrics on age of SCT vs STH (Closed)
|
Created:
4 years, 5 months ago by Eran Messeri Modified:
4 years, 5 months ago CC:
chromium-reviews, certificate-transparency-chrome_googlegroups.com, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMeasure how often SCTs can't be checked because they're too new
Signed Certificate Timestamps (SCTs) designate Merkle tree leaves that can
be checked for inclusion with a CT Log's Signed Tree Head (STH).
However, inclusion should only be checked against an STH that is newer
than the SCTs timestamp by at least 24 hours - this is because Logs'
have a Maximum Merge Delay of 24 hours, which is the time they have
to produce a new STH that incorporates a given SCT.
STHs are delivered periodically out of band. If there isn't a new enough
STH, then SCTs will need to be marked as pending inclusion check,
waiting for a new STH to be delivered.
To determine how frequently an STH should be delivered (currently,
daily) and how big the SCT queue list should be (that is, how frequently
clients encounter a brand new SCT that is newer than any STH),
measure how often an SCT can't be checked for inclusion immediately
after it's been seen.
BUG=613495
Committed: https://crrev.com/b03a8a77ead185c85a6c55303ac20176eaa20a56
Cr-Commit-Position: refs/heads/master@{#407471}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rename histogram #
Total comments: 6
Patch Set 3 : Addressing review comments #
Total comments: 2
Patch Set 4 : Renaming enum name #
Total comments: 16
Patch Set 5 : Addressing Ryan's comments #
Total comments: 4
Patch Set 6 : Tri-state enum indicating lack of valid STH #
Total comments: 6
Patch Set 7 : Fixing comments #
Messages
Total messages: 59 (34 generated)
Description was changed from ========== Certificate Transparency: Collect metrics on age of SCT vs STH BUG= ========== to ========== Certificate Transparency: Collect metrics on age of SCT vs STH BUG= ==========
eranm@chromium.org changed reviewers: + robpercival@chromium.org
Rob, PTAL.
On 2016/07/15 12:08:10, Eran Messeri wrote: > Rob, PTAL. LGTM, bar a nit. Think we should have a test for the case when we have an old STH and get a new one? The closest we've got is a test for having no STH and then getting a new one, which follows a slightly different code path.
https://codereview.chromium.org/2153123002/diff/1/components/certificate_tran... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/2153123002/diff/1/components/certificate_tran... components/certificate_transparency/single_tree_tracker_unittest.cc:135: histograms.ExpectTotalCount(kPendingSTHHistogramName, 0); I think it's worth adding a comment for this line, repeating what you've said in the implementation about how we don't want to increment this when there is no STH at all.
On 2016/07/15 12:49:51, Rob Percival wrote: > On 2016/07/15 12:08:10, Eran Messeri wrote: > > Rob, PTAL. > > LGTM, bar a nit. Think we should have a test for the case when we have an old > STH and get a new one? The closest we've got is a test for having no STH and > then getting a new one, which follows a slightly different code path. What would you like to test in this scenario? You'll note that no UMA is emitted when a fresh STH is provided which enables some SCTs to be checked for inclusion, as I want this metric to indicate only how many of the observed SCTs can be checked for inclusion immediately.
The CQ bit was checked by eranm@chromium.org to run a CQ dry run
https://codereview.chromium.org/2153123002/diff/1/components/certificate_tran... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/2153123002/diff/1/components/certificate_tran... components/certificate_transparency/single_tree_tracker_unittest.cc:135: histograms.ExpectTotalCount(kPendingSTHHistogramName, 0); On 2016/07/15 14:38:16, Rob Percival wrote: > I think it's worth adding a comment for this line, repeating what you've said in > the implementation about how we don't want to increment this when there is no > STH at all. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
eranm@chromium.org changed reviewers: + asvitkine@chromium.org, rsleevi@chromium.org
On 2016/07/18 07:51:41, Eran Messeri wrote: > On 2016/07/15 12:49:51, Rob Percival wrote: > > On 2016/07/15 12:08:10, Eran Messeri wrote: > > > Rob, PTAL. > > > > LGTM, bar a nit. Think we should have a test for the case when we have an old > > STH and get a new one? The closest we've got is a test for having no STH and > > then getting a new one, which follows a slightly different code path. > > What would you like to test in this scenario? You'll note that no UMA is emitted > when a fresh STH is provided which enables some SCTs to be checked for > inclusion, as I want this metric to indicate only how many of the observed SCTs > can be checked for inclusion immediately. I'd like to test the change in SCT verification state for that scenario. That's outside of the scope of this change though, so the LGTM stands regardless.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2153123002/diff/20001/components/certificate_... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2153123002/diff/20001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:18: "Net.CertificateTransparency.CanInclusionCheckSCT"; If you're only going to use it from the same file once, you might as well inline it into the macro invocation. However, in this case it looks like you're actually interested in this name from the test, so suggest declaring this in the header. In can be in an "internal" namespace or a static class member. https://codereview.chromium.org/2153123002/diff/20001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:53: UMA_HISTOGRAM_BOOLEAN(kCanCheckForInclusionHistogramName, true); Each UMA macro expansion results in a bunch of code. So please refactor to only have this macro once for this histogram. (e.g. either make a helper function that takes the bool param or change the structure of this function. https://codereview.chromium.org/2153123002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2153123002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:26507: + enum="Boolean"> Define a more specific enum, like BooleanCovered or something that actually explicitly spells out the two cases. (Just in the XML).
Thanks for the prompt review, PTAL. https://codereview.chromium.org/2153123002/diff/20001/components/certificate_... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2153123002/diff/20001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:18: "Net.CertificateTransparency.CanInclusionCheckSCT"; On 2016/07/18 15:03:57, Alexei Svitkine (slow) wrote: > If you're only going to use it from the same file once, you might as well inline > it into the macro invocation. > > However, in this case it looks like you're actually interested in this name from > the test, so suggest declaring this in the header. > > In can be in an "internal" namespace or a static class member. Done. https://codereview.chromium.org/2153123002/diff/20001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:53: UMA_HISTOGRAM_BOOLEAN(kCanCheckForInclusionHistogramName, true); On 2016/07/18 15:03:57, Alexei Svitkine (slow) wrote: > Each UMA macro expansion results in a bunch of code. So please refactor to only > have this macro once for this histogram. (e.g. either make a helper function > that takes the bool param or change the structure of this function. Done. https://codereview.chromium.org/2153123002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2153123002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:26507: + enum="Boolean"> On 2016/07/18 15:03:57, Alexei Svitkine (slow) wrote: > Define a more specific enum, like BooleanCovered or something that actually > explicitly spells out the two cases. (Just in the XML). Done.
The CQ bit was checked by eranm@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.
lgtm % comment https://codereview.chromium.org/2153123002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2153123002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:67303: +<enum name="BooleanCanBeChecked" type="int"> Nit: Since this is a very specific enum, name it appropriately. (e.g. STHCanBeChecked). Otherwise, BooleanCanBeChecked sounds like something someone else could re-use if going by just the name.
Alexei, thanks for the review, addressed your comment. Ryan - this is now ready for you to review. https://codereview.chromium.org/2153123002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2153123002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:67303: +<enum name="BooleanCanBeChecked" type="int"> On 2016/07/20 15:14:04, Alexei Svitkine (slow) wrote: > Nit: Since this is a very specific enum, name it appropriately. (e.g. > STHCanBeChecked). Otherwise, BooleanCanBeChecked sounds like something someone > else could re-use if going by just the name. Done.
The CQ bit was checked by eranm@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...
Please significantly expand the CL description. Based on the unit tests and code documentation as is, I have difficulty understanding your goals. In particular, I'm rather confused what "False" is supposed to mean here. The histogram description itself leaves me fairly confused, and while I've tried to reword it partially, I'm still at a loss for what's expected. Concretely, it's non-obvious why observing an SCT before you've observed an STH results in no logging, but observing an SCT after you've observed an STH does. Why is that distinction meaningful? If so, it should be documented - ideally in code, but minimally in the CL description. For example, consider the rather large description on https://codereview.chromium.org/2161983003/ for what is, in effect, 16 line change. It tries to capture why the subtlety is, so that it's clearer in the future. https://codereview.chromium.org/2153123002/diff/60001/components/certificate_... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2153123002/diff/60001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:32: } // namespace Generally we try to have the unnamed namespace at the top. It's unclear if you put internal first because of kCanCheckForInclusionHistogramName, but if so, that wouldn't be necessary, because the .h declares that symbol so you'd be fine referencing it before the definition. https://codereview.chromium.org/2153123002/diff/60001/components/certificate_... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/2153123002/diff/60001/components/certificate_... components/certificate_transparency/single_tree_tracker.h:30: extern const char kCanCheckForInclusionHistogramName[]; This does feel unnecessary, and it doesn't sound like Alexei was requiring it, so I'd like to push back on this a little bit. While I agree it's coupling a unittest to an implementation detail, my main concerns are: Putting it into an internal namespace still preserves global symbol visibility. That is, it doesn't allow the compiler to help protect you from any mistakes. While a static class member (private, with the unit test friended) would be better for this, in that it provides a better guarantee (through compiler-enforce visibility), the fact remains that if the unittest looks at this histogram, it still remains that the unittest is testing the implementation details of the class. As such, I don't think there's much of a line between embedding the histogram name itself in the test, without exposing any symbol, as you're still testing implementation details. Arguably, by duplicating the symbol name, you're making it clearer the contract of the unit tests expects a particular histogram named a particular way to be set. That's my longer justification for why I would say to duplicate in the test. At the risk of code golfing, which may be an anti-pattern, it does seem the duplication is the dominant form. https://codereview.chromium.org/2153123002/diff/60001/components/certificate_... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/2153123002/diff/60001/components/certificate_... components/certificate_transparency/single_tree_tracker_unittest.cc:21: using internal::kCanCheckForInclusionHistogramName; Newline https://codereview.chromium.org/2153123002/diff/60001/components/certificate_... components/certificate_transparency/single_tree_tracker_unittest.cc:94: // Since there's no valid STH expect nothing was logged to UMA. Grammar: Something feels off on this, most likely punctuation (gut says a comma between STH and expect) that said, this could be reworded to be clearer // Expect nothing to be logged because there's no valid STH. That said, from a design perspective, it's unclear why you would expect that nothing's logged if it's pending an STH - I would think that if the histogram is measuring whether or not you can check for inclusion, you would expect that it would be logged if you encounter an SCT that you can't check for inclusion (yet). https://codereview.chromium.org/2153123002/diff/60001/components/certificate_... components/certificate_transparency/single_tree_tracker_unittest.cc:122: // can be checked for inclusion. Similarly, this comment doesn't make sense in the context of the CLs stated goals. To me, the non-obvious part is not what you're testing for here, but why you're testing it, compared with the above test. https://codereview.chromium.org/2153123002/diff/60001/components/certificate_... components/certificate_transparency/single_tree_tracker_unittest.cc:176: // STH when it has STH but the observed SCT is newer than that. "than that" is ambiguous. grammatically, this reads weird, like it's missing punctuation. https://codereview.chromium.org/2153123002/diff/60001/components/certificate_... components/certificate_transparency/single_tree_tracker_unittest.cc:186: tree_tracker_->OnSCTVerified(chain_.get(), cert_sct_.get()); newline between 184-185 and 186-187 https://codereview.chromium.org/2153123002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2153123002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:26580: + immediately without waiting for a new STH. Grammatically, this reads rather weird as a description. In particular, the "in that case" combined with the previous sentence feels fairly ambiguous. For example, it seems based on how you're actually using it, that this is something like: When a Signed Certificate Timestamp (SCT) is observed, measures whether the SCT can be checked against the Signed Tree Head (STH) currently known to the client. An SCT can be checked against an STH if the STH's timestamp is at least 24 hours (the log's Maximum Merge Delay) newer than the SCT. Is this correct?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Certificate Transparency: Collect metrics on age of SCT vs STH BUG= ========== to ========== Signed Certificate Timestamps (SCTs), together with the certificate they relate to, designate Merkle tree leaves that should be checked for inclusion. A Merkle tree leaf can be checked for inclusion once the client has a Signed Tree Head (STH) that's 24 hours older than the timestamp in the SCT (When SCT timestamp + 24h < STH timestamp holds). If the leaf cannot be checked for inclusion because it's too new, then the SCT and certificate should be queued so that an inclusion check will be performed once a newer STH is obtained. BUG=613495 ==========
Description was changed from ========== Signed Certificate Timestamps (SCTs), together with the certificate they relate to, designate Merkle tree leaves that should be checked for inclusion. A Merkle tree leaf can be checked for inclusion once the client has a Signed Tree Head (STH) that's 24 hours older than the timestamp in the SCT (When SCT timestamp + 24h < STH timestamp holds). If the leaf cannot be checked for inclusion because it's too new, then the SCT and certificate should be queued so that an inclusion check will be performed once a newer STH is obtained. BUG=613495 ========== to ========== Signed Certificate Timestamps (SCTs), together with the certificate they relate to, designate Merkle tree leaves that should be checked for inclusion. A Merkle tree leaf can be checked for inclusion once the client has a Signed Tree Head (STH) that's 24 hours older than the timestamp in the SCT (When SCT timestamp + 24h < STH timestamp holds). If the leaf cannot be checked for inclusion because it's too new, then the SCT and certificate should be queued so that an inclusion check will be performed once a newer STH is obtained. What we'd like to measure is how often SCTs have to be queued because they cannot be checked for inclusion immediately. This data will be used to help us decide things like: - Frequency of STH updates (right now STHs are updated daily). - Size of the queue for pending SCTs and whether we should bother persisting them to disk. BUG=613495 ==========
https://codereview.chromium.org/2153123002/diff/60001/components/certificate_... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2153123002/diff/60001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:27: void LogCanBeCheckedForInclusionToUMA(bool can_be_checked) { FYI I've documented, in this function, what we're measuring and why. https://codereview.chromium.org/2153123002/diff/60001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:32: } // namespace On 2016/07/20 20:34:00, Ryan Sleevi (extremely slow) wrote: > Generally we try to have the unnamed namespace at the top. It's unclear if you > put internal first because of kCanCheckForInclusionHistogramName, but if so, > that wouldn't be necessary, because the .h declares that symbol so you'd be fine > referencing it before the definition. Moved the unnamed namespace to the top, will keep that in mind for future changes. https://codereview.chromium.org/2153123002/diff/60001/components/certificate_... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/2153123002/diff/60001/components/certificate_... components/certificate_transparency/single_tree_tracker.h:30: extern const char kCanCheckForInclusionHistogramName[]; On 2016/07/20 20:34:00, Ryan Sleevi (extremely slow) wrote: > This does feel unnecessary, and it doesn't sound like Alexei was requiring it, > so I'd like to push back on this a little bit. > > While I agree it's coupling a unittest to an implementation detail, my main > concerns are: > > Putting it into an internal namespace still preserves global symbol visibility. > That is, it doesn't allow the compiler to help protect you from any mistakes. > While a static class member (private, with the unit test friended) would be > better for this, in that it provides a better guarantee (through > compiler-enforce visibility), the fact remains that if the unittest looks at > this histogram, it still remains that the unittest is testing the implementation > details of the class. > > As such, I don't think there's much of a line between embedding the histogram > name itself in the test, without exposing any symbol, as you're still testing > implementation details. Arguably, by duplicating the symbol name, you're making > it clearer the contract of the unit tests expects a particular histogram named a > particular way to be set. > > That's my longer justification for why I would say to duplicate in the test. > > At the risk of code golfing, which may be an anti-pattern, it does seem the > duplication is the dominant form. Done, duplicated in the test. https://codereview.chromium.org/2153123002/diff/60001/components/certificate_... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/2153123002/diff/60001/components/certificate_... components/certificate_transparency/single_tree_tracker_unittest.cc:21: using internal::kCanCheckForInclusionHistogramName; On 2016/07/20 20:34:00, Ryan Sleevi (extremely slow) wrote: > Newline Done. https://codereview.chromium.org/2153123002/diff/60001/components/certificate_... components/certificate_transparency/single_tree_tracker_unittest.cc:94: // Since there's no valid STH expect nothing was logged to UMA. On 2016/07/20 20:34:00, Ryan Sleevi (extremely slow) wrote: > Grammar: Something feels off on this, most likely punctuation (gut says a comma > between STH and expect) > > that said, this could be reworded to be clearer > > // Expect nothing to be logged because there's no valid STH. Done. > > > That said, from a design perspective, it's unclear why you would expect that > nothing's logged if it's pending an STH - I would think that if the histogram is > measuring whether or not you can check for inclusion, you would expect that it > would be logged if you encounter an SCT that you can't check for inclusion > (yet). Good point. I've attempted to explain it in the documentation for LogCanBeCheckedForInclusionToUMA (single_tree_tracker.cc). The gist is that we'd like to learn how often clients encounter "very new" SCTs and logging UMA when a client does not have a valid STH at all would skew the data as it would log its inability to check inclusion for every SCT observed. https://codereview.chromium.org/2153123002/diff/60001/components/certificate_... components/certificate_transparency/single_tree_tracker_unittest.cc:122: // can be checked for inclusion. On 2016/07/20 20:34:00, Ryan Sleevi (extremely slow) wrote: > Similarly, this comment doesn't make sense in the context of the CLs stated > goals. > > To me, the non-obvious part is not what you're testing for here, but why you're > testing it, compared with the above test. Expanded the comment to document why it's expected to have a value emitted to UMA here. https://codereview.chromium.org/2153123002/diff/60001/components/certificate_... components/certificate_transparency/single_tree_tracker_unittest.cc:176: // STH when it has STH but the observed SCT is newer than that. On 2016/07/20 20:34:00, Ryan Sleevi (extremely slow) wrote: > "than that" is ambiguous. > > grammatically, this reads weird, like it's missing punctuation. Re-worded the comment a bit, removed 'than that' and referred explicitly to the STH. https://codereview.chromium.org/2153123002/diff/60001/components/certificate_... components/certificate_transparency/single_tree_tracker_unittest.cc:186: tree_tracker_->OnSCTVerified(chain_.get(), cert_sct_.get()); On 2016/07/20 20:34:00, Ryan Sleevi (extremely slow) wrote: > newline between 184-185 and 186-187 Done.
The CQ bit was checked by eranm@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...
Description was changed from ========== Signed Certificate Timestamps (SCTs), together with the certificate they relate to, designate Merkle tree leaves that should be checked for inclusion. A Merkle tree leaf can be checked for inclusion once the client has a Signed Tree Head (STH) that's 24 hours older than the timestamp in the SCT (When SCT timestamp + 24h < STH timestamp holds). If the leaf cannot be checked for inclusion because it's too new, then the SCT and certificate should be queued so that an inclusion check will be performed once a newer STH is obtained. What we'd like to measure is how often SCTs have to be queued because they cannot be checked for inclusion immediately. This data will be used to help us decide things like: - Frequency of STH updates (right now STHs are updated daily). - Size of the queue for pending SCTs and whether we should bother persisting them to disk. BUG=613495 ========== to ========== Measure how often SCTs can't be checked because they're too new Signed Certificate Timestamps (SCTs) are Merkle tree leaves that can be checked for inclusion with a CT Log's Signed Tree Head (STH). However, inclusion should only be checked against an STH that is newer than the SCTs timestamp by at least 24 hours - this is because Logs' have a Maximum Merge Delay of 24 hours, which is the time they have to produce a new STH that incorporates a given SCT. STHs are delivered periodically out of band. If there isn't a new enough STH, then SCTs will need to be marked as pending inclusion check, waiting for a new STH to be delivered. To determine how frequently an STH should be delivered (currently, daily) and how big the SCT queue list should be (that is, how frequently clients encounter a brand new SCT that is newer than any STH), measure how often an SCT can't be checked for inclusion immediately after it's been seen. BUG=613495 ==========
I tried to rewrite the CL description to be clearer, and to follow the general guidance of http://chris.beams.io/posts/git-commit/ Which is to say: One line explainer [Further explanation] https://codereview.chromium.org/2153123002/diff/80001/components/certificate_... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2153123002/diff/80001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:31: // logged as if they cannot be checked for inclusion, skewing the data. I don't understand this last comment, from a design perspective. If the client observes an SCT before it has an STH, are you suggesting you wouldn't store that SCT to disk for future inclusion checks? That it wouldn't affect the size of the queue of pending SCTs? That it wouldn't affect the frequency of updates? That's the non-obvious part of all of this, and it doesn't seem to have been answered. 28 - 31 doesn't really feel like it explains why, since if your goal from 18-19 is to measure on first observation, why don't we observe? Is it because you're not delivering STHs? Is it because you're deferring loading of STHs? If so, shouldn't you *also* be measuring how often you encounter an SCT when there's no STHs, to know what's acceptable for the loading/initialization?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2153123002/diff/80001/components/certificate_... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2153123002/diff/80001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:31: // logged as if they cannot be checked for inclusion, skewing the data. On 2016/07/21 18:15:01, Ryan Sleevi (extremely slow) wrote: > I don't understand this last comment, from a design perspective. > > If the client observes an SCT before it has an STH, are you suggesting you > wouldn't store that SCT to disk for future inclusion checks? That it wouldn't > affect the size of the queue of pending SCTs? That it wouldn't affect the > frequency of updates? > > That's the non-obvious part of all of this, and it doesn't seem to have been > answered. 28 - 31 doesn't really feel like it explains why, since if your goal > from 18-19 is to measure on first observation, why don't we observe? > > Is it because you're not delivering STHs? Is it because you're deferring loading > of STHs? If so, shouldn't you *also* be measuring how often you encounter an SCT > when there's no STHs, to know what's acceptable for the loading/initialization? If I understand correctly, the question is why I don't log anything related to the case where the client doesn't have STHs at all in this change. Right now there's a Finch experiment related to the regression caused by STHSet (https://crbug.com/607946) which means some users don't get STH updates at all, so I know data from these users will add noise. STHs should be loaded right after start-up, very few connections should be made during that time. Generally, it doesn't seem to me like we need to report data from users without STHs because: - We have data from the component updater on how many clients receive component updates and how long does it take for an update to propagate, so we don't need to count it here. - The number of users that never get STHs should be very small, but if we include in this metric every SCT that couldn't be checked because there's no valid STH at all, clients that don't have valid STHs will always report that they can't check inclusion, skewing what we're trying to measure. - Whatever decisions (on cache size, persisting to disk, etc) would also apply to clients that don't have valid STHs but because the number of these clients should be very small (waffles@ sent me some data from the component updater, have to further analyze it), and their behaviour pathological (i.e. such clients may never get a fresh STH or get it much later after other clients do) I'm not sure it's sensible to take data from them into account. Does it all make sense?
https://codereview.chromium.org/2153123002/diff/80001/components/certificate_... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2153123002/diff/80001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:31: // logged as if they cannot be checked for inclusion, skewing the data. On 2016/07/21 20:01:33, Eran Messeri wrote: > If I understand correctly, the question is why I don't log anything related to > the case where the client doesn't have STHs at all in this change. > > Right now there's a Finch experiment related to the regression caused by STHSet > (https://crbug.com/607946) which means some users don't get STH updates at all, > so I know data from these users will add noise. No, because we can filter out that population of users. > STHs should be loaded right > after start-up, very few connections should be made during that time. How do you know it's "very few" if you're not measuring it? With the TaskScheduler work suggesting it might take 10 minutes (or more) before these could be loaded, that could be a substantial number of communications. Similarly, on first install, it may be hours. That seems like precisely the thing you should be measuring. > Generally, it doesn't seem to me like we need to report data from users without > STHs because: > - We have data from the component updater on how many clients receive component > updates and how long does it take for an update to propagate, so we don't need > to count it here. But you don't have data on how many connections would be potentially affected, and so any policies you make based upon this data would be inaccurate. > - The number of users that never get STHs should be very small, but if we > include in this metric every SCT that couldn't be checked because there's no > valid STH at all, clients that don't have valid STHs will always report that > they can't check inclusion, skewing what we're trying to measure. But you can already filter that out. > - Whatever decisions (on cache size, persisting to disk, etc) would also apply > to clients that don't have valid STHs but because the number of these clients > should be very small (waffles@ sent me some data from the component updater, > have to further analyze it), and their behaviour pathological (i.e. such clients > may never get a fresh STH or get it much later after other clients do) I'm not > sure it's sensible to take data from them into account. The fact that the decisions you would make would affect them seems to be precisely why you should be taking it into account. > Does it all make sense? I parse your answers, but I do not understand/agree with the conclusions. So yes/no?
From your reply I gather we should measure cases where SCTs are not checked because there's no valid STH at all. Would a tri-state histogram (no valid STH / valid STH but too old / valid STH and covers the SCT) address that need? BTW I was unaware some populations could be filtered, do you refer to filtering by Finch study/group on the web dashboard or more advanced filtering correlating several UMA histograms over the raw data?
On 2016/07/21 20:38:46, Eran Messeri wrote: > From your reply I gather we should measure cases where SCTs are not checked > because there's no valid STH at all. > Would a tri-state histogram (no valid STH / valid STH but too old / valid STH > and covers the SCT) address that need? Yes, I believe so. That's because if a user gets stuck in the "no valid STH" case, they'll be pathologically at the maximum queue size. > BTW I was unaware some populations could be filtered, do you refer to filtering > by Finch study/group on the web dashboard or more advanced filtering correlating > several UMA histograms over the raw data? I was specifically talking about using the Finch Dashboard, selecting your study (Presumably, STHSetComponentStudy), and then evaluating these histograms to see variations between the control group and the disabled group. You can make your decisions based on the ControlGroup's results.
Description was changed from ========== Measure how often SCTs can't be checked because they're too new Signed Certificate Timestamps (SCTs) are Merkle tree leaves that can be checked for inclusion with a CT Log's Signed Tree Head (STH). However, inclusion should only be checked against an STH that is newer than the SCTs timestamp by at least 24 hours - this is because Logs' have a Maximum Merge Delay of 24 hours, which is the time they have to produce a new STH that incorporates a given SCT. STHs are delivered periodically out of band. If there isn't a new enough STH, then SCTs will need to be marked as pending inclusion check, waiting for a new STH to be delivered. To determine how frequently an STH should be delivered (currently, daily) and how big the SCT queue list should be (that is, how frequently clients encounter a brand new SCT that is newer than any STH), measure how often an SCT can't be checked for inclusion immediately after it's been seen. BUG=613495 ========== to ========== Measure how often SCTs can't be checked because they're too new Signed Certificate Timestamps (SCTs) designate Merkle tree leaves that can be checked for inclusion with a CT Log's Signed Tree Head (STH). However, inclusion should only be checked against an STH that is newer than the SCTs timestamp by at least 24 hours - this is because Logs' have a Maximum Merge Delay of 24 hours, which is the time they have to produce a new STH that incorporates a given SCT. STHs are delivered periodically out of band. If there isn't a new enough STH, then SCTs will need to be marked as pending inclusion check, waiting for a new STH to be delivered. To determine how frequently an STH should be delivered (currently, daily) and how big the SCT queue list should be (that is, how frequently clients encounter a brand new SCT that is newer than any STH), measure how often an SCT can't be checked for inclusion immediately after it's been seen. BUG=613495 ==========
The CQ bit was checked by eranm@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...
Per your feedback, switched the histogram from bool to an enum indicating all 3 states: - Needs valid STH. - Needs fresh STH. - Can be checked for inclusion. PTAL. @asvitkine, you may also want to take another look as I've changed the histogram.
https://codereview.chromium.org/2153123002/diff/80001/components/certificate_... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2153123002/diff/80001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:31: // logged as if they cannot be checked for inclusion, skewing the data. On 2016/07/21 20:14:18, Ryan Sleevi (extremely slow) wrote: > On 2016/07/21 20:01:33, Eran Messeri wrote: > > If I understand correctly, the question is why I don't log anything related to > > the case where the client doesn't have STHs at all in this change. > > > > Right now there's a Finch experiment related to the regression caused by > STHSet > > (https://crbug.com/607946) which means some users don't get STH updates at > all, > > so I know data from these users will add noise. > > No, because we can filter out that population of users. > > > STHs should be loaded right > > after start-up, very few connections should be made during that time. > > How do you know it's "very few" if you're not measuring it? With the > TaskScheduler work suggesting it might take 10 minutes (or more) before these > could be loaded, that could be a substantial number of communications. > Similarly, on first install, it may be hours. That seems like precisely the > thing you should be measuring. > > > Generally, it doesn't seem to me like we need to report data from users > without > > STHs because: > > - We have data from the component updater on how many clients receive > component > > updates and how long does it take for an update to propagate, so we don't need > > to count it here. > > But you don't have data on how many connections would be potentially affected, > and so any policies you make based upon this data would be inaccurate. > > > - The number of users that never get STHs should be very small, but if we > > include in this metric every SCT that couldn't be checked because there's no > > valid STH at all, clients that don't have valid STHs will always report that > > they can't check inclusion, skewing what we're trying to measure. > > But you can already filter that out. > > > - Whatever decisions (on cache size, persisting to disk, etc) would also apply > > to clients that don't have valid STHs but because the number of these clients > > should be very small (waffles@ sent me some data from the component updater, > > have to further analyze it), and their behaviour pathological (i.e. such > clients > > may never get a fresh STH or get it much later after other clients do) I'm not > > sure it's sensible to take data from them into account. > > The fact that the decisions you would make would affect them seems to be > precisely why you should be taking it into account. > > > Does it all make sense? > > I parse your answers, but I do not understand/agree with the conclusions. So > yes/no? Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
still lgtm https://codereview.chromium.org/2153123002/diff/100001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2153123002/diff/100001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:18: enum SCTCanBeCheckedForInclusion { Add a comment not to renumber these since they're used in uma.
lgtm https://codereview.chromium.org/2153123002/diff/100001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2153123002/diff/100001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:21: CAN_BE_CHECKED = 2, Document these. This is the perfect place to capture the subtlety :) https://codereview.chromium.org/2153123002/diff/100001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:71: // Do not log histogram if there's no STH for this log yet, as it does Comment needs updating
The CQ bit was checked by eranm@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...
All fixed, will be submitting if the trybots are happy. https://codereview.chromium.org/2153123002/diff/100001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2153123002/diff/100001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:18: enum SCTCanBeCheckedForInclusion { On 2016/07/22 13:37:33, Alexei Svitkine (slow) wrote: > Add a comment not to renumber these since they're used in uma. Done. https://codereview.chromium.org/2153123002/diff/100001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:21: CAN_BE_CHECKED = 2, On 2016/07/22 16:57:30, Ryan Sleevi (extremely slow) wrote: > Document these. This is the perfect place to capture the subtlety :) Done. https://codereview.chromium.org/2153123002/diff/100001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:71: // Do not log histogram if there's no STH for this log yet, as it does On 2016/07/22 16:57:30, Ryan Sleevi (extremely slow) wrote: > Comment needs updating Done.
The CQ bit was unchecked by eranm@chromium.org
The CQ bit was checked by eranm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robpercival@chromium.org, asvitkine@chromium.org, rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/2153123002/#ps120001 (title: "Fixing comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Measure how often SCTs can't be checked because they're too new Signed Certificate Timestamps (SCTs) designate Merkle tree leaves that can be checked for inclusion with a CT Log's Signed Tree Head (STH). However, inclusion should only be checked against an STH that is newer than the SCTs timestamp by at least 24 hours - this is because Logs' have a Maximum Merge Delay of 24 hours, which is the time they have to produce a new STH that incorporates a given SCT. STHs are delivered periodically out of band. If there isn't a new enough STH, then SCTs will need to be marked as pending inclusion check, waiting for a new STH to be delivered. To determine how frequently an STH should be delivered (currently, daily) and how big the SCT queue list should be (that is, how frequently clients encounter a brand new SCT that is newer than any STH), measure how often an SCT can't be checked for inclusion immediately after it's been seen. BUG=613495 ========== to ========== Measure how often SCTs can't be checked because they're too new Signed Certificate Timestamps (SCTs) designate Merkle tree leaves that can be checked for inclusion with a CT Log's Signed Tree Head (STH). However, inclusion should only be checked against an STH that is newer than the SCTs timestamp by at least 24 hours - this is because Logs' have a Maximum Merge Delay of 24 hours, which is the time they have to produce a new STH that incorporates a given SCT. STHs are delivered periodically out of band. If there isn't a new enough STH, then SCTs will need to be marked as pending inclusion check, waiting for a new STH to be delivered. To determine how frequently an STH should be delivered (currently, daily) and how big the SCT queue list should be (that is, how frequently clients encounter a brand new SCT that is newer than any STH), measure how often an SCT can't be checked for inclusion immediately after it's been seen. BUG=613495 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Measure how often SCTs can't be checked because they're too new Signed Certificate Timestamps (SCTs) designate Merkle tree leaves that can be checked for inclusion with a CT Log's Signed Tree Head (STH). However, inclusion should only be checked against an STH that is newer than the SCTs timestamp by at least 24 hours - this is because Logs' have a Maximum Merge Delay of 24 hours, which is the time they have to produce a new STH that incorporates a given SCT. STHs are delivered periodically out of band. If there isn't a new enough STH, then SCTs will need to be marked as pending inclusion check, waiting for a new STH to be delivered. To determine how frequently an STH should be delivered (currently, daily) and how big the SCT queue list should be (that is, how frequently clients encounter a brand new SCT that is newer than any STH), measure how often an SCT can't be checked for inclusion immediately after it's been seen. BUG=613495 ========== to ========== Measure how often SCTs can't be checked because they're too new Signed Certificate Timestamps (SCTs) designate Merkle tree leaves that can be checked for inclusion with a CT Log's Signed Tree Head (STH). However, inclusion should only be checked against an STH that is newer than the SCTs timestamp by at least 24 hours - this is because Logs' have a Maximum Merge Delay of 24 hours, which is the time they have to produce a new STH that incorporates a given SCT. STHs are delivered periodically out of band. If there isn't a new enough STH, then SCTs will need to be marked as pending inclusion check, waiting for a new STH to be delivered. To determine how frequently an STH should be delivered (currently, daily) and how big the SCT queue list should be (that is, how frequently clients encounter a brand new SCT that is newer than any STH), measure how often an SCT can't be checked for inclusion immediately after it's been seen. BUG=613495 Committed: https://crrev.com/b03a8a77ead185c85a6c55303ac20176eaa20a56 Cr-Commit-Position: refs/heads/master@{#407471} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b03a8a77ead185c85a6c55303ac20176eaa20a56 Cr-Commit-Position: refs/heads/master@{#407471} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
