Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MinMaxSumCountAggregator export to Prometheus; map to summary + min & max? #1172

Closed
plajjan opened this issue Sep 28, 2020 · 2 comments
Closed
Labels
1.10.0rc1 release candidate 1 for metrics GA exporters metrics release:required-for-ga To be resolved before GA release

Comments

@plajjan
Copy link
Contributor

plajjan commented Sep 28, 2020

This is a question / discussion issue. I am working in this area and would like to get feedback and direction before I write too much code.

After #945 we have working export of ValueRecorder that have a MinMaxSumCountAggregator attached. It maps the MinMaxSumCountAggregator to the SummaryMetricFamily in Prometheus, which gets us a _sum and _count metric in the end. That means we've covered two parts of our aggregator, AFAICT we simply don't map or transmit the min and max parts? Could and should we map those to individual GaugeMetricFamily?

Like, we now have this code:

        elif isinstance(metric_record.instrument, ValueRecorder):
            value = metric_record.aggregator.checkpoint
            if isinstance(metric_record.aggregator, MinMaxSumCountAggregator):
                prometheus_metric = SummaryMetricFamily(
                    name=metric_name,
                    documentation=description,
                    labels=label_keys,
                )
                prometheus_metric.add_metric(
                    labels=label_values,
                    count_value=value.count,
                    sum_value=value.sum,
                )
            else:
                prometheus_metric = UnknownMetricFamily(
                    name=metric_name,
                    documentation=description,
                    labels=label_keys,
                )
                prometheus_metric.add_metric(labels=label_values, value=value)

We could do something along the lines of:

        elif isinstance(metric_record.instrument, ValueRecorder):
            value = metric_record.aggregator.checkpoint
            if isinstance(metric_record.aggregator, MinMaxSumCountAggregator):
                # count and sum mapped to summary
                sum_metric = SummaryMetricFamily(
                    name=metric_name,
                    documentation=description,
                    labels=label_keys,
                )
                sum_metric.add_metric(
                    labels=label_values,
                    count_value=value.count,
                    sum_value=value.sum,
                )
                # min and max mapped to individual gauges
                min_metric = GaugeMetricFamily(
                    name=metric_name + "_min",
                    documentation=description,
                    labels=label_keys
                )
                min_metric.add_metric(
                    labels=label_values, value=value.min
                )
                max_metric = GaugeMetricFamily(
                    name=metric_name + "_max",
                    documentation=description,
                    labels=label_keys
                )
                max_metric.add_metric(
                    labels=label_values, value=value.max
                )
                logger.info(f"FLONK {metric_name} {label_values[0]} max: {value.max}")
                prometheus_metric = [sum_metric, min_metric, max_metric]

There is no direct match in Prometheus for a MinMaxSumCount, so is this the next best thing we could and should do?

There are associated changes elsewhere, like we need to return a list of metrics since we do a 1:N map and not 1:1 like before, but that's all manageable I think.

I'm sort of new to the idea of these metric types and I would like to understand if the above is conceptually correct?

@cbrand given that you wrote #945, your feedback here would be much appreciated!

@plajjan
Copy link
Contributor Author

plajjan commented Sep 28, 2020

I tried implementing the above, which works (as mentioned requires a few changes in other places).

However, the result isn't entirely what I imagined. I figured MinMaxSumCount (MMSC) would keep track of min and max within a poll cycle, not be a total min / max across all of time. Perhaps I'm just not getting how these are supposed to be used, but isn't this useless? At least in the export to a TSDB, since the TSDB itself can keep track of min / max over time. Is MMSC also meant to work as some form of bare bones TSDB within a system, so we could easily look at global min / max?

I also read open-telemetry/opentelemetry-specification#636 but can't say I now have a better grip of what we should do here.

If it is working as designed, perhaps this just isn't the aggregator I want. I'm considering whether HistogramAggregator is a better fit for me. No support in the prometheus exporter for histograms though, so that's another project...

@lzchen
Copy link
Contributor

lzchen commented Oct 2, 2020

I figured MinMaxSumCount (MMSC) would keep track of min and max within a poll cycle

I believe you can get the min/max within a poll cycle by setting stateful to be false. Although since the metrics SDK specs are still WIP, there might be significant changes in how we represent statefulness.

@lzchen lzchen added metrics sdk Affects the SDK package. exporters and removed sdk Affects the SDK package. labels Nov 9, 2020
@codeboten codeboten added release:required-for-ga To be resolved before GA release 1.10.0rc1 release candidate 1 for metrics GA labels Nov 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.10.0rc1 release candidate 1 for metrics GA exporters metrics release:required-for-ga To be resolved before GA release
Projects
None yet
Development

No branches or pull requests

3 participants