fix(ext/prometheus): support minmaxsumcount aggregator#945
Conversation
|
I seem to be unable to get with classes in a type annotation from an external library sphinx to build without a warning. Thus, b71d498 removes the Iterable and Union statements which include objects of the |
| labels=label_values, value=metric_record.aggregator.checkpoint | ||
| ) | ||
| value = metric_record.aggregator.checkpoint | ||
| if isinstance(value, MinMaxSumCountAggregator._TYPE): # pylint: disable=W0212 |
There was a problem hiding this comment.
Nit: Compare the aggregator itself instead of the value?
isinstance(metric_record.aggregator, MinMaxSumCountAggregator)
| if isinstance( | ||
| value, MinMaxSumCountAggregator._TYPE # pylint: disable=W0212 | ||
| ): | ||
| value = metric_record.aggregator.checkpoint |
There was a problem hiding this comment.
Seems like value is defined twice? Line 165
instead of value
| metric_name += self._sanitize(metric_record.instrument.name) | ||
|
|
||
| description = getattr(metric_record.instrument, "description", "") | ||
| if isinstance(metric_record.instrument, Counter): |
There was a problem hiding this comment.
Curious, is does prometheus not support any other metric type? What about UpDownCounter?
There was a problem hiding this comment.
Prometheus does actually provide a lot of other metric types:
Following the README at: https://github.com/prometheus/client_python#instrumenting the following types are supported:
I am not quite sure why the initial implementation only included unknown data sets and counters.
Maybe it was just enough for the use case of the contributor of this package.
|
I'm seeing this problem too. Any particular reason why PR isn't merged yet? It was approved but never merged... CI is currently failing, does the patch perhaps need to be rebased? |
|
From last time I looked into it it seems that the build is failing due to deviations in the master branch (mainly one removed functionality from the metrics package). I pushed a fix up which seems to work with the new master base branch as expected. @lzchen please take a look. |
|
@lzchen I meant that I see the bug that this PR fixes, so I am interested in getting it resolved. Looks like CI is passing now after @cbrand applied his fix (thanks @cbrand for the quick turnaround). The branch is approved since earlier, perhaps a quick check on the newer changes is warranted but otherwise, good to merge? |
4 participants
This PR fixes the export of
MinMaxSumCountAggregatoraggregated metrics in Prometheus.Before this PR, the specified test produces the following exception:
To fix this, the
_translate_to_prometheusfunction checks if the metric's checkpoint value is a minmaxsumcount namedtuple instance and creates aSummaryMetricFamilyobject of it as this seems to be the best possible supported Prometheus Export type, thus dropping recording of the min and max entries, but persisting thesumandcountcalculations.Additionally, this PR adds types to all functions within this package. Some unused imports have also been removed.