Exporter/Prometheus: Convert None label values to empty string.#486
Conversation
| metric_name = desc['name'] | ||
| metric_description = desc['documentation'] | ||
| label_keys = desc['labels'] | ||
| tag_values = list(map(cast_none_to_empty_str, tag_values)) |
There was a problem hiding this comment.
Let's add a comment above this code to say
# Prometheus requires that all tag values be strings hence
# the need to cast none to the empty string before exporting.
# See https://github.com/census-instrumentation/opencensus-python/issues/480
tag_values = list(map(cast_none_to_empty_str, tag_values)There was a problem hiding this comment.
SG, done. Also fixed a bug in histogram buckets.
There was a problem hiding this comment.
Nit, but I think it's a bit more readable and idiomatic to do:
tag_values = [tv for tv in tag_values if tv else ""]There was a problem hiding this comment.
And why wrap the comment at <79 chars?
There was a problem hiding this comment.
Updated, thanks for the suggestion.
And why wrap the comment at <79 chars?
Test session "lint" checks line length and will fail on lines with >80 chars, including comments.
There was a problem hiding this comment.
I mean why not wrap at the last word boundary before 79, the line is wrapped early. Not a big deal in any case.
| cum_count += agg_data.counts_per_bucket[ii] | ||
| points[str(bound)] = cum_count | ||
| # Prometheus requires buckets to be sorted, and +Inf present. | ||
| # In OpenCensus we don't have +Inf in the bucket bonds so need to |
There was a problem hiding this comment.
| # Prometheus requires buckets to be sorted, and +Inf present. | ||
| # In OpenCensus we don't have +Inf in the bucket bonds so need to | ||
| # append it here. | ||
| points["+Inf"] = agg_data.count_data |
There was a problem hiding this comment.
What's this bug fix in here? A test demonstrating the problem would be useful. I thought this code runs through the Prometheus Python client first before being exported? I think it would be good for us to file and diagnose that bug separately than sneak in a bug fix here without figuring out its extent.
There was a problem hiding this comment.
See https://github.com/prometheus/client_python/blob/master/prometheus_client/metrics_core.py#L204.
I've removed this commit from this PR for now.
songy23
force-pushed
the
prometheus-none-label
branch
from
c4e41c6 to
003da06
Compare
odeke-em
left a comment
odeke-em
left a comment
There was a problem hiding this comment.
LGTM, please squash the commits and we are good to go. Please file a separate bug for the +Inf inclusion into exported buckets.
songy23
force-pushed
the
prometheus-none-label
branch
from
003da06 to
cdc4cae
Compare
songy23
force-pushed
the
prometheus-none-label
branch
from
cdc4cae to
cc61de8
Compare
| metric_name = desc['name'] | ||
| metric_description = desc['documentation'] | ||
| label_keys = desc['labels'] | ||
| tag_values = list(map(cast_none_to_empty_str, tag_values)) |
There was a problem hiding this comment.
Nit, but I think it's a bit more readable and idiomatic to do:
tag_values = [tv for tv in tag_values if tv else ""]| metric_name = desc['name'] | ||
| metric_description = desc['documentation'] | ||
| label_keys = desc['labels'] | ||
| tag_values = list(map(cast_none_to_empty_str, tag_values)) |
There was a problem hiding this comment.
And why wrap the comment at <79 chars?
|
Thanks for reviewing! |
4 participants
Fixes #480.
/cc @PikBot