Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2057153002:
Add GetUserMedia ETLD+1 rappor metrics. (Closed)
|
Created:
4 years, 6 months ago by Guido Urdaneta Modified:
4 years, 6 months ago CC:
chromium-reviews, blink-reviews, haraken, tommyw+watchlist_chromium.org, asvitkine+watch_chromium.org, mcasas+watch+mediastream_chromium.org, Henrik Grunell Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd GetUserMedia ETLD+1 rappor metrics.
BUG=618990
Committed: https://crrev.com/a71b7b4bdf469eca1f8ebb0925af0142a48530aa
Cr-Commit-Position: refs/heads/master@{#400329}
Patch Set 1 #Patch Set 2 : fix email #
Total comments: 2
Patch Set 3 : Change approach to use per-document instead of per-call stats, update metric names #
Total comments: 10
Patch Set 4 : jww comments #
Total comments: 2
Patch Set 5 : Deprecate old metrics and rebase #Patch Set 6 : Stop reporting host-based GetUserMedia rappor #
Messages
Total messages: 35 (13 generated)
guidou@chromium.org changed reviewers: + tommi@chromium.org
Hi, PTAL https://codereview.chromium.org/2057153002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp (right): https://codereview.chromium.org/2057153002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp:137: Platform::current()->recordRapporURL("GetUserMedia.SecureOrigin", WebURL(document->url())); Is "GetUserMedia.SecureOrigin" a good name for this metric? The name of the metric currently reported by HostsUsingFeatures is "PowerfulFeatureUse.Host.GetUserMedia.Secure".
Description was changed from ========== Add GetUserMedia rapport metrics. BUG=618990 ========== to ========== Add GetUserMedia rapport metrics. BUG=618990 ==========
guidou@chromium.org changed reviewers: + isherman@chromium.org
+isherman for rappor.xml
lgtm https://codereview.chromium.org/2057153002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp (right): https://codereview.chromium.org/2057153002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/UserMediaRequest.cpp:137: Platform::current()->recordRapporURL("GetUserMedia.SecureOrigin", WebURL(document->url())); On 2016/06/10 12:47:49, Guido Urdaneta wrote: > Is "GetUserMedia.SecureOrigin" a good name for this metric? > The name of the metric currently reported by HostsUsingFeatures is > "PowerfulFeatureUse.Host.GetUserMedia.Secure". Sounds reasonable to me and consistent with how things are currently being reported - but I don't have anything else to compare against really.
How are these metrics different from the existing PowerfulFeatureUse.Host.GetUserMedia.* metrics?
On 2016/06/10 21:13:25, Ilya Sherman wrote: > How are these metrics different from the existing > PowerfulFeatureUse.Host.GetUserMedia.* metrics? If they are simply providing more data about the URL, then please (a) name them in parallel to the existing metrics, and (b) probably deprecate the existing metrics.
On 2016/06/10 21:16:07, Ilya Sherman wrote: > On 2016/06/10 21:13:25, Ilya Sherman wrote: > > How are these metrics different from the existing > > PowerfulFeatureUse.Host.GetUserMedia.* metrics? > > If they are simply providing more data about the URL, then please (a) name them > in parallel to the existing metrics, and (b) probably deprecate the existing > metrics. PS3 changes the naming of the metrics to be more consistent with the existing ones.
guidou@chromium.org changed reviewers: + japhet@chromium.org, jww@chromium.org
+japhet and jww: Please review changes to WebKit/Source/core/frame
Thanks for writing this up! Generally makes sense, but as per my comments below, I think some of the old host recording logic may be a little messed up. https://codereview.chromium.org/2057153002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/HostsUsingFeatures.cpp (right): https://codereview.chromium.org/2057153002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/HostsUsingFeatures.cpp:93: m_urlAndValues.append(std::make_pair(url.getString(), counter)); nit: Per my comment in HostsUsingFeatures.h, I'd prefer that you store the KURL in the pair rather than the serialized URL since the value is just turned back into a KURL later. https://codereview.chromium.org/2057153002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/HostsUsingFeatures.cpp:111: HashMap<String, HostsUsingFeatures::Value> aggregatedByHost; Things are a bit confused here, as represented by the variable name |aggregatedByHost|. What you've written up here is, in fact, aggregating by URL, not Host, but you left the variable name the same. However, I think that you actually need both. That is, you need one aggregation by Host for all of the old metrics, and then you need to aggregate by URL for your one new metric. So maybe you still need recordHostToRappor() as a method in addition to a new recordUrlToRappor()? You'll also need to be careful in creating the new version of recordHostToRappor since you'll have multiple URLs with potentially the same host, so you can't just blindly add aggregatedByHost.add(urlAndValue.first.host()); you'll need to check to see if the host is already in the HashMap and potentially add to it. https://codereview.chromium.org/2057153002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/HostsUsingFeatures.cpp:141: void HostsUsingFeatures::Value::recordURLToRappor(const KURL& url) See my comments above in recordURLToRappor(), but I think you want this to remain recordHostToRappor() and to add a new method recordURLRappor that only counts up your URLs. https://codereview.chromium.org/2057153002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/HostsUsingFeatures.cpp:168: // Platform::recordRapporURL uses the ETLD+1 of the given URL nit: Would you mind updating the comment above Platform::recordRapporURL in third_party/WebKit/public/platform/Platform.h to mention this as well? That way when we forget all of this for the next metric we write up, it's a little more discoverable ;-) https://codereview.chromium.org/2057153002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/HostsUsingFeatures.h (right): https://codereview.chromium.org/2057153002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/HostsUsingFeatures.h:81: Vector<std::pair<String, HostsUsingFeatures::Value>, 1> m_urlAndValues; nit: Why store the serialized URL and not just the KURL, especially since you just rebuild the KURL later?
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/2057153002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/HostsUsingFeatures.cpp (right): https://codereview.chromium.org/2057153002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/HostsUsingFeatures.cpp:93: m_urlAndValues.append(std::make_pair(url.getString(), counter)); On 2016/06/11 18:43:20, jww wrote: > nit: Per my comment in HostsUsingFeatures.h, I'd prefer that you store the KURL > in the pair rather than the serialized URL since the value is just turned back > into a KURL later. Done. https://codereview.chromium.org/2057153002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/HostsUsingFeatures.cpp:111: HashMap<String, HostsUsingFeatures::Value> aggregatedByHost; Fixed. Now there are separate functions to aggregate by URL and Host. https://codereview.chromium.org/2057153002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/HostsUsingFeatures.cpp:141: void HostsUsingFeatures::Value::recordURLToRappor(const KURL& url) On 2016/06/11 18:43:21, jww wrote: > See my comments above in recordURLToRappor(), but I think you want this to > remain recordHostToRappor() and to add a new method recordURLRappor that only > counts up your URLs. Done. https://codereview.chromium.org/2057153002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/HostsUsingFeatures.cpp:168: // Platform::recordRapporURL uses the ETLD+1 of the given URL On 2016/06/11 18:43:21, jww wrote: > nit: Would you mind updating the comment above Platform::recordRapporURL in > third_party/WebKit/public/platform/Platform.h to mention this as well? That way > when we forget all of this for the next metric we write up, it's a little more > discoverable ;-) Done. https://codereview.chromium.org/2057153002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/HostsUsingFeatures.h (right): https://codereview.chromium.org/2057153002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/HostsUsingFeatures.h:81: Vector<std::pair<String, HostsUsingFeatures::Value>, 1> m_urlAndValues; On 2016/06/11 18:43:21, jww wrote: > nit: Why store the serialized URL and not just the KURL, especially since you > just rebuild the KURL later? Done.
Description was changed from ========== Add GetUserMedia rapport metrics. BUG=618990 ========== to ========== Add GetUserMedia ETLD+1 rappor metrics. BUG=618990 ==========
lgtm
https://codereview.chromium.org/2057153002/diff/80001/tools/metrics/rappor/ra... File tools/metrics/rappor/rappor.xml (right): https://codereview.chromium.org/2057153002/diff/80001/tools/metrics/rappor/ra... tools/metrics/rappor/rappor.xml:1452: </rappor-metric> Can the Host.GetUserMedia metrics be deprecated, since they are being replaced by these new metrics?
On 2016/06/13 23:30:45, Ilya Sherman wrote: > https://codereview.chromium.org/2057153002/diff/80001/tools/metrics/rappor/ra... > File tools/metrics/rappor/rappor.xml (right): > > https://codereview.chromium.org/2057153002/diff/80001/tools/metrics/rappor/ra... > tools/metrics/rappor/rappor.xml:1452: </rappor-metric> > Can the Host.GetUserMedia metrics be deprecated, since they are being replaced > by these new metrics? We'd like to not remove them at once. They measure different things. But deprecating them (I take it this means "don't remove them now, but promise to delete them in the near future unless things change") seems like a reasonable thing to do.
guidou@chromium.org changed reviewers: + foolip@chromium.org - japhet@chromium.org
https://codereview.chromium.org/2057153002/diff/80001/tools/metrics/rappor/ra... File tools/metrics/rappor/rappor.xml (right): https://codereview.chromium.org/2057153002/diff/80001/tools/metrics/rappor/ra... tools/metrics/rappor/rappor.xml:1452: </rappor-metric> On 2016/06/13 23:30:45, Ilya Sherman wrote: > Can the Host.GetUserMedia metrics be deprecated, since they are being replaced > by these new metrics? Done.
guidou@chromium.org changed reviewers: + japhet@chromium.org - foolip@chromium.org
isherman@chromium.org changed reviewers: + holte@chromium.org
Thanks. I think this more or less LG, but I'm a little unsure about RAPPOR's privacy guarantees when logging overlapping data to multiple metrics. So, adding Steve for his thoughts on that.
On 2016/06/14 21:58:12, Ilya Sherman wrote: > Thanks. I think this more or less LG, but I'm a little unsure about RAPPOR's > privacy guarantees when logging overlapping data to multiple metrics. So, > adding Steve for his thoughts on that. Reporting host and ETLD+1 as separate metrics does degrade the privacy guarantees, since it gives an extra sample with different noise. That's not necessarily a complete blocker, but definitely would be preferable to avoid.
On 2016/06/14 22:42:40, Steven Holte wrote: > On 2016/06/14 21:58:12, Ilya Sherman wrote: > > Thanks. I think this more or less LG, but I'm a little unsure about RAPPOR's > > privacy guarantees when logging overlapping data to multiple metrics. So, > > adding Steve for his thoughts on that. > > Reporting host and ETLD+1 as separate metrics does degrade the privacy > guarantees, since it gives an extra sample with different noise. That's not > necessarily a complete blocker, but definitely would be preferable to avoid. In PS6 we stop reporting the host metrics to avoid degrading the privacy guarantees. PTAL.
LGTM, thanks.
lgtm
lgtm
The CQ bit was checked by guidou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org, jww@chromium.org Link to the patchset: https://codereview.chromium.org/2057153002/#ps120001 (title: "Stop reporting host-based GetUserMedia rappor")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2057153002/120001
Message was sent while issue was closed.
Description was changed from ========== Add GetUserMedia ETLD+1 rappor metrics. BUG=618990 ========== to ========== Add GetUserMedia ETLD+1 rappor metrics. BUG=618990 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add GetUserMedia ETLD+1 rappor metrics. BUG=618990 ========== to ========== Add GetUserMedia ETLD+1 rappor metrics. BUG=618990 Committed: https://crrev.com/a71b7b4bdf469eca1f8ebb0925af0142a48530aa Cr-Commit-Position: refs/heads/master@{#400329} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a71b7b4bdf469eca1f8ebb0925af0142a48530aa Cr-Commit-Position: refs/heads/master@{#400329} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
