-
Notifications
You must be signed in to change notification settings - Fork 802
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
[#674] Made the registry restriction handle newly added metrics. #675
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -94,28 +94,7 @@ def restricted_registry(self, names): | |
|
||
Experimental.""" | ||
names = set(names) | ||
collectors = set() | ||
metrics = [] | ||
with self._lock: | ||
if 'target_info' in names and self._target_info: | ||
metrics.append(self._target_info_metric()) | ||
names.remove('target_info') | ||
for name in names: | ||
if name in self._names_to_collectors: | ||
collectors.add(self._names_to_collectors[name]) | ||
for collector in collectors: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing this code has caused a serious performance regression. The whole point of this feature is not to collect from collectors that aren't needed, so that it's very cheap to access just one or two collectors. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, thank you for the context. @pavel-lexyr perhaps let's approach this by improving the documentation, would you be willing to open a PR to change this back but with more complete documentation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't need to undo the whole change (even though it is a behaviour change as to when it binds), but the collector lookup is important. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed your concerns in #680. Let me know if you find the implementation agreeable. |
||
for metric in collector.collect(): | ||
samples = [s for s in metric.samples if s[0] in names] | ||
if samples: | ||
m = Metric(metric.name, metric.documentation, metric.type) | ||
m.samples = samples | ||
metrics.append(m) | ||
|
||
class RestrictedRegistry(object): | ||
def collect(self): | ||
return metrics | ||
|
||
return RestrictedRegistry() | ||
return RestrictedRegistry(names, self) | ||
|
||
def set_target_info(self, labels): | ||
with self._lock: | ||
|
@@ -150,4 +129,16 @@ def get_sample_value(self, name, labels=None): | |
return None | ||
|
||
|
||
class RestrictedRegistry(object): | ||
def __init__(self, names, registry): | ||
self._name_set = set(names) | ||
self._registry = registry | ||
|
||
def collect(self): | ||
for metric in self._registry.collect(): | ||
m = metric.restricted_metric(self._name_set) | ||
if m: | ||
yield m | ||
|
||
|
||
REGISTRY = CollectorRegistry(auto_describe=True) |
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.
Do you think there is a good use case for using this function externally? If not perhaps it should be
_restricted_metric
so we could easily change/remove it later.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.
Fixed. Good catch!