-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Drop protobuf support for OpenMetrics #2098
Conversation
4909086
to
2ea956f
Compare
# message.type is the index in this array | ||
# see: https://github.com/prometheus/client_model/blob/master/ruby/lib/prometheus/client/model/metrics.pb.rb | ||
self.METRIC_TYPES = ['counter', 'gauge', 'summary', 'untyped', 'histogram'] | ||
self.METRIC_TYPES = ['counter', 'gauge', 'summary', 'histogram'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just make this a class constant?
custom_hostname = self._get_hostname(hostname, sample, scraper_config) | ||
tags = self._metric_tags(metric_name, val, sample, scraper_config, hostname=custom_hostname) | ||
if sample[self.SAMPLE_NAME].endswith("_sum"): | ||
self.gauge("{}.{}.sum".format(scraper_config['namespace'], metric_name), val, tags=tags, hostname=custom_hostname) | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we still collecting {}.{}.quantile
metrics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it should work, I put the quantile in the .count
like the brackets and histograms, didn't pay attention it was separated for summary, I'll change this
if label_name not in scraper_config['exclude_labels']: | ||
tag_name = label_name | ||
if label_name in scraper_config['labels_mapper']: | ||
tag_name = scraper_config['labels_mapper'][label_name] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's simplify this to tag_name = scraper_config['labels_mapper'].get(label_name, label_name)
while we're here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's probably some other improvement like this lying around with labels being a dict
, I focused on making it work first 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm trying to find some more hehe 😄
We might as well improve this class while we can!
self.gauge("{}.{}.sum".format(scraper_config['namespace'], metric_name), val, tags=tags, hostname=custom_hostname) | ||
elif sample[self.SAMPLE_NAME].endswith("_count"): | ||
self.gauge("{}.{}.count".format(scraper_config['namespace'], metric_name), val, tags=tags, hostname=custom_hostname) | ||
elif sample[self.SAMPLE_NAME].endswith("_bucket") and scraper_config['send_histograms_buckets']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably just merge these two conditionals into:
elif sample[self.SAMPLE_NAME].endswith("_count") or (sample[self.SAMPLE_NAME].endswith("_bucket") and scraper_config['send_histograms_buckets']):
8438763
to
1769441
Compare
|
||
def process_metric(self, message, scraper_config, metric_transformers=None): | ||
for sample in metric.samples: | ||
for label_name in scraper_config['_watched_labels'].intersection(set(sample[self.SAMPLE_LABELS].keys())): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With how many lookups we make to sample[self.SAMPLE_LABELS]
, we should probably just set it to a variable haha
And I think we can do this in multiple functions. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rishabh while I agree it'll be better for readability, I'm not sure about the memory footprint of a new variable containing labels.
the lookup for sample[self.SAMPLE_LABELS]
is O(1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, the footprint isn't a concern, it's just readability :)
for label in metric.label: | ||
if label.name == scraper_config['label_to_hostname']: | ||
return label.value | ||
if scraper_config['label_to_hostname'] in sample[self.SAMPLE_LABELS]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just merge these two conditions together.
8d0f587
to
12cb681
Compare
_met = _rate.metric.add() | ||
_met.gauge.value = 42 | ||
_rate = GaugeMetricFamily('my_rate', 'Random rate') | ||
_rate.add_metric([], 42) | ||
check = mocked_prometheus_check | ||
mocked_prometheus_scraper_config['rate_metrics'] = ["my_rate"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can get rid of this since rate_metrics
isn't a thing anymore.
12cb681
to
a11fdb8
Compare
a11fdb8
to
99f2a1d
Compare
* Drop protobuf support for OpenMetrics (text only) and optimize for text support
* Drop protobuf support for OpenMetrics (text only) and optimize for text support
Drop protobuf support for OpenMetrics (text only)
What does this PR do?
Motivation
Optimizing the text only support for open metrics
Review checklist
no-changelog
label attached