Re: [PATCH v2 1/3] qmp: Support for querying stats

[Paolo Bonzini <pbonzini@redhat.com>]

On 12/7/21 19:42, Daniel P. Berrangé wrote:
> Now onto the values being reported. AFAICT from the kernel
> docs, for all the types of data it currently reports
> (cumulative, instant, peak, none), there is only ever going
> to be a single value. I assume the ability to report multiple
> values is future proofing for a later requirement.

Yes, in fact histogram values have since been added.

> Second, for a given named statistic, AFAICT, the data type,
> unit, base and exponent are all fixed. I don't see a reason
> for us to be reporting that information every time we call
> 'query-stats'. Just report the name + value(s).  Apps that
> want a specific named stat won't care about the dimensions,
> because they'll already know what the value means.

I agree on this.

> The 'name' and 'type' are used for filtering I presume. Only allowing
> a single value for each feels pretty inflexible. I'd say we want to
> allow mutliple requests at a time for efficiency.
> 
> Bearing in mind my other suggestions above, I'd think we should have
> something  more like
> 
>   { 'enum': 'StatsProvider',
>     'data': ["kvm", "qemu", "tcg", ....],
>   }
> 
>   { 'struct': 'StatsRequest',
>     'data': {
>        'provider': 'StatsProvider',
>        // If omitted, report everything for this provider
>        '*fields': [ 'str' ]

I think provider should be optional as well.  See below.

>     }
>   }
> 
>   { 'struct': 'StatsVCPURequest',
>     'base': 'StatsRequest',
>     'data': {
>       // To request subset of vCPUs e.g.
>       //  "cpu_set": "0-3"
>       // Could use ['int'] instead if desired
>       '*cpu_set': str,

Yes, ['int'] is preferrable.

>     },
>   }
> 
>   { 'struct': 'StatsFilter',
>     'data': {
>       // If omitted means don't report that group of data
>       '*vcpu': 'StatsVCPURequest',
>       '*vm': 'StatsRequest',
>     },
>   }

I agree except that I think this and StatsResults should be unions, even 
if it means running multiple query-stats commands.  There should also be 
an enum ['vcpu', 'vm'] that acts as the discriminator for both 
StatsFilter and StatsResults:

  { 'enum': 'StatsTarget',
    'data': [ 'vcpu', 'vm' ] }

  { 'union': 'StatsFilter',
    'base': { 'target': 'StatsTarget', '*providers': ['StatsProvider'] },
    'discriminator': 'target',
    'data': { 'vcpu': ['*cpu-set': ['int']] }
}

  { 'union': 'StatsResults',
    'base': { 'target': 'StatsTarget', stats: ['StatsResultsEntry'] },
    'discriminator': 'target',
    'data': { 'vcpu': ['id': 'int'] }
}

Alternatively, the providers should simply be keys in the dictionary

Paolo

> 
>   { 'alternate': 'StatsValue',
>     'data': { 'scalar': 'int64',
>               'list': [ 'int64 ' ] }
>   }
> 
>   { 'struct': 'StatsResultsEntry',
>     'data': {
>       'provider': 'StatsProvider',
>       'stats': [ 'StatsValue' ]
>     }
>   }
> 
>   { 'struct': 'StatsResults':
>     'data': {
>       '*vcpu': [ [ 'StatsResultsEntry' ] ],
>       '*vm': [ 'StatsResultsEntry' ]
>     }
>   }
> 
>   { 'command': 'query-stats',
>     'data': { 'filter': '*StatsFilter' },
>     'returns': 'StatsResults' }