Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2787103003:
Add System health stories for Emerging market (Closed)
DescriptionAdd System health stories for Emerging market
This CL adds system health stories which are important for emerging
market.
BUG=708300, 709436
Review-Url: https://codereview.chromium.org/2787103003
Cr-Commit-Position: refs/heads/master@{#465012}
Committed: https://chromium.googlesource.com/chromium/src/+/2c96514f650b4db1f6abebf0d6b307d65bca3e5b
Patch Set 1 #
Total comments: 10
Patch Set 2 : Fixes. #Patch Set 3 : Fixes. #
Total comments: 19
Patch Set 4 : Address comments, clean up #Patch Set 5 : Add instagram and newtab -> search. #Patch Set 6 : nits. #
Total comments: 16
Patch Set 7 : Only add new stories. #
Total comments: 10
Patch Set 8 : fixes. #
Total comments: 10
Patch Set 9 : Remove youtube and add recording. #Patch Set 10 : nits. #
Total comments: 4
Patch Set 11 : Fix insta, flipkart and maps. #Patch Set 12 : fix insta flipkart and maps. #Patch Set 13 : Disable flipkart, fix story tag #
Total comments: 4
Patch Set 14 : Fix a description. #Messages
Total messages: 60 (31 generated)
Description was changed from ========== System health stories for EM BUG= ========== to ========== System health stories for EM BUG= ==========
ssid@chromium.org changed reviewers: + perezju@chromium.org
+Juan please take an initial look at the usage of api and locations of stories added. Also, please add suggestions for removing stories. Thanks!
perezju@chromium.org changed reviewers: + nednguyen@google.com
+nednguyen who might have more comments https://codereview.chromium.org/2787103003/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/blank_stories.py (right): https://codereview.chromium.org/2787103003/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/blank_stories.py:32: NAME = 'blank:about:newtab' I don't think this is a "blank" story, it's actually hopping for content to get loaded. Maybe this could be browse:chrome:newtab? https://codereview.chromium.org/2787103003/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/loading_stories.py (right): https://codereview.chromium.org/2787103003/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/loading_stories.py:77: SUPPORTED_PLATFORMS = platforms.MOBILE_ONLY We actually want to move away (as much as possible) from stories that just load a page. Could we select a few of these and make them into browse:* stories?
https://codereview.chromium.org/2787103003/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/searching_stories.py (right): https://codereview.chromium.org/2787103003/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/searching_stories.py:51: platform.device.RunShellCommand(['input', 'text', 'drake'], Hmhh, this should be using https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... I am shocked that we expose the "device" API directly to user. +perezju@
Overall look not ok to me. Can you add EMERGING_MARKET tag? https://cs.chromium.org/chromium/src/tools/perf/page_sets/system_health/story... https://codereview.chromium.org/2787103003/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/loading_stories.py (right): https://codereview.chromium.org/2787103003/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/loading_stories.py:434: action_runner.Wait(1) One thing about these wait is if the page misbehaves, you won't know. Can you do s.t like: WaiForElementToBeInViewport(..)
Patchset #2 (id:20001) has been deleted
Do you think it looks better now? Juan, how do I decide which stories to remove. Is there a doc on why the existing stories were chosen? https://codereview.chromium.org/2787103003/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/blank_stories.py (right): https://codereview.chromium.org/2787103003/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/blank_stories.py:32: NAME = 'blank:about:newtab' On 2017/03/31 08:45:54, perezju wrote: > I don't think this is a "blank" story, it's actually hopping for content to get > loaded. > > Maybe this could be browse:chrome:newtab? Done. https://codereview.chromium.org/2787103003/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/loading_stories.py (right): https://codereview.chromium.org/2787103003/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/loading_stories.py:77: SUPPORTED_PLATFORMS = platforms.MOBILE_ONLY On 2017/03/31 08:45:54, perezju wrote: > We actually want to move away (as much as possible) from stories that just load > a page. Could we select a few of these and make them into browse:* stories? Moved a few stories. Thanks! Some of these don't fit in browsing stories, we just want to load the page and it's good enough, like irctc and cric buzz. https://codereview.chromium.org/2787103003/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/loading_stories.py:434: action_runner.Wait(1) On 2017/03/31 09:00:16, nednguyen wrote: > One thing about these wait is if the page misbehaves, you won't know. Can you do > s.t like: > WaiForElementToBeInViewport(..) I actually added these waits as a user behavior rather than waiting for page to load. I see there are other Wait calls in searching_stories which simulate user reading the page. The waitForElement in the first line waits for the actual map surface. These actions will only move the surface around. So, I am not waiting for any specific element here. https://codereview.chromium.org/2787103003/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/searching_stories.py (right): https://codereview.chromium.org/2787103003/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/searching_stories.py:51: platform.device.RunShellCommand(['input', 'text', 'drake'], On 2017/03/31 08:56:55, nednguyen wrote: > Hmhh, this should be using > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... > > I am shocked that we expose the "device" API directly to user. +perezju@ Sorry I did not notice we have different action runner. I was planning to expose device in platform to be able to do this. Fixed this.
perezju@chromium.org changed reviewers: + mythria@chromium.org
On 2017/04/04 03:43:48, ssid wrote: > Do you think it looks better now? > Juan, how do I decide which stories to remove. Is there a doc on why the > existing stories were chosen? The original set of stories were chosen by Petr, this is the doc on which he was working at the time: https://docs.google.com/spreadsheets/d/1t15Ya5ssYBeXAZhHm3RJqfwBRpgWsxoib8_kw... (see "short stories" tab.) Anyway, today I'll be looking at the set of stories and make a proposal on which to keep/replace/remove. I'll post back here when I have that info. https://codereview.chromium.org/2787103003/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/loading_stories.py (right): https://codereview.chromium.org/2787103003/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/loading_stories.py:434: action_runner.Wait(1) On 2017/04/04 03:43:48, ssid wrote: > On 2017/03/31 09:00:16, nednguyen wrote: > > One thing about these wait is if the page misbehaves, you won't know. Can you > do > > s.t like: > > WaiForElementToBeInViewport(..) > > I actually added these waits as a user behavior rather than waiting for page to > load. I see there are other Wait calls in searching_stories which simulate user > reading the page. > The waitForElement in the first line waits for the actual map surface. These > actions will only move the surface around. So, I am not waiting for any specific > element here. I would suggest to add some comments to any remaining waits to document that this is the intention. https://codereview.chromium.org/2787103003/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/searching_stories.py (right): https://codereview.chromium.org/2787103003/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/searching_stories.py:51: platform.device.RunShellCommand(['input', 'text', 'drake'], On 2017/03/31 08:56:55, nednguyen (slow til 4-10) wrote: > Hmhh, this should be using > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... > > I am shocked that we expose the "device" API directly to user. +perezju@ I've filed https://github.com/catapult-project/catapult/issues/3469 https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:506: menu_button = package+':id/menu_button' I think we should add an "app_ui" property to the android_browser_backend (similar to the system_ui on platform) which has the package name pre-filled. This way you should be able to just say: action_runner.tab.browser.app_ui.WaitForUiNode(resouce_id='menu_item_text') See: https://github.com/catapult-project/catapult/blob/master/devil/devil/android/... https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:516: class _ArticleBrowsingStoryEM(_ArticleBrowsingStory): I don't think we need this class. It's better to add PLATFORM and TAGS explicitly to each story below. https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:563: _DIRECTIONS_LOADED = '[class="ml-fab-inner ml-button ml-button-navigation-fab"]' Do sync with Mythri, I think she was also working on getting a mobile version of browse:tools:maps. https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/media_stories.py (right): https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:149: pass # Audio autoplays on Pandora, no need to search. Pandora? https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/searching_stories.py (right): https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/searching_stories.py:55: platform.android_action_runner.InputKeyEvent(66) use the constant name from: https://github.com/catapult-project/catapult/blob/master/devil/devil/android/... https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/story_tags.py (right): https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/story_tags.py:31: 'connections.') Rephrase suggestion: Story has significant usage in emerging markets with low-end mobile devices and slow network connections.
Patchset #4 (id:80001) has been deleted
On 2017/04/04 09:23:38, perezju wrote: > On 2017/04/04 03:43:48, ssid wrote: > > Do you think it looks better now? > > Juan, how do I decide which stories to remove. Is there a doc on why the > > existing stories were chosen? > > The original set of stories were chosen by Petr, this is the doc on which he was > working at the time: > https://docs.google.com/spreadsheets/d/1t15Ya5ssYBeXAZhHm3RJqfwBRpgWsxoib8_kw... > (see "short stories" tab.) > > Anyway, today I'll be looking at the set of stories and make a proposal on which > to keep/replace/remove. I'll post back here when I have that info. > > https://codereview.chromium.org/2787103003/diff/1/tools/perf/page_sets/system... > File tools/perf/page_sets/system_health/loading_stories.py (right): > Okay if you are looking at which stories to remove, should I wait till we deice that, or should I add the new stories and remove old ones when we decide? https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:506: menu_button = package+':id/menu_button' On 2017/04/04 09:23:38, perezju wrote: > I think we should add an "app_ui" property to the android_browser_backend > (similar to the system_ui on platform) which has the package name pre-filled. > This way you should be able to just say: > > action_runner.tab.browser.app_ui.WaitForUiNode(resouce_id='menu_item_text') > > See: > https://github.com/catapult-project/catapult/blob/master/devil/devil/android/... Done. https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:516: class _ArticleBrowsingStoryEM(_ArticleBrowsingStory): On 2017/04/04 09:23:38, perezju wrote: > I don't think we need this class. It's better to add PLATFORM and TAGS > explicitly to each story below. Done. https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:563: _DIRECTIONS_LOADED = '[class="ml-fab-inner ml-button ml-button-navigation-fab"]' On 2017/04/04 09:23:38, perezju wrote: > Do sync with Mythri, I think she was also working on getting a mobile version of > browse:tools:maps. Yes I had made this similar to the new Maps benchmark in the other CL. Will rebase CL once that lands. https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/media_stories.py (right): https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:149: pass # Audio autoplays on Pandora, no need to search. On 2017/04/04 09:23:38, perezju wrote: > Pandora? Done. https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/searching_stories.py (right): https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/searching_stories.py:55: platform.android_action_runner.InputKeyEvent(66) On 2017/04/04 09:23:38, perezju wrote: > use the constant name from: > https://github.com/catapult-project/catapult/blob/master/devil/devil/android/... Done. https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/story_tags.py (right): https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/story_tags.py:31: 'connections.') On 2017/04/04 09:23:38, perezju wrote: > Rephrase suggestion: Story has significant usage in emerging markets with > low-end mobile devices and slow network connections. Done.
Description was changed from ========== System health stories for EM BUG= ========== to ========== Add System health stories for Emerging market This CL adds system health stories which are important for emerging market. BUG=708300 ==========
Thanks a lot for doing this. I was also working on the android version of maps page, but I guess we no longer need it. I have added a couple of comments/suggestions for the maps page. Thanks, Mythri. https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:551: URL='https://maps.google.co.in/' Is there any reason we start with maps.google.co.in instead of just maps.google.com. Since I assume when we search for restaurants near me, it shows restaurants around mountain view. Also, is the maps.google.co.in any different from other locations? If it is different I suggest we start with a smaller area something like: https://www.google.co.in/maps/@12.9708205,77.5879784,15.88z and search for restaurants (instead of restaurants near me) and find directions from somewhere near that location. https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:556: _MAPS_ZOOM_IN_SELECTOR = '[aria-label="Zoom in"]' I think this is not required. https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:563: _DIRECTIONS_LOADED = '[class="ml-fab-inner ml-button ml-button-navigation-fab"]' On 2017/04/04 20:59:49, ssid wrote: > On 2017/04/04 09:23:38, perezju wrote: > > Do sync with Mythri, I think she was also working on getting a mobile version > of > > browse:tools:maps. > > Yes I had made this similar to the new Maps benchmark in the other CL. Will > rebase CL once that lands. Thanks for doing this. I was also working on the android version of the maps benchmark, but I guess it won't be needed anymore. https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:575: action_runner.Wait(3) # User looking t restaurants It would be great, if we could add some scrolling and/or Zooming here.
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
Juan, I moved the stories to appropriate files. I have not renamed any story yet to avoid extra confusion on the dashboard. I will remove stories after a week and rename stories the week after. This is to avoid missing regressions. wdyt? https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:551: URL='https://maps.google.co.in/' On 2017/04/05 09:30:15, mythria wrote: > Is there any reason we start with maps.google.co.in instead of just > http://maps.google.com. Since I assume when we search for restaurants near me, it shows > restaurants around mountain view. Also, is the maps.google.co.in any different > from other locations? If it is different I suggest we start with a smaller area > something like: https://www.google.co.in/maps/@12.9708205,77.5879784,15.88z and > search for restaurants (instead of restaurants near me) and find directions from > somewhere near that location. Initially i added .co.in when I didn't have the searching. Changed it to .com to avoid issues. https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:556: _MAPS_ZOOM_IN_SELECTOR = '[aria-label="Zoom in"]' On 2017/04/05 09:30:15, mythria wrote: > I think this is not required. Done. https://codereview.chromium.org/2787103003/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:563: _DIRECTIONS_LOADED = '[class="ml-fab-inner ml-button ml-button-navigation-fab"]' On 2017/04/05 09:30:15, mythria wrote: > On 2017/04/04 20:59:49, ssid wrote: > > On 2017/04/04 09:23:38, perezju wrote: > > > Do sync with Mythri, I think she was also working on getting a mobile > version > > of > > > browse:tools:maps. > > > > Yes I had made this similar to the new Maps benchmark in the other CL. Will > > rebase CL once that lands. > > Thanks for doing this. I was also working on the android version of the maps > benchmark, but I guess it won't be needed anymore. Acknowledged.
On 2017/04/07 22:18:34, ssid wrote: > Juan, I moved the stories to appropriate files. I have not renamed any story yet > to avoid extra confusion on the dashboard. I will remove stories after a week > and rename stories the week after. This is to avoid missing regressions. wdyt? To make this easier to review, let's *only* add new stories in this CL. Do not move existing stories between files; and do not remove yet old stories (I guess unless you are providing a direct replacement). The plan to remove later and then rename later sgtm. https://codereview.chromium.org/2787103003/diff/180001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2787103003/diff/180001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:262: 'aw-search-results\\"]') I'm not expert on CSS selectors, but is there a way and would it work to select only for links that contain e.g. "aw-search-results" in their class? It sounds that things like "a-spacing-none" and "a-link-normal" are not really distinguishing elements of the links you want to find and click on. https://codereview.chromium.org/2787103003/diff/180001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:283: ITEM_SELECTOR = '[onerror=\\"this.style.display=\'none\'\\"]' :( I guess there is no better way to match these items? Also sorry for the quoting madness, I filed crbug.com/709929 https://codereview.chromium.org/2787103003/diff/180001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:291: class GoogleMapsMobileStory(ArticleBrowsingStory): does this really need to subclass ArticleBrowsingStory? Sounds like it's no longer browsing "Articles" any more. https://codereview.chromium.org/2787103003/diff/180001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/loading_stories.py (right): https://codereview.chromium.org/2787103003/diff/180001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/loading_stories.py:75: NAME = 'load:shopping:avito' As suggested on the doc, I propose *not* to add load stories for avito/olx; But it's fine to add load stories for flipkart/lazada. The rationale is that we would like to remove all load stories in the future, if they can be reasonably covered by just measuring "memory usage after load" on existing browse stories. https://codereview.chromium.org/2787103003/diff/180001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/loading_stories.py:159: SUPPORTED_PLATFORMS = platforms.DESKTOP_ONLY I thought you were not going to remove stories now? https://codereview.chromium.org/2787103003/diff/180001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/loading_stories.py:230: NAME = 'load:news:cricbuzz' Similarly, do not add more load news stories. But OK to add load stories load stories that match your new EM stories (e.g. load:shopping:flipkart). https://codereview.chromium.org/2787103003/diff/180001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/searching_stories.py (right): https://codereview.chromium.org/2787103003/diff/180001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/searching_stories.py:48: NAME = 'search:google:omnibox' this one should be called already: browse:chrome:omnibox https://codereview.chromium.org/2787103003/diff/180001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/searching_stories.py:71: NAME = 'search:chrome:newtab' should be called already: browse:chrome:newtab
Patchset #7 (id:200001) has been deleted
I thought moving stories to files would it's fiels would look better. But yes just adding new stories is easier to review. ptal, thanks! https://codereview.chromium.org/2787103003/diff/180001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2787103003/diff/180001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:262: 'aw-search-results\\"]') On 2017/04/10 10:13:56, perezju wrote: > I'm not expert on CSS selectors, but is there a way and would it work to select > only for links that contain e.g. "aw-search-results" in their class? It sounds > that things like "a-spacing-none" and "a-link-normal" are not really > distinguishing elements of the links you want to find and click on. Cool. Didn't know this was possible. https://codereview.chromium.org/2787103003/diff/180001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:283: ITEM_SELECTOR = '[onerror=\\"this.style.display=\'none\'\\"]' On 2017/04/10 10:13:56, perezju wrote: > :( I guess there is no better way to match these items? > > Also sorry for the quoting madness, I filed crbug.com/709929 There is this other way. But i thought dummy_img looks weirder than that. https://codereview.chromium.org/2787103003/diff/180001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:291: class GoogleMapsMobileStory(ArticleBrowsingStory): On 2017/04/10 10:13:56, perezju wrote: > does this really need to subclass ArticleBrowsingStory? Sounds like it's no > longer browsing "Articles" any more. Sorry fixed. https://codereview.chromium.org/2787103003/diff/180001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/loading_stories.py (right): https://codereview.chromium.org/2787103003/diff/180001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/loading_stories.py:75: NAME = 'load:shopping:avito' On 2017/04/10 10:13:56, perezju wrote: > As suggested on the doc, I propose *not* to add load stories for avito/olx; But > it's fine to add load stories for flipkart/lazada. > > The rationale is that we would like to remove all load stories in the future, if > they can be reasonably covered by just measuring "memory usage after load" on > existing browse stories. Done. https://codereview.chromium.org/2787103003/diff/180001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/loading_stories.py:159: SUPPORTED_PLATFORMS = platforms.DESKTOP_ONLY On 2017/04/10 10:13:56, perezju wrote: > I thought you were not going to remove stories now? Sorry, this was error. https://codereview.chromium.org/2787103003/diff/180001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/loading_stories.py:230: NAME = 'load:news:cricbuzz' On 2017/04/10 10:13:56, perezju wrote: > Similarly, do not add more load news stories. > > But OK to add load stories load stories that match your new EM stories (e.g. > load:shopping:flipkart). I do not know how else i can add these stories. I have removed as much as I could from load stories. https://codereview.chromium.org/2787103003/diff/180001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/searching_stories.py (right): https://codereview.chromium.org/2787103003/diff/180001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/searching_stories.py:48: NAME = 'search:google:omnibox' On 2017/04/10 10:13:57, perezju wrote: > this one should be called already: browse:chrome:omnibox Done. https://codereview.chromium.org/2787103003/diff/180001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/searching_stories.py:71: NAME = 'search:chrome:newtab' On 2017/04/10 10:13:57, perezju wrote: > should be called already: browse:chrome:newtab Done.
Patchset #7 (id:220001) has been deleted
Thanks ssid, this is looking pretty much ready. lgtm but wait for Ned to have a final pass, and comment on adding the extra loading only news stories. https://codereview.chromium.org/2787103003/diff/240001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2787103003/diff/240001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:396: class YouTubeMobileStory(_MediaBrowsingStory): You should be removing this one https://codereview.chromium.org/2787103003/diff/240001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:496: NAME = 'browse:media:instagram' nit: let's call this browse:social:instagram. https://codereview.chromium.org/2787103003/diff/240001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/loading_stories.py (left): https://codereview.chromium.org/2787103003/diff/240001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/loading_stories.py:219: # No way to disable autoplay on desktop. nit: keep this comment? https://codereview.chromium.org/2787103003/diff/240001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/media_stories.py (right): https://codereview.chromium.org/2787103003/diff/240001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:147: nit: remove one blank line https://codereview.chromium.org/2787103003/diff/240001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/searching_stories.py (right): https://codereview.chromium.org/2787103003/diff/240001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/searching_stories.py:101: app_ui.WaitForUiNode(resource_id=resource) WaitForUiNode already returns the node when found, so you should be able to use it directly instead of your _GetUiNode.
Thanks addressed comments. https://codereview.chromium.org/2787103003/diff/240001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2787103003/diff/240001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:396: class YouTubeMobileStory(_MediaBrowsingStory): On 2017/04/11 09:43:35, perezju wrote: > You should be removing this one Done. https://codereview.chromium.org/2787103003/diff/240001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:496: NAME = 'browse:media:instagram' On 2017/04/11 09:43:35, perezju wrote: > nit: let's call this browse:social:instagram. Done. https://codereview.chromium.org/2787103003/diff/240001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/loading_stories.py (left): https://codereview.chromium.org/2787103003/diff/240001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/loading_stories.py:219: # No way to disable autoplay on desktop. On 2017/04/11 09:43:35, perezju wrote: > nit: keep this comment? Done. https://codereview.chromium.org/2787103003/diff/240001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/media_stories.py (right): https://codereview.chromium.org/2787103003/diff/240001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:147: On 2017/04/11 09:43:35, perezju wrote: > nit: remove one blank line Done. https://codereview.chromium.org/2787103003/diff/240001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/searching_stories.py (right): https://codereview.chromium.org/2787103003/diff/240001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/searching_stories.py:101: app_ui.WaitForUiNode(resource_id=resource) On 2017/04/11 09:43:35, perezju wrote: > WaitForUiNode already returns the node when found, so you should be able to use > it directly instead of your _GetUiNode. Done.
Ned, ptal thanks!
On 2017/04/11 21:53:06, ssid wrote: > Ned, ptal thanks! Is there a specific device in which I should record the stories or any device is good?
I would recommend recording these on low end devices (just in case some of the page's server returns different content for low end devices). Also please make sure that you run these many times on the low end devices to find out if any of them are flaky. https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:580: class GoogleMapsMobileStory(system_health_story.SystemHealthStory): Same here: please add docstring explain what this story does in detail https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/media_stories.py (right): https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:120: class YoutubeMobileStory(_PlayMediaStory): Also add docstring describe what this story is doing. https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/searching_stories.py (right): https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/searching_stories.py:44: class SearchOmniboxStory(system_health_story.SystemHealthStory): Can you add description explain what this story does in details? Similar to https://cs.chromium.org/webrtc/src/tools/perf/page_sets/system_health/browsin... https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/searching_stories.py:61: action_runner.WaitForNavigate(15) why is this 15 seconds? Can you make this wait to be more deterministic by changing it to wait for some element (maybe an extra few seconds after that if you think it make the flow looks more realistic) https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/searching_stories.py:66: """Story that loads new tab page and opens menu.""" The story below does more than this. Can you improve the documentation?
Description was changed from ========== Add System health stories for Emerging market This CL adds system health stories which are important for emerging market. BUG=708300 ========== to ========== Add System health stories for Emerging market This CL adds system health stories which are important for emerging market. BUG=708300,709436 ==========
On 2017/04/11 22:24:10, nednguyen wrote: > I would recommend recording these on low end devices (just in case some of the > page's server returns different content for low end devices). > > Also please make sure that you run these many times on the low end devices to > find out if any of them are flaky. > > https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... > File tools/perf/page_sets/system_health/browsing_stories.py (right): > > https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/browsing_stories.py:580: class > GoogleMapsMobileStory(system_health_story.SystemHealthStory): > Same here: please add docstring explain what this story does in detail > > https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... > File tools/perf/page_sets/system_health/media_stories.py (right): > > https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/media_stories.py:120: class > YoutubeMobileStory(_PlayMediaStory): > Also add docstring describe what this story is doing. > > https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... > File tools/perf/page_sets/system_health/searching_stories.py (right): > > https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/searching_stories.py:44: class > SearchOmniboxStory(system_health_story.SystemHealthStory): > Can you add description explain what this story does in details? Similar to > https://cs.chromium.org/webrtc/src/tools/perf/page_sets/system_health/browsin... > > https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/searching_stories.py:61: > action_runner.WaitForNavigate(15) > why is this 15 seconds? Can you make this wait to be more deterministic by > changing it to wait for some element (maybe an extra few seconds after that if > you think it make the flow looks more realistic) > > https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/searching_stories.py:66: """Story that loads > new tab page and opens menu.""" > The story below does more than this. Can you improve the documentation? Also isn't landing this depending on landing https://codereview.chromium.org/2784233006/ ?
Patchset #9 (id:280001) has been deleted
I had some issues with youtube in wpr recording. So, i removed the youtube playing story and got back the old youtube stories. I made suggested changes and added wpr recordings for all stories. I ran the tests with repeat 12 times on a new svelte device and on nexus 5x. Both ran successfully. The catapult side blocking cl just landed. Other catapult changes are not a blocker. ptal thanks! https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:580: class GoogleMapsMobileStory(system_health_story.SystemHealthStory): On 2017/04/11 22:24:10, nednguyen wrote: > Same here: please add docstring explain what this story does in detail Done. https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/media_stories.py (right): https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:120: class YoutubeMobileStory(_PlayMediaStory): On 2017/04/11 22:24:10, nednguyen wrote: > Also add docstring describe what this story is doing. removed this story for now. https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/searching_stories.py (right): https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/searching_stories.py:44: class SearchOmniboxStory(system_health_story.SystemHealthStory): On 2017/04/11 22:24:10, nednguyen wrote: > Can you add description explain what this story does in details? Similar to > https://cs.chromium.org/webrtc/src/tools/perf/page_sets/system_health/browsin... > Done. https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/searching_stories.py:61: action_runner.WaitForNavigate(15) On 2017/04/11 22:24:10, nednguyen wrote: > why is this 15 seconds? Can you make this wait to be more deterministic by > changing it to wait for some element (maybe an extra few seconds after that if > you think it make the flow looks more realistic) I didn't know this argument was not required. removed. https://codereview.chromium.org/2787103003/diff/260001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/searching_stories.py:66: """Story that loads new tab page and opens menu.""" On 2017/04/11 22:24:10, nednguyen wrote: > The story below does more than this. Can you improve the documentation? Done.
The CQ bit was checked by ssid@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: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
telemetry_perf_unittests is timed out on android_n5x_swarming_rel I think you would need to blacklist some stories you plan to remove next in https://cs.chromium.org/chromium/src/tools/perf/benchmarks/system_health_smok... Please file a bug for removing those stories & use that bug number for the comment https://codereview.chromium.org/2787103003/diff/360001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/story_tags.py (right): https://codereview.chromium.org/2787103003/diff/360001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/story_tags.py:29: 'emerging-market', 'Story has significant usage in emerging markets with ' "emerging_market" since we all the tag should be using underscores instead of dash
Patchset #11 (id:340001) has been deleted
Patchset #10 (id:320001) has been deleted
So the swarming bot was timing out because of too many stories failing. I fixed most of them except Flipkart. I couldn't reproduce the issue with Flipkart and it seems to consistently fail on the bots. Is there a way I can actually look at what's happening on the try bot? Any suggestions on how to reproduce the issue? I'm trying Nexus 5x and n mr2 and a total build of chrome. https://codereview.chromium.org/2787103003/diff/360001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/story_tags.py (right): https://codereview.chromium.org/2787103003/diff/360001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/story_tags.py:29: 'emerging-market', 'Story has significant usage in emerging markets with ' On 2017/04/14 00:33:27, nednguyen wrote: > "emerging_market" since we all the tag should be using underscores instead of > dash Done.
https://codereview.chromium.org/2787103003/diff/360001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/story_tags.py (right): https://codereview.chromium.org/2787103003/diff/360001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/story_tags.py:29: 'emerging-market', 'Story has significant usage in emerging markets with ' On 2017/04/17 06:56:31, ssid wrote: > On 2017/04/14 00:33:27, nednguyen wrote: > > "emerging_market" since we all the tag should be using underscores instead of > > dash > > Done. Sorry. Actually not done. I'll fix once the issue with Flipkart is resolved.
On 2017/04/17 06:56:31, ssid wrote: > So the swarming bot was timing out because of too many stories failing. I fixed > most of them except Flipkart. I couldn't reproduce the issue with Flipkart and > it seems to consistently fail on the bots. Is there a way I can actually look at > what's happening on the try bot? Any suggestions on how to reproduce the issue? > I'm trying Nexus 5x and n mr2 and a total build of chrome. I would use swarming command to exactly reproduce what was run on the bot: python swarming.py reproduce -S chromium-swarm.appspot.com 358781e3dad78210 (https://chromium-swarm.appspot.com/task?id=358781e3dad78210&refresh=10&show_r...) Though if you can get down to only 1 failing story, I would suggest disable it with a bug to reenable it later so you can land this CL. > > https://codereview.chromium.org/2787103003/diff/360001/tools/perf/page_sets/s... > File tools/perf/page_sets/system_health/story_tags.py (right): > > https://codereview.chromium.org/2787103003/diff/360001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/story_tags.py:29: 'emerging-market', 'Story > has significant usage in emerging markets with ' > On 2017/04/14 00:33:27, nednguyen wrote: > > "emerging_market" since we all the tag should be using underscores instead of > > dash > > Done.
The CQ bit was checked by ssid@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...
Disabled flipkart, the bot is green now, Ned ptal Thanks! https://codereview.chromium.org/2787103003/diff/360001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/story_tags.py (right): https://codereview.chromium.org/2787103003/diff/360001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/story_tags.py:29: 'emerging-market', 'Story has significant usage in emerging markets with ' On 2017/04/17 06:57:47, ssid wrote: > On 2017/04/17 06:56:31, ssid wrote: > > On 2017/04/14 00:33:27, nednguyen wrote: > > > "emerging_market" since we all the tag should be using underscores instead > of > > > dash > > > > Done. > Sorry. > Actually not done. I'll fix once the issue with Flipkart is resolved. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The telemetry_perf_unittests is now taking 14 minutes on android_n5x_swarming_rel, which is very closed to our 15 minutes budget promised with the lab. Please work on removing the extra sh stories next after this CL. lgtm https://codereview.chromium.org/2787103003/diff/420001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/searching_stories.py (right): https://codereview.chromium.org/2787103003/diff/420001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/searching_stories.py:73: This story does in a loop: nits: change this to: "For each of the search queries in |_SEARCH_TEXTS| below, this story does: _ enter the search query from new tab page _ ... https://codereview.chromium.org/2787103003/diff/420001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/story_tags.py (right): https://codereview.chromium.org/2787103003/diff/420001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/story_tags.py:30: 'low-end mobile devices and slow network connections.') hmhh, actually sorry that I didn't notice slow network connection part. You can set the traffic shaping setting for a story with s.t like traffic_setting = '3g'. But since this CL is already complicated & doing that would increases the cycle time, it could be something to consider in a separate CL.
The CQ bit was checked by ssid@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.
Thanks made changes. Yes, I will send a cl to remove stories soon. https://codereview.chromium.org/2787103003/diff/420001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/searching_stories.py (right): https://codereview.chromium.org/2787103003/diff/420001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/searching_stories.py:73: This story does in a loop: On 2017/04/17 19:20:28, nednguyen wrote: > nits: change this to: > "For each of the search queries in |_SEARCH_TEXTS| below, this story does: > _ enter the search query from new tab page > _ ... Done. https://codereview.chromium.org/2787103003/diff/420001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/story_tags.py (right): https://codereview.chromium.org/2787103003/diff/420001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/story_tags.py:30: 'low-end mobile devices and slow network connections.') On 2017/04/17 19:20:28, nednguyen wrote: > hmhh, actually sorry that I didn't notice slow network connection part. You can > set the traffic shaping setting for a story with s.t like traffic_setting = > '3g'. But since this CL is already complicated & doing that would increases the > cycle time, it could be something to consider in a separate CL. Files https://bugs.chromium.org/p/chromium/issues/detail?id=712288 to keep track. Thanks!
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2787103003/#ps440001 (title: "Fix a description.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 440001, "attempt_start_ts": 1492461433711360,
"parent_rev": "859329ee3ecac2404c5d9358e4eda08d7d27fc03", "commit_rev":
"2c96514f650b4db1f6abebf0d6b307d65bca3e5b"}
Message was sent while issue was closed.
Description was changed from ========== Add System health stories for Emerging market This CL adds system health stories which are important for emerging market. BUG=708300,709436 ========== to ========== Add System health stories for Emerging market This CL adds system health stories which are important for emerging market. BUG=708300,709436 Review-Url: https://codereview.chromium.org/2787103003 Cr-Commit-Position: refs/heads/master@{#465012} Committed: https://chromium.googlesource.com/chromium/src/+/2c96514f650b4db1f6abebf0d6b3... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:440001) as https://chromium.googlesource.com/chromium/src/+/2c96514f650b4db1f6abebf0d6b3...
Message was sent while issue was closed.
This CL removed a number of stories on mobile by making the desktop only. Next time could the CL description make it clear which stories are being removed to make the data-stoppage alert triaging more obvious.
Message was sent while issue was closed.
On 2017/04/27 10:20:48, rmcilroy wrote: > This CL removed a number of stories on mobile by making the desktop only. Next > time could the CL description make it clear which stories are being removed to > make the data-stoppage alert triaging more obvious. Sorry. I will do it next time. |
