Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2095643002:
Add RTCPeerConnection RAPPOR metrics. (Closed)
|
Created:
4 years, 6 months ago by Guido Urdaneta Modified:
4 years, 5 months ago CC:
chromium-reviews, blink-reviews, haraken, tommyw+watchlist_chromium.org, asvitkine+watch_chromium.org, mcasas+watch+mediastream_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd RTCPeerConnection RAPPOR metrics.
BUG=610285
Committed: https://crrev.com/f2dfafd9049222c8058fe0fb3a71d6b2822367b8
Cr-Commit-Position: refs/heads/master@{#403305}
Patch Set 1 #
Total comments: 1
Patch Set 2 : hta@ comments #Patch Set 3 : Change to audio, video and datachannel #Patch Set 4 : record metrics only for connected PCs #
Messages
Total messages: 31 (10 generated)
Description was changed from ========== Add RTCPeerConnection RAPPOR metrics. BUG=610285 ========== to ========== Add RTCPeerConnection RAPPOR metrics. BUG=610285 ==========
guidou@chromium.org changed reviewers: + hta@chromium.org, tommi@chromium.org
Hi, PTAL
1) Can you write tests? See https://codereview.chromium.org/2045193003/ - a layouttest can call window.internals.isUseCounted(document, counter-ID) before and after the event that's supposed to cause a count. 2) Not sure about the naming. The feature numbers are: - Created - Stable - Connected Created and Connected are obvious, but should we call the middle one NegotiationComplete? Otherwise, LGTM!
On 2016/06/23 15:50:40, hta - Chromium wrote: > 1) Can you write tests? > See https://codereview.chromium.org/2045193003/ - a layouttest can call > window.internals.isUseCounted(document, counter-ID) before and after the event > that's supposed to cause a count. isUseCounted() works with UseCounter, but these are RAPPOR metrics. Didn't find an equivalent for RAPPOR. > 2) Not sure about the naming. The feature numbers are: > - Created > - Stable > - Connected > > Created and Connected are obvious, but should we call the middle one > NegotiationComplete? Done.
guidou@chromium.org changed reviewers: + holte@chromium.org, japhet@chromium.org
japhet@chromium.org: Please review changes in WebKit/Source/core/frame tommi@chromium.org: Please review changes in WebKit/Source/modules/mediastream holte@chromium.org: Please review rappor.xml
lgtm!!! great to have this. Can we add separate stats for audio, video and data? (not needed in this cl unless it's easier that way)
https://codereview.chromium.org/2095643002/diff/1/tools/metrics/rappor/rappor... File tools/metrics/rappor/rappor.xml (right): https://codereview.chromium.org/2095643002/diff/1/tools/metrics/rappor/rappor... tools/metrics/rappor/rappor.xml:1665: <rappor-metric name="RTCPeerConnection.Stable" type="ETLD_PLUS_ONE"> It looks like these metrics will essentially be reporting the same value 3 times, which weakens the privacy value effect of the non-deterministic portion of the noise. It would be better if you rearranged the metrics so that they report in a non-overlapping way, e.g. (created but not used), (connected but not stable), (stable).
core/ LGTM
Patchset #3 (id:40001) has been deleted
On 2016/06/23 20:21:06, Steven Holte wrote: > https://codereview.chromium.org/2095643002/diff/1/tools/metrics/rappor/rappor... > File tools/metrics/rappor/rappor.xml (right): > > https://codereview.chromium.org/2095643002/diff/1/tools/metrics/rappor/rappor... > tools/metrics/rappor/rappor.xml:1665: <rappor-metric > name="RTCPeerConnection.Stable" type="ETLD_PLUS_ONE"> > It looks like these metrics will essentially be reporting the same value 3 > times, which weakens the privacy value effect of the non-deterministic portion > of the noise. It would be better if you rearranged the metrics so that they > report in a non-overlapping way, e.g. (created but not used), (connected but not > stable), (stable). What about storing audio, video and datachannel metrics as in PS3? In this case these metrics are independent in the sense that a host creating a peer connection with audio and video is indistinguishable from the same host creating two peer connections, one with audio and one with video.
On 2016/06/27 15:23:44, Guido Urdaneta wrote: > On 2016/06/23 20:21:06, Steven Holte wrote: > > > https://codereview.chromium.org/2095643002/diff/1/tools/metrics/rappor/rappor... > > File tools/metrics/rappor/rappor.xml (right): > > > > > https://codereview.chromium.org/2095643002/diff/1/tools/metrics/rappor/rappor... > > tools/metrics/rappor/rappor.xml:1665: <rappor-metric > > name="RTCPeerConnection.Stable" type="ETLD_PLUS_ONE"> > > It looks like these metrics will essentially be reporting the same value 3 > > times, which weakens the privacy value effect of the non-deterministic portion > > of the noise. It would be better if you rearranged the metrics so that they > > report in a non-overlapping way, e.g. (created but not used), (connected but > not > > stable), (stable). > > > What about storing audio, video and datachannel metrics as in PS3? > In this case these metrics are independent in the sense that a host creating a > peer connection with audio and video is indistinguishable from the same host > creating two peer connections, one with audio and one with video. Independent metrics are better than very directly correlated ones. What we would like to avoid is being able to make that assumption that reports for 2 different metrics are almost always going to have the same underlying value. lgtm
I'm afraid I have to protest here .... the metrics proposed now tell us something completely different from the original metrics proposed. The original metrics (created, negotiated, connected) were useful for answering questions like: - Are there people creating PeerConnections for other purposes than making connections (indicator: Large diff between "created" and "negotiated") - Of the ones that are created, how many complete the negotiation phase (vs being used for something else or failing at negotiation)? - Of the ones that negotiate, how many fail vs ow many succeed? Tracking AddTrack hints at what the originator intended to use the connection for (due to some oddities, you still have to do that for some non-connected usages, so it's not a pure signal). It lets you get information about what kind of media is used (audio, video and data), so it's useful counters, but not the questions outlied above. Another approach would be to count the highest connection state reached when the PC is closed, but I worry about that being an undercount if the app doesn't bother to close properly and just lets the renderer shutdown take care of it. Will RAPPOR data be consistently reported in that case? Sorry to slow this down with more thinking....
On 2016/06/28 10:02:47, hta - Chromium wrote: > I'm afraid I have to protest here .... the metrics proposed now tell us > something completely different from the original metrics proposed. > > The original metrics (created, negotiated, connected) were useful for answering > questions like: > > - Are there people creating PeerConnections for other purposes than making > connections (indicator: Large diff between "created" and "negotiated") > - Of the ones that are created, how many complete the negotiation phase (vs > being used for something else or failing at negotiation)? > - Of the ones that negotiate, how many fail vs ow many succeed? > > Tracking AddTrack hints at what the originator intended to use the connection > for (due to some oddities, you still have to do that for some non-connected > usages, so it's not a pure signal). It lets you get information about what kind > of media is used (audio, video and data), so it's useful counters, but not the > questions outlied above. > > Another approach would be to count the highest connection state reached when the > PC is closed, but I worry about that being an undercount if the app doesn't > bother to close properly and just lets the renderer shutdown take care of it. > Will RAPPOR data be consistently reported in that case? > > Sorry to slow this down with more thinking.... If there is a shutdown shortly after the metrics are recorded, those metrics probably won't get reported, since RAPPOR metrics are not written to disk.
On 2016/06/28 10:02:47, hta - Chromium wrote: > I'm afraid I have to protest here .... the metrics proposed now tell us > something completely different from the original metrics proposed. > > The original metrics (created, negotiated, connected) were useful for answering > questions like: > > - Are there people creating PeerConnections for other purposes than making > connections (indicator: Large diff between "created" and "negotiated") I think we don't need RAPPOR to get this data. If it's a yes/no question, we can use UMA. > - Of the ones that are created, how many complete the negotiation phase (vs > being used for something else or failing at negotiation)? > - Of the ones that negotiate, how many fail vs ow many succeed? I think we can use UMA to get answers to these as well. > Tracking AddTrack hints at what the originator intended to use the connection > for (due to some oddities, you still have to do that for some non-connected > usages, so it's not a pure signal). It lets you get information about what kind > of media is used (audio, video and data), so it's useful counters, but not the > questions outlied above. Agreed. We can find a more reliable place in the code to capture this. > Another approach would be to count the highest connection state reached when the > PC is closed, but I worry about that being an undercount if the app doesn't > bother to close properly and just lets the renderer shutdown take care of it. > Will RAPPOR data be consistently reported in that case? I think this has already been answered, but to be sure, we will lose data if we choose that route, but it's unclear how much. I think we should err on the side of getting data most of the time (e.g. closing a tab at the end of a conversation, would cause us to lose data). > Sorry to slow this down with more thinking....
On 2016/06/29 11:22:21, tommi-chrömium wrote: > On 2016/06/28 10:02:47, hta - Chromium wrote: > > I'm afraid I have to protest here .... the metrics proposed now tell us > > something completely different from the original metrics proposed. > > > > The original metrics (created, negotiated, connected) were useful for > answering > > questions like: > > > > - Are there people creating PeerConnections for other purposes than making > > connections (indicator: Large diff between "created" and "negotiated") > > I think we don't need RAPPOR to get this data. If it's a yes/no question, we > can use UMA. > > > - Of the ones that are created, how many complete the negotiation phase (vs > > being used for something else or failing at negotiation)? > > - Of the ones that negotiate, how many fail vs ow many succeed? > > I think we can use UMA to get answers to these as well. > > > Tracking AddTrack hints at what the originator intended to use the connection > > for (due to some oddities, you still have to do that for some non-connected > > usages, so it's not a pure signal). It lets you get information about what > kind > > of media is used (audio, video and data), so it's useful counters, but not the > > questions outlied above. > > Agreed. We can find a more reliable place in the code to capture this. > > > Another approach would be to count the highest connection state reached when > the > > PC is closed, but I worry about that being an undercount if the app doesn't > > bother to close properly and just lets the renderer shutdown take care of it. > > Will RAPPOR data be consistently reported in that case? > > I think this has already been answered, but to be sure, we will lose data if we > choose that route, but it's unclear how much. I think we should err on the side > of getting data most of the time (e.g. closing a tab at the end of a > conversation, would cause us to lose data). > > > Sorry to slow this down with more thinking.... PS4 changes the recording to occur after the IceConnectionState becomes connected. This way, we will count uses of audio, video or data only for connected PCs.
On 2016/06/30 13:24:31, Guido Urdaneta wrote: > On 2016/06/29 11:22:21, tommi-chrömium wrote: > > On 2016/06/28 10:02:47, hta - Chromium wrote: > > > I'm afraid I have to protest here .... the metrics proposed now tell us > > > something completely different from the original metrics proposed. > > > > > > The original metrics (created, negotiated, connected) were useful for > > answering > > > questions like: > > > > > > - Are there people creating PeerConnections for other purposes than making > > > connections (indicator: Large diff between "created" and "negotiated") > > > > I think we don't need RAPPOR to get this data. If it's a yes/no question, we > > can use UMA. > > > > > - Of the ones that are created, how many complete the negotiation phase (vs > > > being used for something else or failing at negotiation)? > > > - Of the ones that negotiate, how many fail vs ow many succeed? > > > > I think we can use UMA to get answers to these as well. > > > > > Tracking AddTrack hints at what the originator intended to use the > connection > > > for (due to some oddities, you still have to do that for some non-connected > > > usages, so it's not a pure signal). It lets you get information about what > > kind > > > of media is used (audio, video and data), so it's useful counters, but not > the > > > questions outlied above. > > > > Agreed. We can find a more reliable place in the code to capture this. > > > > > Another approach would be to count the highest connection state reached when > > the > > > PC is closed, but I worry about that being an undercount if the app doesn't > > > bother to close properly and just lets the renderer shutdown take care of > it. > > > Will RAPPOR data be consistently reported in that case? > > > > I think this has already been answered, but to be sure, we will lose data if > we > > choose that route, but it's unclear how much. I think we should err on the > side > > of getting data most of the time (e.g. closing a tab at the end of a > > conversation, would cause us to lose data). > > > > > Sorry to slow this down with more thinking.... > > PS4 changes the recording to occur after the IceConnectionState becomes > connected. > This way, we will count uses of audio, video or data only for connected PCs. tommi: can you take another look at PS4?
lgtm
The CQ bit was checked by guidou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org, hta@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2095643002/#ps80001 (title: "record metrics only for connected PCs")
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
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 tommi@chromium.org
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 ========== Add RTCPeerConnection RAPPOR metrics. BUG=610285 ========== to ========== Add RTCPeerConnection RAPPOR metrics. BUG=610285 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Add RTCPeerConnection RAPPOR metrics. BUG=610285 ========== to ========== Add RTCPeerConnection RAPPOR metrics. BUG=610285 Committed: https://crrev.com/f2dfafd9049222c8058fe0fb3a71d6b2822367b8 Cr-Commit-Position: refs/heads/master@{#403305} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f2dfafd9049222c8058fe0fb3a71d6b2822367b8 Cr-Commit-Position: refs/heads/master@{#403305} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
