Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1924743008:
media: Report RAPPOR for media pipeline errors (Closed)
|
Created:
4 years, 7 months ago by xhwang Modified:
4 years, 7 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Report RAPPOR for media pipeline errors
BUG=607949
TEST=Manually tested with a pipeline decode error.
Committed: https://crrev.com/418866bcc258edba7b25fb291141e74e52943f6b
Cr-Commit-Position: refs/heads/master@{#391101}
Patch Set 1 #
Total comments: 3
Patch Set 2 : rebase only #
Total comments: 1
Messages
Total messages: 22 (7 generated)
Description was changed from ========== media: Report RAPPOR for media pipeline errors. BUG= TEST= ========== to ========== media: Report RAPPOR for media pipeline errors BUG= TEST= ==========
xhwang@chromium.org changed reviewers: + dalecurtis@chromium.org, sandersd@chromium.org
Not compiled, not tested, needs rebase. But since this is pretty simple, I'd like to have you take a quick look to see whether the idea is good. If so I'll continue and test it out. https://chromiumcodereview.appspot.com/1924743008/diff/1/media/blink/webmedia... File media/blink/webmediaplayer_util.cc (right): https://chromiumcodereview.appspot.com/1924743008/diff/1/media/blink/webmedia... media/blink/webmediaplayer_util.cc:153: blink::WebStringToGURL(security_origin.toString())); I guess we are more interested in who's failing than how it failed. So I am not even using the |error| here. Or do we actually want to distinguish at least the major error cases? e.g. decode error, not supported, etc. https://chromiumcodereview.appspot.com/1924743008/diff/1/tools/metrics/rappor... File tools/metrics/rappor/rappor.xml (right): https://chromiumcodereview.appspot.com/1924743008/diff/1/tools/metrics/rappor... tools/metrics/rappor/rappor.xml:492: </rappor-metric> format fix only
https://chromiumcodereview.appspot.com/1924743008/diff/1/media/blink/webmedia... File media/blink/webmediaplayer_util.cc (right): https://chromiumcodereview.appspot.com/1924743008/diff/1/media/blink/webmedia... media/blink/webmediaplayer_util.cc:153: blink::WebStringToGURL(security_origin.toString())); On 2016/04/29 00:04:17, xhwang wrote: > I guess we are more interested in who's failing than how it failed. So I am not > even using the |error| here. Or do we actually want to distinguish at least the > major error cases? e.g. decode error, not supported, etc. Just FWIW, the top errors are PIPELINE_ERROR_ABORT PIPELINE_ERROR_DECODE PIPELINE_ERROR_NETWORK PIPELINE_ERROR_READ
Also, this doesn't exactly cover Media.GpuVideoDecoderInitializeStatus, which is deep in GpuVideoDecoder, and we have to do more plumbing to report RAPPOR for that one.
rebase only
Dale/Dan: I tested this CL and it's working. PTAL I am still thinking about what's the easiest way to report RAPPOR from GVD :)
Description was changed from ========== media: Report RAPPOR for media pipeline errors BUG= TEST= ========== to ========== media: Report RAPPOR for media pipeline errors BUG=607949 TEST=Manually tested with a pipeline decode error. ==========
Oooh, I like! lgtm
xhwang@chromium.org changed reviewers: + isherman@chromium.org
isherman: Please OWNERS review rappor.xml.
lgtm
+Steve, as the primary RAPPOR expert on the team https://chromiumcodereview.appspot.com/1924743008/diff/20001/media/blink/webm... File media/blink/webmediaplayer_util.cc (right): https://chromiumcodereview.appspot.com/1924743008/diff/20001/media/blink/webm... media/blink/webmediaplayer_util.cc:153: blink::WebStringToGURL(security_origin.toString())); Hmm, would it make sense to just have a single metric, and send the load type as an additional param for the metric? Steve recently added this functionality, and it seems like a good match for what you're trying to do here. (OTOH, I see that you already have other metrics broken down along similar axes, so maybe it makes more sense to remain parallel with those.)
On 2016/04/29 20:25:31, Ilya Sherman (Away May 5-15) wrote: > +Steve, as the primary RAPPOR expert on the team > > https://chromiumcodereview.appspot.com/1924743008/diff/20001/media/blink/webm... > File media/blink/webmediaplayer_util.cc (right): > > https://chromiumcodereview.appspot.com/1924743008/diff/20001/media/blink/webm... > media/blink/webmediaplayer_util.cc:153: > blink::WebStringToGURL(security_origin.toString())); > Hmm, would it make sense to just have a single metric, and send the load type as > an additional param for the metric? Steve recently added this functionality, > and it seems like a good match for what you're trying to do here. > > (OTOH, I see that you already have other metrics broken down along similar axes, > so maybe it makes more sense to remain parallel with those.) I don't think we've plumbed this API through to blink yet, so this lgtm as is.
On 2016/05/02 18:15:08, Steven Holte wrote: > On 2016/04/29 20:25:31, Ilya Sherman (Away May 5-15) wrote: > > +Steve, as the primary RAPPOR expert on the team > > > > > https://chromiumcodereview.appspot.com/1924743008/diff/20001/media/blink/webm... > > File media/blink/webmediaplayer_util.cc (right): > > > > > https://chromiumcodereview.appspot.com/1924743008/diff/20001/media/blink/webm... > > media/blink/webmediaplayer_util.cc:153: > > blink::WebStringToGURL(security_origin.toString())); > > Hmm, would it make sense to just have a single metric, and send the load type > as > > an additional param for the metric? Steve recently added this functionality, > > and it seems like a good match for what you're trying to do here. > > > > (OTOH, I see that you already have other metrics broken down along similar > axes, > > so maybe it makes more sense to remain parallel with those.) > > I don't think we've plumbed this API through to blink yet, so this lgtm as is. This code is actually in chromium (chromium's impl of blink media APIs). But since we already have existing metrics using the same naming pattern, I'd like to be consistent with those. I'll definitely consider using the new API for future metrics.
On 2016/05/02 18:19:24, xhwang wrote: > On 2016/05/02 18:15:08, Steven Holte wrote: > > On 2016/04/29 20:25:31, Ilya Sherman (Away May 5-15) wrote: > > > +Steve, as the primary RAPPOR expert on the team > > > > > > > > > https://chromiumcodereview.appspot.com/1924743008/diff/20001/media/blink/webm... > > > File media/blink/webmediaplayer_util.cc (right): > > > > > > > > > https://chromiumcodereview.appspot.com/1924743008/diff/20001/media/blink/webm... > > > media/blink/webmediaplayer_util.cc:153: > > > blink::WebStringToGURL(security_origin.toString())); > > > Hmm, would it make sense to just have a single metric, and send the load > type > > as > > > an additional param for the metric? Steve recently added this > functionality, > > > and it seems like a good match for what you're trying to do here. > > > > > > (OTOH, I see that you already have other metrics broken down along similar > > axes, > > > so maybe it makes more sense to remain parallel with those.) > > > > I don't think we've plumbed this API through to blink yet, so this lgtm as is. > > This code is actually in chromium (chromium's impl of blink media APIs). But > since we already have existing metrics using the same naming pattern, I'd like > to be consistent with those. I'll definitely consider using the new API for > future metrics. BTW, any examples to use the new API?
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1924743008/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1924743008/20001
Message was sent while issue was closed.
Description was changed from ========== media: Report RAPPOR for media pipeline errors BUG=607949 TEST=Manually tested with a pipeline decode error. ========== to ========== media: Report RAPPOR for media pipeline errors BUG=607949 TEST=Manually tested with a pipeline decode error. ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== media: Report RAPPOR for media pipeline errors BUG=607949 TEST=Manually tested with a pipeline decode error. ========== to ========== media: Report RAPPOR for media pipeline errors BUG=607949 TEST=Manually tested with a pipeline decode error. Committed: https://crrev.com/418866bcc258edba7b25fb291141e74e52943f6b Cr-Commit-Position: refs/heads/master@{#391101} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/418866bcc258edba7b25fb291141e74e52943f6b Cr-Commit-Position: refs/heads/master@{#391101} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
