Re: [PATCH 1/5] histograms: initial histograms interface

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 31 Jul 2023 16:53:53 -0400
Stevie Alvarez <stevie.6strings@gmail.com> wrote:

> >   
> > > +enum traceeval_flags {
> > > +	TRACEEVAL_FL_SIGNED	= (1 << 0),
> > > +	TRACEEVAL_FL_STATS	= (1 << 1)  
> > 
> > As I think about this more, I think we should just do "stats" for all
> > numbered values. It doesn't really make sense for keys.  
> 
> So get rid of the "stats" flag altogether and do it by default for numbered
> values? And what do you mean when you say it doesn't make sense for keys? A
> key could be a numeric value too, but you might be meaning something else; I'm
> not quite following.

We don't need to keep the stats (max, min, total, count) for keys because
they are unique instances. Each instance should be the same. That is, the
stats are being kept for an instance of the histogram. And an instance is
defined by a unique key. If we keep stats on keys, the max and min will be
the same, and the total will just be max * hitcount.

I would also not keep stats if something is marked as a TIMESTAMP, as a
TIMESTAMP is just a place in time and not something that we need to keep
max min on. Hmm, I guess we could do it, and that would give us the time of
the first and last hit of that particular instance. Yeah, let's keep the
stats even on TIMESTAMPS. Doesn't hurt (even if total doesn't make sense).

> 
> > 
> > But we should still have TRACEEVAL_FL_TIMESTAMP (see traceeval_insert()
> > below)
> >   
> > > +};
> > > +
> > > +/**  
> > 

[..]

> >  * This function will insert an entry defined by @keys and @vals into
> >  * the traceeval instance. The array of @keys and @vals must match that
> >  * of what was added to the corresponding parameters of
> >  * traceeval_init() that created @teval. No checking is performed, if
> >  * there is a mismatch in array length, it will result in undefined behavior.
> >  * The types of the @keys and @vals must also match the types used for
> >  * the corresponding parameters to traceeval_init().
> >  *
> >  * If an entry already exists that matches the @keys, then strings and
> >  * values will be overwritten by the current values and the old values
> >  * will be discarded. For number values, the max, min, total and count
> >  * will be recorded. If an entry in @vals has TRACEEVAL_FL_TIMESTAMP
> >  * set in its corresponding traceeval_type descriptor, then it will be
> >  * used to timestamp the max and min values.  
> 
> So only values can be a timestamp? Or at least timestamp metrics can
> only be gathered for values? This goes back to my comment above about
> the flags.

Yeah, I think it only makes sense for a timestamp to be a value. As keys
are all shown, and we only keep track of a bit of a value. A timestamp is
used to know when a particular key was hit, so it doesn't make sense to be
a key.

-- Steve