Re: [PATCH v2 10/17] libtraceeval histogram: Add updating of stats

[Ross Zwisler <zwisler@google.com>]

On Fri, Aug 11, 2023 at 01:39:33AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Whenever an entry is added that already exists (overwriting the values)
> keep track of the stats for the number values (max, min, total, count).
> 
> Also move the stat structure out of the public view. We may want to modify
> this structure in the future, and so it should not become an API.
> 
> Add accessor functions to get to the stat values.
> 
> Add traceeval_stat() to acquire a stat handle from a specific key for a
> specific value.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  include/traceeval-hist.h |  17 ++--
>  src/eval-local.h         |   9 ++
>  src/histograms.c         | 182 +++++++++++++++++++++++++++++++++++----
>  3 files changed, 183 insertions(+), 25 deletions(-)
> 
> diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
> index 1c02f3039809..d061a4532b06 100644
> --- a/include/traceeval-hist.h
> +++ b/include/traceeval-hist.h
> @@ -130,13 +130,7 @@ struct traceeval_type {
>  };
>  
>  /* Statistics about a given entry element */
> -struct traceeval_stat {
> -	unsigned long long	max;
> -	unsigned long long	min;
> -	unsigned long long	total;
> -	unsigned long long	avg;
> -	unsigned long long	std;
> -};
> +struct traceeval_stat;
>  
>  /* Iterator over aggregated data */
>  struct traceeval_iterator;
> @@ -160,4 +154,13 @@ int traceeval_query(struct traceeval *teval, const union traceeval_data *keys,
>  void traceeval_results_release(struct traceeval *teval,
>  			       union traceeval_data *results);
>  
> +struct traceeval_stat *traceeval_stat(struct traceeval *teval,
> +				      const union traceeval_data *keys,
> +				      struct traceeval_type *type);
> +
> +unsigned long long traceeval_stat_max(struct traceeval_stat *stat);
> +unsigned long long traceeval_stat_min(struct traceeval_stat *stat);
> +unsigned long long traceeval_stat_total(struct traceeval_stat *stat);
> +unsigned long long traceeval_stat_count(struct traceeval_stat *stat);
> +
>  #endif /* __LIBTRACEEVAL_HIST_H__ */
> diff --git a/src/eval-local.h b/src/eval-local.h
> index 820d7ad096e8..190b19db14d2 100644
> --- a/src/eval-local.h
> +++ b/src/eval-local.h
> @@ -45,11 +45,20 @@ struct hash_table {
>  	struct hash_iter	iter;
>  };
>  
> +struct traceeval_stat {
> +	unsigned long long	max;
> +	unsigned long long	min;
> +	unsigned long long	total;
> +	unsigned long long	std;
> +	size_t			count;
> +};
> +
>  /* A key-value pair */
>  struct entry {
>  	struct hash_item	hash;
>  	union traceeval_data	*keys;
>  	union traceeval_data	*vals;
> +	struct traceeval_stat	*val_stats;

I'm confused about why 'val_stats' is in 'struct entry', instead of in
'struct traceeval'?

I would think that we'd want to keep our stats on a per-histogram basis, where
we have an array of 'struct traceeval_stat' structs with the same size as our
'val_types' array, so that for a given compound value, say 

{ TRACEVAL_TYPE_NUMBER_64, TRACEEVAL_TYPE_NUMBER_16, TRACEEVAL_NUMBER_8}

we'd have stats for each of these entries, like min, max, average, etc.?

With the stats associated with each entry, I don't see how we keep all the
stats for all the entries up to date as we do insertions & removals.