Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13548005:
Add UMA reporting on failure to load ppapi plugins. (Closed)
|
Created:
7 years, 8 months ago by xhwang Modified:
7 years, 8 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd UMA reporting on failure to load ppapi plugins.
The UMA histogram name is appended with library names. Since we only have limited number of ppapi plugins this shouldn't be an issue for the server to handle.
BUG=226107
TEST=Tested Widevine CDM loading.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194494
Patch Set 1 #Patch Set 2 : rebase only #Patch Set 3 : fix gypi #
Total comments: 4
Patch Set 4 : merge include_dir #Patch Set 5 : Add todo. #Patch Set 6 : fix include dir #Patch Set 7 : CL refacted. #
Total comments: 14
Patch Set 8 : comments resolved #
Total comments: 2
Patch Set 9 : #Patch Set 10 : Also report loading error for flash. #
Total comments: 8
Patch Set 11 : use FactoryGet #Patch Set 12 : status -> result #
Total comments: 2
Patch Set 13 : remove empty line in DEPS #Patch Set 14 : Report from PpapiThread. #
Total comments: 4
Patch Set 15 : comment resolved #
Total comments: 2
Patch Set 16 : nit #Patch Set 17 : Updated histograms.xml #Patch Set 18 : ppapi -> PPAPI #
Total comments: 6
Patch Set 19 : rebase #
Messages
Total messages: 49 (0 generated)
PTAL
LGTM % comments https://codereview.chromium.org/13548005/diff/26001/content/content_ppapi_plu... File content/content_ppapi_plugin.gypi (right): https://codereview.chromium.org/13548005/diff/26001/content/content_ppapi_plu... content/content_ppapi_plugin.gypi:15: '<(INTERMEDIATE_DIR)', add after line 30 instead https://codereview.chromium.org/13548005/diff/26001/content/ppapi_plugin/ppap... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/13548005/diff/26001/content/ppapi_plugin/ppap... content/ppapi_plugin/ppapi_thread.cc:268: Should we add other Pepper plugins as well? There is a small well-defined set of them (unless NaCl nexe's also run through here).
+piman for OWNERS review. https://codereview.chromium.org/13548005/diff/26001/content/content_ppapi_plu... File content/content_ppapi_plugin.gypi (right): https://codereview.chromium.org/13548005/diff/26001/content/content_ppapi_plu... content/content_ppapi_plugin.gypi:15: '<(INTERMEDIATE_DIR)', On 2013/04/04 19:30:14, ddorwin wrote: > add after line 30 instead Done. https://codereview.chromium.org/13548005/diff/26001/content/ppapi_plugin/ppap... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/13548005/diff/26001/content/ppapi_plugin/ppap... content/ppapi_plugin/ppapi_thread.cc:268: On 2013/04/04 19:30:14, ddorwin wrote: > Should we add other Pepper plugins as well? There is a small well-defined set of > them (unless NaCl nexe's also run through here). I'd like to see if this works well before we add more plugins. Added TODO.
widevine shouldn't be a dependency of content - can we move that logic to chrome?
On 2013/04/04 21:16:58, piman wrote: > widevine shouldn't be a dependency of content - can we move that logic to > chrome? Thanks for the comment. It's a valid point that content doesn't depend on widevine. That being said, this is still a valuable metrics that we want to collect. As I mentioned in the TODO, we can add more plugin types to report when a plugin fails to load (e.g. flash, pdf, etc), which will make this more interesting. Since this is in the plugin process, it'll kind of be overkill to send the plugin name back to the browser and report a load failure there. As a workaround, instead of pulling the widevine_cdm_version.h header directly, I can define a list of plugins names that we want to collect metrics on. So we don't have explicitly dependency on widevine, but will have strings like "widevine", "shockwaveflash" defined in content code. Would that be acceptable? Or do you have any suggestions?
On 2013/04/04 22:40:22, xhwang wrote: > On 2013/04/04 21:16:58, piman wrote: > > widevine shouldn't be a dependency of content - can we move that logic to > > chrome? > > Thanks for the comment. It's a valid point that content doesn't depend on > widevine. > > That being said, this is still a valuable metrics that we want to collect. As I > mentioned in the TODO, we can add more plugin types to report when a plugin > fails to load (e.g. flash, pdf, etc), which will make this more interesting. > Since this is in the plugin process, it'll kind of be overkill to send the > plugin name back to the browser and report a load failure there. > > As a workaround, instead of pulling the widevine_cdm_version.h header directly, > I can define a list of plugins names that we want to collect metrics on. So we > don't have explicitly dependency on widevine, but will have strings like > "widevine", "shockwaveflash" defined in content code. Would that be acceptable? > > Or do you have any suggestions? For example you could feed that list from the chrome/ layer. Or alternatively have a way to get a notification from content/ to chrome/ that a plugin loaded (or not) Basically, think about it this way: should Chrome Embedded Framework or Opera know anything about how widevine?
On 2013/04/04 23:11:45, piman wrote: > On 2013/04/04 22:40:22, xhwang wrote: > > On 2013/04/04 21:16:58, piman wrote: > > > widevine shouldn't be a dependency of content - can we move that logic to > > > chrome? > > > > Thanks for the comment. It's a valid point that content doesn't depend on > > widevine. > > > > That being said, this is still a valuable metrics that we want to collect. As > I > > mentioned in the TODO, we can add more plugin types to report when a plugin > > fails to load (e.g. flash, pdf, etc), which will make this more interesting. > > Since this is in the plugin process, it'll kind of be overkill to send the > > plugin name back to the browser and report a load failure there. > > > > As a workaround, instead of pulling the widevine_cdm_version.h header > directly, > > I can define a list of plugins names that we want to collect metrics on. So we > > don't have explicitly dependency on widevine, but will have strings like > > "widevine", "shockwaveflash" defined in content code. Would that be > acceptable? > > > > Or do you have any suggestions? > > For example you could feed that list from the chrome/ layer. Or alternatively > have a way to get a notification from content/ to chrome/ that a plugin loaded > (or not) > > Basically, think about it this way: should Chrome Embedded Framework or Opera > know anything about how widevine? That's legitimate argument. I'll think about it and update this CL. Thanks!
I have refactored this CL. Now we use chrome_content_plugin_client.cc to report UMA instead of in ppapi_thread.cc directly. PTAL.
LG. Thanks. https://codereview.chromium.org/13548005/diff/35001/content/ppapi_plugin/ppap... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/13548005/diff/35001/content/ppapi_plugin/ppap... content/ppapi_plugin/ppapi_thread.cc:266: // Get the GetInterface function (required). Unrelated comment: It's strange that we get this for the broker. It appears to be used in checks that the broker may need to pass, though. https://codereview.chromium.org/13548005/diff/35001/content/ppapi_plugin/ppap... content/ppapi_plugin/ppapi_thread.cc:286: plugin_entry_points_.initialize_module = Unrelated comment: It's weird that we save the initialize for the plugin but not the broker. I guess this is because Remoting Viewer sets this value outside this function. https://codereview.chromium.org/13548005/diff/35001/content/ppapi_plugin/ppap... content/ppapi_plugin/ppapi_thread.cc:328: LOG(WARNING) << "No PPP_InitializeBroker in plugin library"; This is another ENTRY_POINT_MISSING. Do we want to track broker errors too? Same with the init errors below. Since we're making the enum, we could add separate BROKER values. https://codereview.chromium.org/13548005/diff/35001/content/ppapi_plugin/ppap... File content/ppapi_plugin/ppapi_thread.h (right): https://codereview.chromium.org/13548005/diff/35001/content/ppapi_plugin/ppap... content/ppapi_plugin/ppapi_thread.h:117: void ReportPluginLoadStatus(const base::FilePath& path, Status or Result? https://codereview.chromium.org/13548005/diff/35001/content/public/plugin/con... File content/public/plugin/content_plugin_client.h (right): https://codereview.chromium.org/13548005/diff/35001/content/public/plugin/con... content/public/plugin/content_plugin_client.h:17: enum PluginLoadStatus { Status or Result? https://codereview.chromium.org/13548005/diff/35001/content/public/plugin/con... content/public/plugin/content_plugin_client.h:35: // Notifies that a plugin has been loaded. Could also be that a plugin load failed. I'm not sure how to word that, though.
+jam for content/public changes. https://codereview.chromium.org/13548005/diff/35001/content/public/plugin/con... File content/public/plugin/content_plugin_client.h (right): https://codereview.chromium.org/13548005/diff/35001/content/public/plugin/con... content/public/plugin/content_plugin_client.h:19: UNKNOWN_ERROR, You don't use this. Remove? https://codereview.chromium.org/13548005/diff/35001/content/public/plugin/con... content/public/plugin/content_plugin_client.h:23: LOAD_STATUS_MAX nit: this last value I was very confused about until I looked at the use of it. Could you add a comment?
PTAL again https://codereview.chromium.org/13548005/diff/35001/content/ppapi_plugin/ppap... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/13548005/diff/35001/content/ppapi_plugin/ppap... content/ppapi_plugin/ppapi_thread.cc:328: LOG(WARNING) << "No PPP_InitializeBroker in plugin library"; On 2013/04/08 18:04:34, ddorwin wrote: > This is another ENTRY_POINT_MISSING. Do we want to track broker errors too? Same > with the init errors below. Since we're making the enum, we could add separate > BROKER values. Added "is_broker" to report UMA for broker separately. https://codereview.chromium.org/13548005/diff/35001/content/ppapi_plugin/ppap... File content/ppapi_plugin/ppapi_thread.h (right): https://codereview.chromium.org/13548005/diff/35001/content/ppapi_plugin/ppap... content/ppapi_plugin/ppapi_thread.h:117: void ReportPluginLoadStatus(const base::FilePath& path, On 2013/04/08 18:04:34, ddorwin wrote: > Status or Result? Done. https://codereview.chromium.org/13548005/diff/35001/content/public/plugin/con... File content/public/plugin/content_plugin_client.h (right): https://codereview.chromium.org/13548005/diff/35001/content/public/plugin/con... content/public/plugin/content_plugin_client.h:17: enum PluginLoadStatus { On 2013/04/08 18:04:34, ddorwin wrote: > Status or Result? Done. https://codereview.chromium.org/13548005/diff/35001/content/public/plugin/con... content/public/plugin/content_plugin_client.h:19: UNKNOWN_ERROR, On 2013/04/08 19:38:43, piman wrote: > You don't use this. Remove? Done. https://codereview.chromium.org/13548005/diff/35001/content/public/plugin/con... content/public/plugin/content_plugin_client.h:23: LOAD_STATUS_MAX On 2013/04/08 19:38:43, piman wrote: > nit: this last value I was very confused about until I looked at the use of it. > Could you add a comment? Done. https://codereview.chromium.org/13548005/diff/35001/content/public/plugin/con... content/public/plugin/content_plugin_client.h:35: // Notifies that a plugin has been loaded. On 2013/04/08 18:04:34, ddorwin wrote: > Could also be that a plugin load failed. I'm not sure how to word that, though. Done.
LGTM modulo comments that John might have re: content/public.
https://codereview.chromium.org/13548005/diff/51001/content/ppapi_plugin/ppap... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/13548005/diff/51001/content/ppapi_plugin/ppap... content/ppapi_plugin/ppapi_thread.cc:341: ReportLoadResult(path, ContentPluginClient::ENTRY_POINT_MISSING); Is this a missing entry point or a failure (INIT_FAILED)?
https://codereview.chromium.org/13548005/diff/51001/content/ppapi_plugin/ppap... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/13548005/diff/51001/content/ppapi_plugin/ppap... content/ppapi_plugin/ppapi_thread.cc:341: ReportLoadResult(path, ContentPluginClient::ENTRY_POINT_MISSING); On 2013/04/09 00:50:40, ddorwin wrote: > Is this a missing entry point or a failure (INIT_FAILED)? Done.
Added Flash check in addition to Widevine. +isherman for confirming the use of UMA_HISTOGRAM_* in macro. PTAL, thanks!
Cool. https://codereview.chromium.org/13548005/diff/82001/chrome/plugin/chrome_cont... File chrome/plugin/chrome_content_plugin_client.cc (right): https://codereview.chromium.org/13548005/diff/82001/chrome/plugin/chrome_cont... chrome/plugin/chrome_content_plugin_client.cc:75: // |name| must be const string based on how UMA_HISTOGRAM_ENUMERATION works. You should also explain why this must be a macro for UMA to work. (You don't want someone cleaning up your macro into a function. :) ) https://codereview.chromium.org/13548005/diff/82001/chrome/plugin/chrome_cont... chrome/plugin/chrome_content_plugin_client.cc:92: REPORT_PLUGIN_LOAD_UMA("PepperFlash"); pass result into your macro as if it was a function.
LGTM for UMA histogram usage, with a suggestion: https://codereview.chromium.org/13548005/diff/82001/chrome/plugin/chrome_cont... File chrome/plugin/chrome_content_plugin_client.cc (right): https://codereview.chromium.org/13548005/diff/82001/chrome/plugin/chrome_cont... chrome/plugin/chrome_content_plugin_client.cc:78: if (!is_broker) { \ For readability's sake, please pass is_broker in as an argument. https://codereview.chromium.org/13548005/diff/82001/chrome/plugin/chrome_cont... chrome/plugin/chrome_content_plugin_client.cc:85: } while (0); This is ok, but IMO it'd be cleaner to write a function like this one: https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...
PTAL again. https://codereview.chromium.org/13548005/diff/82001/chrome/plugin/chrome_cont... File chrome/plugin/chrome_content_plugin_client.cc (right): https://codereview.chromium.org/13548005/diff/82001/chrome/plugin/chrome_cont... chrome/plugin/chrome_content_plugin_client.cc:75: // |name| must be const string based on how UMA_HISTOGRAM_ENUMERATION works. On 2013/04/09 01:44:11, ddorwin wrote: > You should also explain why this must be a macro for UMA to work. (You don't > want someone cleaning up your macro into a function. :) ) Use FactoryGet following Ilya's suggestion as we don't need these comments. https://codereview.chromium.org/13548005/diff/82001/chrome/plugin/chrome_cont... chrome/plugin/chrome_content_plugin_client.cc:78: if (!is_broker) { \ On 2013/04/09 01:49:22, Ilya Sherman wrote: > For readability's sake, please pass is_broker in as an argument. Done. https://codereview.chromium.org/13548005/diff/82001/chrome/plugin/chrome_cont... chrome/plugin/chrome_content_plugin_client.cc:85: } while (0); On 2013/04/09 01:49:22, Ilya Sherman wrote: > This is ok, but IMO it'd be cleaner to write a function like this one: > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... Thanks! This is much better! https://codereview.chromium.org/13548005/diff/82001/chrome/plugin/chrome_cont... chrome/plugin/chrome_content_plugin_client.cc:92: REPORT_PLUGIN_LOAD_UMA("PepperFlash"); On 2013/04/09 01:44:11, ddorwin wrote: > pass result into your macro as if it was a function. Done.
lgtm
jam@: could you please do an OWNERS review on content/public/* ?
it's unclear to me why you're changing chrome? i.e. why not just send these uma stats from the content code, and do it for all pepper plugins (so you wouldn't need the constants for plugin names from chrome)? https://codereview.chromium.org/13548005/diff/95001/chrome/plugin/DEPS File chrome/plugin/DEPS (right): https://codereview.chromium.org/13548005/diff/95001/chrome/plugin/DEPS#newcode5 chrome/plugin/DEPS:5: why the blank line?
> it's unclear to me why you're changing chrome? i.e. why not just send these uma > stats from the content code, and do it for all pepper plugins (so you wouldn't > need the constants for plugin names from chrome)? Plugins are built separately and we want to have the information about which plugin(s) failed to load for which reason. For example, knowing that 2% of all plugins failed to load on Windows is pretty much useless. We still don't know which one failed. By differentiating failure cases with plugin names, we'll know which one is failing and this makes investigating much easier. However, plugin name parsing cannot be done in content code, that why I change chrome to do that. https://codereview.chromium.org/13548005/diff/95001/chrome/plugin/DEPS File chrome/plugin/DEPS (right): https://codereview.chromium.org/13548005/diff/95001/chrome/plugin/DEPS#newcode5 chrome/plugin/DEPS:5: On 2013/04/09 16:17:01, jam wrote: > why the blank line? Done.
On 2013/04/09 16:28:05, xhwang wrote: > > it's unclear to me why you're changing chrome? i.e. why not just send these > uma > > stats from the content code, and do it for all pepper plugins (so you wouldn't > > need the constants for plugin names from chrome)? > > Plugins are built separately and we want to have the information about which > plugin(s) failed to load for which reason. For example, knowing that 2% of all > plugins failed to load on Windows is pretty much useless. We still don't know > which one failed. By differentiating failure cases with plugin names, we'll know > which one is failing and this makes investigating much easier. However, plugin > name parsing cannot be done in content code, that why I change chrome to do > that. I'm not suggesting sending just percentage stats. i'm suggesting that content send the name of the plugin (say the base part of the filename).
On 2013/04/09 17:02:11, jam wrote: > On 2013/04/09 16:28:05, xhwang wrote: > > > it's unclear to me why you're changing chrome? i.e. why not just send these > > uma > > > stats from the content code, and do it for all pepper plugins (so you > wouldn't > > > need the constants for plugin names from chrome)? > > > > Plugins are built separately and we want to have the information about which > > plugin(s) failed to load for which reason. For example, knowing that 2% of all > > plugins failed to load on Windows is pretty much useless. We still don't know > > which one failed. By differentiating failure cases with plugin names, we'll > know > > which one is failing and this makes investigating much easier. However, plugin > > name parsing cannot be done in content code, that why I change chrome to do > > that. > > I'm not suggesting sending just percentage stats. i'm suggesting that content > send the name of the plugin (say the base part of the filename). One problem with this approach is that the plugin names on different platforms are different (e.g. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/chro...). So we'll have different histograms on different platforms, which may end up with too many small histograms. Also, if we ever change the plugin name, we'll end up with a new histogram. The current CL doesn't have these two problems.
On 2013/04/09 17:14:12, xhwang wrote: > On 2013/04/09 17:02:11, jam wrote: > > On 2013/04/09 16:28:05, xhwang wrote: > > > > it's unclear to me why you're changing chrome? i.e. why not just send > these > > > uma > > > > stats from the content code, and do it for all pepper plugins (so you > > wouldn't > > > > need the constants for plugin names from chrome)? > > > > > > Plugins are built separately and we want to have the information about which > > > plugin(s) failed to load for which reason. For example, knowing that 2% of > all > > > plugins failed to load on Windows is pretty much useless. We still don't > know > > > which one failed. By differentiating failure cases with plugin names, we'll > > know > > > which one is failing and this makes investigating much easier. However, > plugin > > > name parsing cannot be done in content code, that why I change chrome to do > > > that. > > > > I'm not suggesting sending just percentage stats. i'm suggesting that content > > send the name of the plugin (say the base part of the filename). > > One problem with this approach is that the plugin names on different platforms > are different (e.g. > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/chro...). > So we'll have different histograms on different platforms, which may end up with > too many small histograms. > > Also, if we ever change the plugin name, we'll end up with a new histogram. > > The current CL doesn't have these two problems. These seem like minor issues. This is just a histogram for debugging, and it's likely that one would want to break out these stats per platform anyways. Changing the plugin name seems like a rare thing, and even if it happens, getting a new histogram seems like a minor cost.
On 2013/04/09 17:22:05, jam wrote: > On 2013/04/09 17:14:12, xhwang wrote: > > On 2013/04/09 17:02:11, jam wrote: > > > On 2013/04/09 16:28:05, xhwang wrote: > > > > > it's unclear to me why you're changing chrome? i.e. why not just send > > these > > > > uma > > > > > stats from the content code, and do it for all pepper plugins (so you > > > wouldn't > > > > > need the constants for plugin names from chrome)? > > > > > > > > Plugins are built separately and we want to have the information about > which > > > > plugin(s) failed to load for which reason. For example, knowing that 2% of > > all > > > > plugins failed to load on Windows is pretty much useless. We still don't > > know > > > > which one failed. By differentiating failure cases with plugin names, > we'll > > > know > > > > which one is failing and this makes investigating much easier. However, > > plugin > > > > name parsing cannot be done in content code, that why I change chrome to > do > > > > that. > > > > > > I'm not suggesting sending just percentage stats. i'm suggesting that > content > > > send the name of the plugin (say the base part of the filename). > > > > One problem with this approach is that the plugin names on different platforms > > are different (e.g. > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/chro...). > > So we'll have different histograms on different platforms, which may end up > with > > too many small histograms. BTW, we'll also have a lot more entries in histograms.xml (see https://chromereviews.googleplex.com/7570013/). For example, we'll have Plugin.PluginLoadResult.PepperFlashPlayer.plugin Plugin.PluginLoadResult.pepflashplayer.dll Plugin.PluginLoadResult.libpepflashplayer.so Plugin.PluginLoadResult.widevinecdmadapter.plugin Plugin.PluginLoadResult.widevinecdmadapter.dll Plugin.PluginLoadResult.libwidevinecdmadapter.so Plugin.BrokerLoadResult.PepperFlashPlayer.plugin Plugin.BrokerLoadResult.pepflashplayer.dll Plugin.BrokerLoadResult.libpepflashplayer.so Plugin.BrokerLoadResult.widevinecdmadapter.plugin Plugin.BrokerLoadResult.widevinecdmadapter.dll Plugin.BrokerLoadResult.libwidevinecdmadapter.so (some combination won't appear in real life though) instead of: Plugin.BrokerLoadResult.PepperFlash Plugin.BrokerLoadResult.WidevineCDM Plugin.PluginLoadResult.PepperFlash Plugin.PluginLoadResult.WidevineCDM +isherman,+jar for comments. > > > > Also, if we ever change the plugin name, we'll end up with a new histogram. > > > > The current CL doesn't have these two problems. > > These seem like minor issues. This is just a histogram for debugging, and it's > likely that one would want to break out these stats per platform anyways. > Changing the plugin name seems like a rare thing, and even if it happens, > getting a new histogram seems like a minor cost. I agree these aren't big issues. I just wanted to see the trade-off between your suggestion and the current CL. To me, I don't see why changing chrome code is such a big issue. (Probably because I rarely touch chrome code :) But I definitely trust your expertise here.) I just feel it's convenient to have less histograms for the same amount of information. Changing plugin names IS a rare thing. But to do things correctly, we might need to add a comment where the plugin is defined that if the name is changed one should also update histograms.xml.
On 2013/04/09 17:22:05, jam wrote: > On 2013/04/09 17:14:12, xhwang wrote: > > On 2013/04/09 17:02:11, jam wrote: > > > On 2013/04/09 16:28:05, xhwang wrote: > > > > > it's unclear to me why you're changing chrome? i.e. why not just send > > these > > > > uma > > > > > stats from the content code, and do it for all pepper plugins (so you > > > wouldn't > > > > > need the constants for plugin names from chrome)? > > > > > > > > Plugins are built separately and we want to have the information about > which > > > > plugin(s) failed to load for which reason. For example, knowing that 2% of > > all > > > > plugins failed to load on Windows is pretty much useless. We still don't > > know > > > > which one failed. By differentiating failure cases with plugin names, > we'll > > > know > > > > which one is failing and this makes investigating much easier. However, > > plugin > > > > name parsing cannot be done in content code, that why I change chrome to > do > > > > that. > > > > > > I'm not suggesting sending just percentage stats. i'm suggesting that > content > > > send the name of the plugin (say the base part of the filename). > > > > One problem with this approach is that the plugin names on different platforms > > are different (e.g. > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/chro...). > > So we'll have different histograms on different platforms, which may end up > with > > too many small histograms. > > > > Also, if we ever change the plugin name, we'll end up with a new histogram. > > > > The current CL doesn't have these two problems. > > These seem like minor issues. This is just a histogram for debugging, and it's > likely that one would want to break out these stats per platform anyways. > Changing the plugin name seems like a rare thing, and even if it happens, > getting a new histogram seems like a minor cost. This is not a minor issue. We should never write a code path that can lead to an unbounded number of histograms being created. Each new histogram imposes a performance cost, both in Chrome and in the eventual processing of the data.
On 2013/04/09 20:52:08, Ilya Sherman wrote: > On 2013/04/09 17:22:05, jam wrote: > > On 2013/04/09 17:14:12, xhwang wrote: > > > On 2013/04/09 17:02:11, jam wrote: > > > > On 2013/04/09 16:28:05, xhwang wrote: > > > > > > it's unclear to me why you're changing chrome? i.e. why not just send > > > these > > > > > uma > > > > > > stats from the content code, and do it for all pepper plugins (so you > > > > wouldn't > > > > > > need the constants for plugin names from chrome)? > > > > > > > > > > Plugins are built separately and we want to have the information about > > which > > > > > plugin(s) failed to load for which reason. For example, knowing that 2% > of > > > all > > > > > plugins failed to load on Windows is pretty much useless. We still don't > > > know > > > > > which one failed. By differentiating failure cases with plugin names, > > we'll > > > > know > > > > > which one is failing and this makes investigating much easier. However, > > > plugin > > > > > name parsing cannot be done in content code, that why I change chrome to > > do > > > > > that. > > > > > > > > I'm not suggesting sending just percentage stats. i'm suggesting that > > content > > > > send the name of the plugin (say the base part of the filename). > > > > > > One problem with this approach is that the plugin names on different > platforms > > > are different (e.g. > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/chro...). > > > So we'll have different histograms on different platforms, which may end up > > with > > > too many small histograms. > > > > > > Also, if we ever change the plugin name, we'll end up with a new histogram. > > > > > > The current CL doesn't have these two problems. > > > > These seem like minor issues. This is just a histogram for debugging, and it's > > likely that one would want to break out these stats per platform anyways. > > Changing the plugin name seems like a rare thing, and even if it happens, > > getting a new histogram seems like a minor cost. > > This is not a minor issue. We should never write a code path that can lead to > an unbounded number of histograms being created. Each new histogram imposes a > performance cost, both in Chrome and in the eventual processing of the data. No one mentioned unbounded. There's a small set of ppapi plugins.
On 2013/04/09 20:56:25, jam wrote: > On 2013/04/09 20:52:08, Ilya Sherman wrote: > > On 2013/04/09 17:22:05, jam wrote: > > > On 2013/04/09 17:14:12, xhwang wrote: > > > > On 2013/04/09 17:02:11, jam wrote: > > > > > On 2013/04/09 16:28:05, xhwang wrote: > > > > > > > it's unclear to me why you're changing chrome? i.e. why not just > send > > > > these > > > > > > uma > > > > > > > stats from the content code, and do it for all pepper plugins (so > you > > > > > wouldn't > > > > > > > need the constants for plugin names from chrome)? > > > > > > > > > > > > Plugins are built separately and we want to have the information about > > > which > > > > > > plugin(s) failed to load for which reason. For example, knowing that > 2% > > of > > > > all > > > > > > plugins failed to load on Windows is pretty much useless. We still > don't > > > > know > > > > > > which one failed. By differentiating failure cases with plugin names, > > > we'll > > > > > know > > > > > > which one is failing and this makes investigating much easier. > However, > > > > plugin > > > > > > name parsing cannot be done in content code, that why I change chrome > to > > > do > > > > > > that. > > > > > > > > > > I'm not suggesting sending just percentage stats. i'm suggesting that > > > content > > > > > send the name of the plugin (say the base part of the filename). > > > > > > > > One problem with this approach is that the plugin names on different > > platforms > > > > are different (e.g. > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/chro...). > > > > So we'll have different histograms on different platforms, which may end > up > > > with > > > > too many small histograms. > > > > > > > > Also, if we ever change the plugin name, we'll end up with a new > histogram. > > > > > > > > The current CL doesn't have these two problems. > > > > > > These seem like minor issues. This is just a histogram for debugging, and > it's > > > likely that one would want to break out these stats per platform anyways. > > > Changing the plugin name seems like a rare thing, and even if it happens, > > > getting a new histogram seems like a minor cost. > > > > This is not a minor issue. We should never write a code path that can lead to > > an unbounded number of histograms being created. Each new histogram imposes a > > performance cost, both in Chrome and in the eventual processing of the data. > > No one mentioned unbounded. There's a small set of ppapi plugins. Can't an arbitrary developer write a ppapi plugin that users install? What makes the set guaranteed to be small?
> Can't an arbitrary developer write a ppapi plugin that users install? What > makes the set guaranteed to be small? No. Unlike npapi, we only load flash/nacl and netflix on cros. there's a way for devs to load a plugin using the command line, but users won't be doing that.
On 2013/04/09 18:30:47, xhwang wrote: > Plugin.PluginLoadResult.PepperFlashPlayer.plugin > Plugin.PluginLoadResult.pepflashplayer.dll > Plugin.PluginLoadResult.libpepflashplayer.so > Plugin.PluginLoadResult.widevinecdmadapter.plugin > Plugin.PluginLoadResult.widevinecdmadapter.dll > Plugin.PluginLoadResult.libwidevinecdmadapter.so > Plugin.BrokerLoadResult.PepperFlashPlayer.plugin > Plugin.BrokerLoadResult.pepflashplayer.dll > Plugin.BrokerLoadResult.libpepflashplayer.so > Plugin.BrokerLoadResult.widevinecdmadapter.plugin > Plugin.BrokerLoadResult.widevinecdmadapter.dll > Plugin.BrokerLoadResult.libwidevinecdmadapter.so > (some combination won't appear in real life though) We could reduce the number by canonicalizing the filenames so that they are (mostly) common across OSes. For example, lower case all, drop the extension, and remove "lib" from the beginning. In the examples above, only the Mac Flash filename wouldn't fit (pep vs. pepper), and we could solve that if we felt it was important. The main drawback is that we would be reporting values that (I think) would be dropped on the floor unless the XML file was updated. If you didn't see your data, you would know to update the file (per comments), but we might not know that we are dropping data. (If we stick with the current solution, maybe we should add an "Other" case.) On 2013/04/09 22:59:00, jam wrote: > > Can't an arbitrary developer write a ppapi plugin that users install? What > > makes the set guaranteed to be small? > > No. Unlike npapi, we only load flash/nacl and netflix on cros. there's a way for > devs to load a plugin using the command line, but users won't be doing that. Does anyone know if individual .nexe files pass through here?
> Does anyone know if individual .nexe files pass through here? no
On 2013/04/10 00:49:41, ddorwin wrote: > On 2013/04/09 18:30:47, xhwang wrote: > > Plugin.PluginLoadResult.PepperFlashPlayer.plugin > > Plugin.PluginLoadResult.pepflashplayer.dll > > Plugin.PluginLoadResult.libpepflashplayer.so > > Plugin.PluginLoadResult.widevinecdmadapter.plugin > > Plugin.PluginLoadResult.widevinecdmadapter.dll > > Plugin.PluginLoadResult.libwidevinecdmadapter.so > > Plugin.BrokerLoadResult.PepperFlashPlayer.plugin > > Plugin.BrokerLoadResult.pepflashplayer.dll > > Plugin.BrokerLoadResult.libpepflashplayer.so > > Plugin.BrokerLoadResult.widevinecdmadapter.plugin > > Plugin.BrokerLoadResult.widevinecdmadapter.dll > > Plugin.BrokerLoadResult.libwidevinecdmadapter.so > > (some combination won't appear in real life though) > > We could reduce the number by canonicalizing the filenames so that they are > (mostly) common across OSes. For example, lower case all, drop the extension, > and remove "lib" from the beginning. In the examples above, only the Mac Flash > filename wouldn't fit (pep vs. pepper), and we could solve that if we felt it > was important. This seems a good solution that'll make everybody happy (at least happier). Actually we have a GetNativeLibraryName() function that adds "lib", "so", etc to make a library name. (https://code.google.com/p/chromium/codesearch#chromium/src/base/native_librar...) We can implement the reverse of this function to get a canonical name of the libraries. > > The main drawback is that we would be reporting values that (I think) would be > dropped on the floor unless the XML file was updated. If you didn't see your > data, you would know to update the file (per comments), but we might not know > that we are dropping data. (If we stick with the current solution, maybe we > should add an "Other" case.) > > On 2013/04/09 22:59:00, jam wrote: > > > Can't an arbitrary developer write a ppapi plugin that users install? What > > > makes the set guaranteed to be small? > > > > No. Unlike npapi, we only load flash/nacl and netflix on cros. there's a way > for > > devs to load a plugin using the command line, but users won't be doing that. > > Does anyone know if individual .nexe files pass through here?
On 2013/04/10 01:08:53, xhwang wrote: > On 2013/04/10 00:49:41, ddorwin wrote: > > On 2013/04/09 18:30:47, xhwang wrote: > > > Plugin.PluginLoadResult.PepperFlashPlayer.plugin > > > Plugin.PluginLoadResult.pepflashplayer.dll > > > Plugin.PluginLoadResult.libpepflashplayer.so > > > Plugin.PluginLoadResult.widevinecdmadapter.plugin > > > Plugin.PluginLoadResult.widevinecdmadapter.dll > > > Plugin.PluginLoadResult.libwidevinecdmadapter.so > > > Plugin.BrokerLoadResult.PepperFlashPlayer.plugin > > > Plugin.BrokerLoadResult.pepflashplayer.dll > > > Plugin.BrokerLoadResult.libpepflashplayer.so > > > Plugin.BrokerLoadResult.widevinecdmadapter.plugin > > > Plugin.BrokerLoadResult.widevinecdmadapter.dll > > > Plugin.BrokerLoadResult.libwidevinecdmadapter.so > > > (some combination won't appear in real life though) > > > > We could reduce the number by canonicalizing the filenames so that they are > > (mostly) common across OSes. For example, lower case all, drop the extension, > > and remove "lib" from the beginning. In the examples above, only the Mac Flash > > filename wouldn't fit (pep vs. pepper), and we could solve that if we felt it > > was important. > > This seems a good solution that'll make everybody happy (at least happier). > Actually we have a GetNativeLibraryName() function that adds "lib", "so", etc to > make a library name. this seems like unnecessary complexity. let's not do this. whoever is looking at loading error will want to factor this out by OS anyways. so adding more code bloat to make them have one name for all OSs seems unnecessary. basically, what i'm trying to say is that every change should be done in the simplest possible way that accomplishes what we need.
On 2013/04/10 02:41:41, jam wrote: > On 2013/04/10 01:08:53, xhwang wrote: > > On 2013/04/10 00:49:41, ddorwin wrote: > > > On 2013/04/09 18:30:47, xhwang wrote: > > > > Plugin.PluginLoadResult.PepperFlashPlayer.plugin > > > > Plugin.PluginLoadResult.pepflashplayer.dll > > > > Plugin.PluginLoadResult.libpepflashplayer.so > > > > Plugin.PluginLoadResult.widevinecdmadapter.plugin > > > > Plugin.PluginLoadResult.widevinecdmadapter.dll > > > > Plugin.PluginLoadResult.libwidevinecdmadapter.so > > > > Plugin.BrokerLoadResult.PepperFlashPlayer.plugin > > > > Plugin.BrokerLoadResult.pepflashplayer.dll > > > > Plugin.BrokerLoadResult.libpepflashplayer.so > > > > Plugin.BrokerLoadResult.widevinecdmadapter.plugin > > > > Plugin.BrokerLoadResult.widevinecdmadapter.dll > > > > Plugin.BrokerLoadResult.libwidevinecdmadapter.so > > > > (some combination won't appear in real life though) > > > > > > We could reduce the number by canonicalizing the filenames so that they are > > > (mostly) common across OSes. For example, lower case all, drop the > extension, > > > and remove "lib" from the beginning. In the examples above, only the Mac > Flash > > > filename wouldn't fit (pep vs. pepper), and we could solve that if we felt > it > > > was important. > > > > This seems a good solution that'll make everybody happy (at least happier). > > Actually we have a GetNativeLibraryName() function that adds "lib", "so", etc > to > > make a library name. > > this seems like unnecessary complexity. let's not do this. whoever is looking at > loading error will want to factor this out by OS anyways. so adding more code > bloat to make them have one name for all OSs seems unnecessary. Reducing the number of histogram names helps keep histograms.xml smaller and the backend processing that much faster. It's also easier to compare data between OSes if each OS's histograms are named the same. > basically, what i'm trying to say is that every change should be done in the > simplest possible way that accomplishes what we need.
On 2013/04/10 02:50:13, Ilya Sherman wrote: > Reducing the number of histogram names helps keep histograms.xml smaller and the > backend processing that much faster. Can you quantify the cost on the server for each additional enry? A few weeks ago I tried to remove entries that weren't used for 2 years, and was told that they're kept around so that people don't reuse them. Seems that can be done through other means, and the file would be much smaller that way. at the end of the day, i'd rather we bloat an xml file that isn't shipped to users vs code that's shipped to hundreds of millions. > It's also easier to compare data between > OSes if each OS's histograms are named the same. really? if they have the same prefix, they will be listed right beside each other when searching. > > > basically, what i'm trying to say is that every change should be done in the > > simplest possible way that accomplishes what we need.
ddorwin/piman: Change back to report UMA from PpapiThread based on offline discussion. PTAL! Thanks a lot for the patience.
lgtm
https://codereview.chromium.org/13548005/diff/113001/content/ppapi_plugin/ppa... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/13548005/diff/113001/content/ppapi_plugin/ppa... content/ppapi_plugin/ppapi_thread.cc:364: ReportLoadResult(path, LOAD_SUCCESS); Since the comment above applies to the next line, move this after the next line. https://codereview.chromium.org/13548005/diff/113001/content/ppapi_plugin/ppa... content/ppapi_plugin/ppapi_thread.cc:484: histogram_name << "Plugin." << (is_broker_ ? "Broker" : "Plugin") Plugin.Plugin.? How about Ppapi.Plugin/Broker? (Or Pepper.) I'm assuming we woudln't want to share this with NPAPI.
https://codereview.chromium.org/13548005/diff/113001/content/ppapi_plugin/ppa... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/13548005/diff/113001/content/ppapi_plugin/ppa... content/ppapi_plugin/ppapi_thread.cc:364: ReportLoadResult(path, LOAD_SUCCESS); On 2013/04/15 20:34:36, ddorwin wrote: > Since the comment above applies to the next line, move this after the next line. Done. https://codereview.chromium.org/13548005/diff/113001/content/ppapi_plugin/ppa... content/ppapi_plugin/ppapi_thread.cc:484: histogram_name << "Plugin." << (is_broker_ ? "Broker" : "Plugin") On 2013/04/15 20:34:36, ddorwin wrote: > Plugin.Plugin.? > How about Ppapi.Plugin/Broker? (Or Pepper.) I'm assuming we woudln't want to > share this with NPAPI. "Plugin" is the group name for all plugin related metrics. I won't change that. How about Plugin.PpapiPlugin/PpapiBroker...?
lgtm https://codereview.chromium.org/13548005/diff/119001/content/ppapi_plugin/ppa... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/13548005/diff/119001/content/ppapi_plugin/ppa... content/ppapi_plugin/ppapi_thread.cc:363: ReportLoadResult(path, LOAD_SUCCESS); nit: I meant this should probably be the last line in the function.
https://codereview.chromium.org/13548005/diff/119001/content/ppapi_plugin/ppa... File content/ppapi_plugin/ppapi_thread.cc (right): https://codereview.chromium.org/13548005/diff/119001/content/ppapi_plugin/ppa... content/ppapi_plugin/ppapi_thread.cc:363: ReportLoadResult(path, LOAD_SUCCESS); On 2013/04/15 20:54:50, ddorwin wrote: > nit: I meant this should probably be the last line in the function. Done.
jar/ddorwin: please take a look at the histograms.xml file. The histogram name now use an '_' instead of a '.' so that we can use the field trial.
lgtm
notes on xml file below. https://codereview.chromium.org/13548005/diff/77003/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/13548005/diff/77003/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:5254: <fieldtrial name="PpapiPluginName"> One subtle point: I think you append the group names below with a period.... so you'll need to add: <fieldtrial name="PpapiPluginName" separator="."> You can check to be sure what you have "works" by running ./update_descriptions.py, and looking at the txt file that is generated. https://codereview.chromium.org/13548005/diff/77003/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:5258: <group name="PepperFlashPlayer.plugin" label="Flash player on Mac"/> This is much nicer! thanks!
https://codereview.chromium.org/13548005/diff/77003/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/13548005/diff/77003/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:5254: <fieldtrial name="PpapiPluginName"> On 2013/04/16 18:07:45, jar wrote: > One subtle point: I think you append the group names below with a period.... so > you'll need to add: > > <fieldtrial name="PpapiPluginName" separator="."> > > You can check to be sure what you have "works" by running > ./update_descriptions.py, and looking at the txt file that is generated. I didn't know about "separator" so I updated my source code to use "_" instead of ".". (Please see https://codereview.chromium.org/13548005/diff/77003/content/ppapi_plugin/ppap...) Is there any recommendation about using "_" or "." as separators? PS, I don't see update_descriptions.py under src/tools/metrics/histograms. Is it deprecated? https://codereview.chromium.org/13548005/diff/77003/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:5258: <group name="PepperFlashPlayer.plugin" label="Flash player on Mac"/> On 2013/04/16 18:07:45, jar wrote: > This is much nicer! thanks! Thank you for the tip!
histograms LGTM Comment for Isherman to ponder as well below. https://codereview.chromium.org/13548005/diff/77003/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/13548005/diff/77003/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:5254: <fieldtrial name="PpapiPluginName"> hmm... I think it must still reside in the internal repo... sorry. We probably should have a similar file sitting in that directory... as it can be helpful. On 2013/04/16 19:02:21, xhwang wrote: > On 2013/04/16 18:07:45, jar wrote: > > One subtle point: I think you append the group names below with a period.... > so > > you'll need to add: > > > > <fieldtrial name="PpapiPluginName" separator="."> > > > > You can check to be sure what you have "works" by running > > ./update_descriptions.py, and looking at the txt file that is generated. > > I didn't know about "separator" so I updated my source code to use "_" instead > of ".". (Please see > https://codereview.chromium.org/13548005/diff/77003/content/ppapi_plugin/ppap...) > Is there any recommendation about using "_" or "." as separators? > > PS, I don't see update_descriptions.py under src/tools/metrics/histograms. Is it > deprecated?
https://codereview.chromium.org/13548005/diff/77003/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/13548005/diff/77003/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:5254: <fieldtrial name="PpapiPluginName"> On 2013/04/16 21:12:33, jar wrote: > hmm... I think it must still reside in the internal repo... sorry. We probably > should have a similar file sitting in that directory... as it can be helpful. update_descriptions.py needs to combine data from the public and the internal repository, so it doesn't really make sense to open-source it. But, validate_format.py computes all the public histogram description data. If you want to see the generated histogram names, you could dump the |histograms| to a file, or compute |histogram_names = extract_histograms.ExtractNames(histograms)| first if you just want to see the names.
Message was sent while issue was closed.
Committed patchset #19 manually as r194494 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
