Exporter/Prometheus: Upgrade version and fix buckets and labels.#492
Exporter/Prometheus: Upgrade version and fix buckets and labels.#492songy23 merged 7 commits intocensus-instrumentation:masterfrom
Conversation
| contents = urllib2.urlopen("http://localhost:9303/metrics").read() | ||
|
|
||
| self.assertIn(b'# TYPE opencensus_request_count_view counter', | ||
| self.assertIn(b'# TYPE opencensus_request_count_view_total counter', |
There was a problem hiding this comment.
The latest Prometheus client automatically append "_total" suffix to count metrics.
| metric_description = desc['documentation'] | ||
| label_keys = desc['labels'] | ||
|
|
||
| assert(len(tag_values) == len(label_keys)) |
There was a problem hiding this comment.
This is unfortunately going to panic in user code, am not sure if that's something we want?
Sure it demos a bug in the library but redeploying versions of an entire stack will be painful.
Perhaps if we suspect such problems, we can pad or truncate tag values depending on label_keys
Perhaps let's make a TODO for an action item later on and think about it.
There was a problem hiding this comment.
This is unfortunately going to panic in user code, am not sure if that's something we want?
No this won't. to_metric() should only be called within our library and users are not expected to call it directly. If for some reason the label keys and values don't match, that indicates an internal error in our library.
In Java we have the exact same check: https://github.com/census-instrumentation/opencensus-java/blob/master/exporters/stats/prometheus/src/main/java/io/opencensus/exporter/stats/prometheus/PrometheusExportUtils.java#L143-L144.
| assert(agg_data.bounds == sorted(agg_data.bounds)) | ||
| points = {} | ||
| cum_count = 0 | ||
| # buckets are a list of bucket. Each bucket is another list with |
There was a problem hiding this comment.
s/list of bucket./list of buckets./g
| for ii, bound in enumerate(agg_data.bounds): | ||
| cum_count += agg_data.counts_per_bucket[ii] | ||
| points[str(bound)] = cum_count | ||
| bucket = [str(bound), cum_count] |
There was a problem hiding this comment.
Using lists is kind of overkill and potentially expensive.
Why not tuples that properly match the grouping we are using of pairs, or triples?
cur = (str(bound), cum_count,)
buckets = append(cur)There was a problem hiding this comment.
Used list because Prometheus client comments said explicitly they expected a list of lists as buckets (https://github.com/prometheus/client_python/blob/master/prometheus_client/metrics_core.py#L201):
buckets: A list of lists. Each inner list can be a pair of bucket name and value, or a triple of bucket name, value, and exemplar. The buckets must be sorted, and +Inf present.
Guess tuples will work in the same way. Updated.
There was a problem hiding this comment.
@songy23 oh I see, I didn't know that, but perhaps please cite that in your commit. My apologies, please revert to the original that you had as I was assuming that you had just used list of list out of convenience.
There was a problem hiding this comment.
No worries - changed it back to lists.
| elif isinstance(agg_data, | ||
| aggregation_data_module.SumAggregationDataFloat): | ||
| metric = UntypedMetricFamily(name=metric_name, | ||
| metric = UnknownMetricFamily(name=metric_name, |
There was a problem hiding this comment.
What is the SummaryMetricFamily then?
There was a problem hiding this comment.
SummaryMetricFamily is for Summary metrics. Sum metrics should be converted to UnknownMetricFamily.
de3947b to
be8dbf3
Compare
be8dbf3 to
fd2f7fd
Compare
Good catch! I'm surprised to see the bounds need to be sorted but it looks to be the case. Changing the prometheus client version seems significant and user-facing enough to be worth adding a note to the changelog. What do you think? |
Make sense to me, added to changelog. |
UnknownMetricFamilyinstead ofUntypedMetricFamily, as the latter is now deprecated.