Re: [PATCH] libtraceeval: Add traceeval_stat_max/min_timestamp() API

[Ross Zwisler <zwisler@google.com>]

On Thu, Oct 05, 2023 at 06:14:48PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> If a value field is flagged as a TIMESTAMP, and another field is flagged
> as a STAT, have the statistics for the STAT field automatically keep track
> of when the max and min happened (what the TIMESTAMP field was at those
> instances).

I'm confused as to why we have TRACEEVAL_FL_TIMESTAMP and a separate data
entry with that flag, instead of only storing the timestamp in the entry
metadata, as we do with the other STATs?

From the description above where we have one TIMESTAMP and one STAT, I would
expect to see structures defined like this
(from [PATCH v2] libtraceeval: Add wake-lat sample code):

+struct traceeval_type sched_vals[] = {
+       {
+               .name           = "timestamp",
+               .flags          = TRACEEVAL_FL_TIMESTAMP,
+               .type           = TRACEEVAL_TYPE_NUMBER_64,
+       },
+       {
+               .name           = "delta",
+               .flags          = TRACEEVAL_FL_STAT,
+               .type           = TRACEEVAL_TYPE_NUMBER_64,
+       }

where the timestamp is sync'd with the STAT min and max, right?

But we also have this:

+struct traceeval_type wakeup_vals[] = {
+       {
+               .name           = "timestamp",
+               .flags          = TRACEEVAL_FL_TIMESTAMP,
+               .type           = TRACEEVAL_TYPE_NUMBER_64,
+       }
+};

in a structure that doesn't have a corresponding STAT field?  This is also
confusing to me because we need to keep 2 values (min and max TS), but this
only holds one?

The timestamp is already stored independently in the stat metadata in
struct traceeval_stat:

>  struct traceeval_stat {
>  	unsigned long long	max;
> +	unsigned long long	max_ts;
>  	unsigned long long	min;
> +	unsigned long long	min_ts;
>  	unsigned long long	total;
>  	unsigned long long	std;
>  	size_t			count;

which I think is enough?

It seems like instead we really want our two flags to be:

TRACEEVAL_FL_STAT
and
TRACEEVAL_FL_STAT_TS

where the first just keeps the normal stats (min, max, average, std. dev, etc)
and the second does all that plus timestamps for min and max?

This would also allow you to keep min and max stats independently for multiple
entries in a single structure, i.e.:

struct traceeval_type task_vals[] = {
       {
               .name           = "wake_delay",
               .flags          = TRACEEVAL_FL_STAT_TS,
               .type           = TRACEEVAL_TYPE_NUMBER_64,
       },
       {
               .name           = "runtime",
               .flags          = TRACEEVAL_FL_STAT_TS,
               .type           = TRACEEVAL_TYPE_NUMBER_64,
       }
};

which I don't think is possible under the current scheme?

What am I missing?