1040705 - [b2gpopulate] Provide workload quantities based on Gaia's reference workloads

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Gaia's make reference workload has preset quantities labelled light, medium, heavy, x-heavy. We should provide these through b2gpopulate so consumers can be consistent.

The current values are available at https://github.com/mozilla-b2g/gaia/blob/3a6bc02d3218185240847e433683ccef285188ad/test_media/reference-workload/makeReferenceWorkload.sh#L31

Light
* Pictures: 20
* Music: 20
* Videos: 5
* Contacts: 200
* Messages: 200
* Calls: 50
* Events: 900

Medium:
* Pictures: 50
* Music: 50
* Videos: 10
* Contacts: 500
* Messages: 500
* Calls: 100
* Events: 1300

Heavy:
* Pictures: 100
* Music: 100
* Videos: 20
* Contacts: 1000
* Messages: 1000
* Calls: 200
* Events: 2400

X-Heavy:
* Pictures: 250
* Music: 250
* Videos: 50
* Contacts: 2000
* Messages: 2000
* Calls: 500
* Events: 3200

Something like b2gpopulate.HEAVY.CONTACTS should work well from a consumer.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

This is a good first bug, but it would require a device for testing.

Comment 2

[Dave Hunt [:davehunt] [he/him] ⌚BST]

The changes would likely need to be here: https://github.com/mozilla/b2gpopulate/blob/3f7053d3340d00d8a53b6b4f98a050598f663cef/b2gpopulate/b2gpopulate.py#L50 though we would probably rework the DB_PRESET_COUNTS to use the new values so we're not duplicating these values.

Comment 3

[William Lachance (:wlach)]

Gautam, this might be a good bug for you to work on as it'll expose you to a few components eideticker uses. Please let either me or Dave Hunt know if you have questions.

Comment 4

[William Lachance (:wlach)]

So Dave might have other suggestions, I think I might prefer an approach where we define the following constants in b2gpopulate then export them:

LIGHT_WORKLOAD = { 'contacts': 200, 'music': 20, ... }
MEDIUM_WORKLOAD = { 'contacts': 500, 'music': 50, ... }
HEAVY_WORKLOAD = { 'contacts': 1000, 'music': 100, ... }

The consumers could just get quantities by doing something like

import b2gpopulate

num_contacts_to_use_in_test = MEDIUM_WORKLOAD['contacts']
...

Does that make sense?

Comment 5

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #4)
> num_contacts_to_use_in_test = MEDIUM_WORKLOAD['contacts']
> ...

Ooops, meant to say: num_contacts_to_use_in_test = b2gpopulate.MEDIUM_WORKLOAD['contacts']

Comment 6

[Gautam Akiwate [:gakiwate]]

Attachment: Bug-1040705-b2gpopulate-Provide-workload-quantities-.patch

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8458973 [details] [diff] [review]
Bug-1040705-b2gpopulate-Provide-workload-quantities-.patch

Review of attachment 8458973 [details] [diff] [review]:
-----------------------------------------------------------------

This is nice, I particularly like the idea of being able to specify the workload name on the command line. There's a bit of cleanup to do here, but I think it's looking great so far.

::: b2gpopulate/b2gpopulate.py
@@ +26,4 @@
>      'event': [0, 900, 1300, 2400, 3200],
>      'message': [0, 200, 500, 1000, 2000]}
>  
> +WORKLOAD = {

Can we call this WORKLOADS as there are multiple.

@@ +26,5 @@
>      'event': [0, 900, 1300, 2400, 3200],
>      'message': [0, 200, 500, 1000, 2000]}
>  
> +WORKLOAD = {
> +    'empty': { 'calls': 0, 'contacts': 0, 'messages': 0, 'music': 0, 'pictures': 0, 'videos':0, 'events':0 },

Remove space after opening { and before closing }, and keep a space after each : for PEP 8. Also, can we keep the line length shorter? I find the following style much easier to read:

WORKLOADS = {
    'empty': {
        'calls': 0,
        'contacts': 0,
        'messages': 0,
        'music': 0,
        'pictures': 0,
        'videos': 0,
        'events': 0},

@@ +82,4 @@
>  
>      def populate(self, call_count=None, contact_count=None, message_count=None,
>                   music_count=None, picture_count=None, video_count=None,
> +                 event_count=None, workload='empty'):

I like the idea of being able to provide a workload on the command line, but I think we should resolve the quantities before we run populate.

@@ +92,5 @@
>  
>          if call_count is not None:
>              self.populate_calls(call_count, restart=False)
> +        else:
> +            self.populate_calls(WORKLOAD[workload]['calls'], restart=False)

For this, and all the others, we should just use the count passed and extract it from the dictionary before we call populate.

@@ +467,5 @@
> +    parser.add_option(
> +        '--workload',
> +        action='store',
> +        dest='workload',
> +        metavar='string',

Elsewhere we've used 'str' to indicate a string.

@@ +468,5 @@
> +        '--workload',
> +        action='store',
> +        dest='workload',
> +        metavar='string',
> +        default='light',

We should remove the default here so that users can choose to specify individual counts or a workload.

@@ +491,5 @@
> +                  data_type in data_types]
> +        if not len([count for count in counts if count >= 0]) > 0:
> +            parser.print_usage()
> +            print 'Must specify at least one item to populate'
> +            parser.exit()

I think for simplicity we should make specifying a workload and custom counts mutually exclusive. This would mean we should throw an error if a user passes both --contacts=500 and --workload=light for example.

@@ +496,3 @@
>  
>      # only allow preset db values for calls and messages
>      for data_type in DB_PRESET_TYPES:

The DB_PRESET_TYPES and DB_PRESET_COUNTS are superseded by the WORKLOADS, so we should remove them.

Comment 8

[Gautam Akiwate [:gakiwate]]

(In reply to Dave Hunt (:davehunt) from comment #7)
> Comment on attachment 8458973 [details] [diff] [review]
> Bug-1040705-b2gpopulate-Provide-workload-quantities-.patch
> 
> Review of attachment 8458973 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is nice, I particularly like the idea of being able to specify the
> workload name on the command line. There's a bit of cleanup to do here, but
> I think it's looking great so far.
> 
> ::: b2gpopulate/b2gpopulate.py
> @@ +26,4 @@
> >      'event': [0, 900, 1300, 2400, 3200],
> >      'message': [0, 200, 500, 1000, 2000]}
> >  
> > +WORKLOAD = {
> 
> Can we call this WORKLOADS as there are multiple.
> 
> @@ +26,5 @@
> >      'event': [0, 900, 1300, 2400, 3200],
> >      'message': [0, 200, 500, 1000, 2000]}
> >  
> > +WORKLOAD = {
> > +    'empty': { 'calls': 0, 'contacts': 0, 'messages': 0, 'music': 0, 'pictures': 0, 'videos':0, 'events':0 },
> 
> Remove space after opening { and before closing }, and keep a space after
> each : for PEP 8. Also, can we keep the line length shorter? I find the
> following style much easier to read:
> 
> WORKLOADS = {
>     'empty': {
>         'calls': 0,
>         'contacts': 0,
>         'messages': 0,
>         'music': 0,
>         'pictures': 0,
>         'videos': 0,
>         'events': 0},
> 
> @@ +82,4 @@
> >  
> >      def populate(self, call_count=None, contact_count=None, message_count=None,
> >                   music_count=None, picture_count=None, video_count=None,
> > +                 event_count=None, workload='empty'):
> 
> I like the idea of being able to provide a workload on the command line, but
> I think we should resolve the quantities before we run populate.
> 
> @@ +92,5 @@
> >  
> >          if call_count is not None:
> >              self.populate_calls(call_count, restart=False)
> > +        else:
> > +            self.populate_calls(WORKLOAD[workload]['calls'], restart=False)
> 
> For this, and all the others, we should just use the count passed and
> extract it from the dictionary before we call populate.
> 
> @@ +467,5 @@
> > +    parser.add_option(
> > +        '--workload',
> > +        action='store',
> > +        dest='workload',
> > +        metavar='string',
> 
> Elsewhere we've used 'str' to indicate a string.
> 
> @@ +468,5 @@
> > +        '--workload',
> > +        action='store',
> > +        dest='workload',
> > +        metavar='string',
> > +        default='light',
> 
> We should remove the default here so that users can choose to specify
> individual counts or a workload.
> 
> @@ +491,5 @@
> > +                  data_type in data_types]
> > +        if not len([count for count in counts if count >= 0]) > 0:
> > +            parser.print_usage()
> > +            print 'Must specify at least one item to populate'
> > +            parser.exit()
> 
> I think for simplicity we should make specifying a workload and custom
> counts mutually exclusive. This would mean we should throw an error if a
> user passes both --contacts=500 and --workload=light for example.
> 
I don't think it would be much harder to not have it mutually exclusive. Wouldn't allowing for specific overrides allow for more flexibility?

> @@ +496,3 @@
> >  
> >      # only allow preset db values for calls and messages
> >      for data_type in DB_PRESET_TYPES:
> 
> The DB_PRESET_TYPES and DB_PRESET_COUNTS are superseded by the WORKLOADS, so
> we should remove them.

I am not exactly sure what I should remove here?

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Gautam Akiwate [:gakiwate] from comment #8)
> I don't think it would be much harder to not have it mutually exclusive.
> Wouldn't allowing for specific overrides allow for more flexibility?

I agree that it wouldn't be difficult to implement, however I feel that it would make the command line usage confusing to users.

> > @@ +496,3 @@
> > >  
> > >      # only allow preset db values for calls and messages
> > >      for data_type in DB_PRESET_TYPES:
> > 
> > The DB_PRESET_TYPES and DB_PRESET_COUNTS are superseded by the WORKLOADS, so
> > we should remove them.
> 
> I am not exactly sure what I should remove here?

We should replace usage of DB_PRESET_TYPES and DB_PRESET_COUNTS with the new WORKSPACES constant. Currently these values are duplicated.

Comment 10

[Gautam Akiwate [:gakiwate]]

Attachment: Bug-1040705-b2gpopulate-Provide-workload-quantities-.patch

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8460386 [details] [diff] [review]
Bug-1040705-b2gpopulate-Provide-workload-quantities-.patch

Review of attachment 8460386 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good! Most of my comments are about conforming to the PEP 8 style guide. You may want to run pep8 over your code to ensure you conform before submitting the next version of your patch.

::: b2gpopulate/b2gpopulate.py
@@ +19,5 @@
>  from gaiatest import GaiaData
>  from gaiatest import GaiaDevice
>  
> +WORKLOADS = {
> +    'empty': {

b2gpopulate doesn't currently handle 0 very well, I'll raise this as a separate bug though.

@@ +25,5 @@
> +        'contact': 0,
> +        'message': 0,
> +        'music': 0,
> +        'picture': 0,
> +        'video':0,

Nit: Add a single space after the : (applies throughout)

@@ +26,5 @@
> +        'message': 0,
> +        'music': 0,
> +        'picture': 0,
> +        'video':0,
> +        'event':0 },

Nit: Remove the space before the closing } (applies throughout)

@@ +64,1 @@
>  

Nit: Add a blank line

@@ +79,4 @@
>      def __init__(self, data_type):
>          Exception.__init__(
>              self, 'Invalid value for %s count, use one of: %s' % (
> +                data_type, [WORKLOADS[k][data_type] for k in WORKLOADS.keys()]))

Nit: Line exceeds 80 characters. You could move the datatype to the line above, which would resolve this.

@@ +432,4 @@
>          dest='call_count',
>          metavar='int',
>          help='number of calls to create. must be one of: %s' %
> +            [WORKLOADS[k]['call'] for k in WORKLOADS.keys()])

Nit: Increase indent by one space.

@@ +456,4 @@
>          dest='message_count',
>          metavar='int',
>          help='number of messages to create. must be one of: %s' %
> +            [WORKLOADS[k]['message'] for k in WORKLOADS.keys()])

Nit: Increase indent by one space.

@@ +482,5 @@
> +        '--workload',
> +        action='store',
> +        dest='workload',
> +        metavar='str',
> +        choices = WORKLOADS.keys(),

Nit: Remove spaces around =

@@ +483,5 @@
> +        action='store',
> +        dest='workload',
> +        metavar='str',
> +        choices = WORKLOADS.keys(),
> +        help='type of workload to create. must be one of: %s' %

This is displaying the following for me: "type of workload to create. must be one of: ['heavy', 'light', 'medium', 'empty', 'x-heavy']" could we output the keys in ascending order based on the relative sizes?

@@ +484,5 @@
> +        dest='workload',
> +        metavar='str',
> +        choices = WORKLOADS.keys(),
> +        help='type of workload to create. must be one of: %s' %
> +            WORKLOADS.keys())

Nit: Increase indent by one space.

@@ +499,4 @@
>      counts = [getattr(options, '%s_count' % data_type) for
>                data_type in data_types]
>      if not len([count for count in counts if count >= 0]) > 0:
> +        if getattr(options, 'workload') == None:

You should be able to just use:

    if options.workload is None:

@@ +503,5 @@
> +            parser.print_usage()
> +            print 'Must specify at least one item to populate'
> +            parser.exit()
> +    else:
> +        if getattr(options, 'workload') != None:

Similarly, this could be:

    if options.workload is not None:

@@ +505,5 @@
> +            parser.exit()
> +    else:
> +        if getattr(options, 'workload') != None:
> +            parser.print_usage()
> +            print 'Cannot use workload presets with manual loads'

I would suggest "Please specify either a workload or individual values."

@@ +511,2 @@
>  
>      # only allow preset db values for calls and messages

Let's update this comment as it's not just relevant to calls and messages. We can make it more generic.

@@ +517,1 @@
>              raise InvalidCountError(data_type)

This would limit all data types to the workload values. We should allow an arbitrary value for pictures, videos, and music.

@@ +527,5 @@
>                                log_level=options.log_level,
>                                start_timeout=options.start_timeout,
>                                device_serial=options.device_serial)
> +
> +    if getattr(options, 'workload') != None:

I think this should be: if options.workload is None

@@ +537,5 @@
> +            options.picture_count,
> +            options.video_count,
> +            options.event_count)
> +    else:
> +         b2gpopulate.populate(

Decrease indent by one space.

Comment 12

[Gautam Akiwate [:gakiwate]]

Attachment: Bug-1040705-b2gpopulate-Provide-workload-quantities.patch

Fixed the formatting. Made sure that the entire b2gpopulate file is pep8 compliant.

Also, incorpoarted the suggestions of order. The workload options are now put in ascending order.

Comment 13

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8462107 [details] [diff] [review]
Bug-1040705-b2gpopulate-Provide-workload-quantities.patch

Review of attachment 8462107 [details] [diff] [review]:
-----------------------------------------------------------------

Just one small change to make, then I think this will be ready to land!

::: b2gpopulate/b2gpopulate.py
@@ +513,3 @@
>  
> +    # only allow preset values for calls, messages, contacts, events
> +    for data_type in data_types:

You can simplify this by just listing the data types with database restrictions:

# only allow preset values for databases
for type in ['calls', 'contacts', 'events', 'messages']:
    if count and count not in [WORKLOADS[k][type] for k in WORKLOADS.keys()]:
        raise InvalidCountError(type)

Comment 14

[Gautam Akiwate [:gakiwate]]

Attachment: Bug-1040705-b2gpopulate-Provide-workload-quantities.patch

Comment 15

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8462660 [details] [diff] [review]
Bug-1040705-b2gpopulate-Provide-workload-quantities.patch

Review of attachment 8462660 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks! Landed in:
https://github.com/mozilla/b2gpopulate/commit/ca9e5370b25cfba16161f27387ecaa140d5ed0cc