flux-framework / fripp

Flux Realtime Instrumentation for Performance Probing
GNU Lesser General Public License v3.0
1 stars 3 forks source link

flux integration of FRIPP #15

Open garlick opened 3 years ago

garlick commented 3 years ago

As discussed on our call, we should think about how FRIPP instrumentation can be built into Flux.

One idea we discussed was to have a FRIPP context associated with each flux_t handle, if FRIPP is enabled via an environment variable. For example, maybe FLUX_FRIPP_STATSD=host:port could be used to both enable the instrumentation and configure the destination for the data. Then you could do something like:

$ FLUX_FRIPP_STATSD=localhost:8888 flux start

and get instrumentation for that flux instance.

As far as how that looks inside Flux code, I guess it could be something like

int flux_fripp_enable (flux_t *h, double aggregation_period);
void flux_fripp_disable (flux_t *h);

void flux_fripp_sendf (flux_t *h, const char *fmt, ...);

Then if the environment variable is not set, those are just no-ops.

For a start maybe we could just allow aggregation_period to be zero, so that each flux_fripp_sendf() immediately sends data to statsd. Later we could support > 0, create a timer, and queue up data until the timer expires.

Oh, I included the printf-style send function above, but I think we said in the call that we want something more abstract, so we should bounce that around here.

grondo commented 3 years ago

As far as how that looks inside Flux code, I guess it could be something like

Since the environment variable is used to globally enable/disable stats/metrics collection, maybe we could drop flux_fripp_enable() and have the fripp context created when the first metric is added to the handle? Then stats collection is available for any handle user at any time without requiring superfluous initialization calls. If no metrics are "pushed" into the handle, then the fripp context is never initialized.

For a start maybe we could just allow aggregation_period to be zero, so that each flux_fripp_sendf() immediately sends data to statsd. Later we could support > 0, create a timer, and queue up data until the timer expires.

Good idea, though for counter-type metrics that are sampled (e.g. the jobtap plugin that is sampling job states) this may not work out well (you would be sending a lot of unnecessary packets I think). Perhaps that is ok for a first step though. However, something to think about is that for the case of jobtap plugins, the flux handle and thus FRIPP context will be shared between the job manager module and any jobtap plugins. Because of this, there may be multiple different types of metrics being handled by this one FRIPP context. Therefore, I think a useable interface will need to actually perform some aggregation of different metrics within the FRIPP context.

Oh, I included the printf-style send function above, but I think we said in the call that we want something more abstract, so we should bounce that around here.

Considering that we may not want to be tied to the statsd format long term, I would suggest something that does not specifically require the statsd format in the API. I'm not sure of the best API at this point, but here's an example of a possible interface which uses a separate call per stats type:

This is largely taken from statsd-c-client since it is a working example of a C api for this kind of thing.

int flux_stats_inc (flux_t *h, const char *fmt, ...);
int flux_stats_dec (flux_t *h, const char *fmt, ...);
int flux_stats_count (flux_t *h, const char *name, size_t count);
int flux_stats_gauge (flux_t *h, const char *name, size_t value);
int flux_stats_timing (flux_t *h, const char *name, double ts);

statsd-c-client also includes a sample_rate parameter on each of its calls, but I don't understand the statsd sample rate enough to know if that is required or not. Ideally, since we're trying not to be statsd-specific here, the sample rate does not need to be included in the "public" api and can be inferred or set in the underlying FRIPP context. @garrettbslone, what do you think?

It would be nice if each flux_t handle got a namespace by default, but I'm not sure how to do that automatically.. so at first the "name" would have to manually include the fully namespaced name. I imagine this won't be an issue since that would be required with flux_fripp_printf() as well.

grondo commented 3 years ago

Take for example @garrettbslone's jobtap plugin to count job states. With the API above, we do not even have to have a locally instantiated context to save the "current" job state counts. Instead, the jobtap plugin would essentially boil down to something like (psuedocode):

    flux_plugin_arg_unpack (p, "{s:s s:s}", "state", &state, "prev_state", &prev_state);
    flux_stats_inc (h, "job.state.%s", state);
    flux_stats_dec (h, "job.state.%s", prev_state);
garlick commented 3 years ago

Really like those ideas @grondo.

garrettbslone commented 3 years ago

The sample_rate included in statsd-c-client api calls is an internal sample rate which shouldn't need to be included in the flux api.