[#674, #675] Mitigate performance regression by filtering restricted registry collections #680
Conversation
Signed-off-by: Pavel <pavel@lexyr.com>
pavel-lexyr
force-pushed
the
restricted_v2
branch
from
ea2b225 to
009d885
Compare
|
py2.7 passes locally. What do the logs say on the CI? |
You can use a skipIf and import the mock only for the test such as done in https://github.com/prometheus/client_python/blob/master/tests/openmetrics/test_parser.py#L492. |
Signed-off-by: Pavel <pavel@lexyr.com>
|
Aha. That worked - thank you! |
There was a problem hiding this comment.
This approach looks good to me, and I appreciate the test you added. I will give some time for @brian-brazil to review as well.
prometheus_client/registry.py
Outdated
| m = metric._restricted_metric(self._name_set) | ||
| if m: | ||
| yield m | ||
| names = copy.copy(self._name_set) |
There was a problem hiding this comment.
Why is this copy here? This set shouldn't be changing after the object is instantiated.
There was a problem hiding this comment.
Trying to do it in the same way collections used to be filtered with target_info. There used to be a set variable that was modified if target_info was found - in order not to change the class field in the same way, we copy it to names.
prometheus_client/registry.py
Outdated
| collectors = set() | ||
| with self._registry._lock: | ||
| if 'target_info' in names and self._registry._target_info: | ||
| yield self._registry._target_info_metric() |
There was a problem hiding this comment.
I don't think it is safe to yield under the lock.
There was a problem hiding this comment.
Fixed. Good catch!
Signed-off-by: Pavel <pavel@lexyr.com>
prometheus_client/registry.py
Outdated
| with self._registry._lock: | ||
| if 'target_info' in names and self._registry._target_info: | ||
| yield_target_info = True | ||
| names.remove('target_info') |
There was a problem hiding this comment.
You don't need this, the registry should be preventing this from happening in the first place.
There was a problem hiding this comment.
Not sure I understood it right - but done.
prometheus_client/registry.py
Outdated
| yield m | ||
| names = copy.copy(self._name_set) | ||
| collectors = set() | ||
| yield_target_info = False |
There was a problem hiding this comment.
I'd have this as the thing to yield, rather than a bool for cleanliness. Also avoids a race.
Signed-off-by: Pavel <pavel@lexyr.com>
|
👍 |
#675 introduces a significant performance regression in restricted registry collecting. This attempts to reduce the regression by re-introducing filter code removed in the previous version.