Skip to content
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.

Sign up for GitHub

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

Support metric help text in multiprocess mode #804

Merged
merged 2 commits into from
Dec 22, 2022
Merged

Support metric help text in multiprocess mode #804

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f6a2b4a
Support metric help text in multiprocess mode
butlerx May 3, 2022
9a2c50f
Add test for help text
evgenymarkov Dec 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions prometheus_client/metrics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ def f():

def _metric_init(self) -> None:
self._value = values.ValueClass(self._type, self._name, self._name + '_total', self._labelnames,
self._labelvalues)
self._labelvalues, self._documentation)
self._created = time.time()

def inc(self, amount: float = 1, exemplar: Optional[Dict[str, str]] = None) -> None:
Expand Down Expand Up @@ -377,7 +377,7 @@ def __init__(self,
def _metric_init(self) -> None:
self._value = values.ValueClass(
self._type, self._name, self._name, self._labelnames, self._labelvalues,
multiprocess_mode=self._multiprocess_mode
self._documentation, multiprocess_mode=self._multiprocess_mode
)

def inc(self, amount: float = 1) -> None:
Expand Down Expand Up @@ -469,8 +469,8 @@ def create_response(request):

def _metric_init(self) -> None:
self._count = values.ValueClass(self._type, self._name, self._name + '_count', self._labelnames,
self._labelvalues)
self._sum = values.ValueClass(self._type, self._name, self._name + '_sum', self._labelnames, self._labelvalues)
self._labelvalues, self._documentation)
self._sum = values.ValueClass(self._type, self._name, self._name + '_sum', self._labelnames, self._labelvalues, self._documentation)
self._created = time.time()

def observe(self, amount: float) -> None:
Expand Down Expand Up @@ -583,14 +583,15 @@ def _metric_init(self) -> None:
self._buckets: List[values.ValueClass] = []
self._created = time.time()
bucket_labelnames = self._labelnames + ('le',)
self._sum = values.ValueClass(self._type, self._name, self._name + '_sum', self._labelnames, self._labelvalues)
self._sum = values.ValueClass(self._type, self._name, self._name + '_sum', self._labelnames, self._labelvalues, self._documentation)
for b in self._upper_bounds:
self._buckets.append(values.ValueClass(
self._type,
self._name,
self._name + '_bucket',
bucket_labelnames,
self._labelvalues + (floatToGoString(b),))
self._labelvalues + (floatToGoString(b),),
self._documentation)
)

def observe(self, amount: float, exemplar: Optional[Dict[str, str]] = None) -> None:
Expand Down
5 changes: 3 additions & 2 deletions prometheus_client/mmap_dict.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import mmap
import os
import struct
from typing import List

_INITIAL_MMAP_SIZE = 1 << 20
_pack_integer_func = struct.Struct(b'i').pack
Expand Down Expand Up @@ -137,8 +138,8 @@ def close(self):
self._f = None


def mmap_key(metric_name, name, labelnames, labelvalues):
def mmap_key(metric_name: str, name: str, labelnames: List[str], labelvalues: List[str], help_text: str) -> str:
"""Format a key for use in the mmap file."""
# ensure labels are in consistent order for identity
labels = dict(zip(labelnames, labelvalues))
return json.dumps([metric_name, name, labels], sort_keys=True)
return json.dumps([metric_name, name, labels, help_text], sort_keys=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I am trying to think of the ramifications of adding the help text to the mmap key. In the case where help text may change then the key would no longer work properly. I don't think there is a way to change help text though outside of a custom collector which is not supported in multiprocess mode anyway?

I guess the downside of this would be that if we do support custom collectors in multiprocess mode in the future help text might have to be ignored or redone?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if you wanted to add multiprocess support to the custom collectors you'd need to make the same sacrifice of the first write to help text win and ignore all future writes.

I struggle to think of any reason a metrics help text should change in a running application

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I agree, I applications generally should not change help text. The help text is also marked as a private field on the metric types now so shouldn't be changed by users of this library outside of custom collectors.

One thing I would love to see in this PR is either an additional test or some additional assertions in test_collect in test_multiprocess.py. I will let you know if I come up with other concerns in the meantime.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ive not had time to get back to this but looks like @evgenymarkov has written some tests in #866

Copy link
Contributor

@evgenymarkov evgenymarkov Dec 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry. I didn't find this pull request. You can cherry-pick 89e3193 and then I will close my PR.

10 changes: 4 additions & 6 deletions prometheus_client/multiprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
except NameError: # Python >= 2.5
FileNotFoundError = IOError

MP_METRIC_HELP = 'Multiprocess metric'


class MultiProcessCollector:
"""Collector for files for multi-process mode."""
Expand Down Expand Up @@ -53,9 +51,9 @@ def _read_metrics(files):
def _parse_key(key):
val = key_cache.get(key)
if not val:
metric_name, name, labels = json.loads(key)
metric_name, name, labels, help_text = json.loads(key)
labels_key = tuple(sorted(labels.items()))
val = key_cache[key] = (metric_name, name, labels, labels_key)
val = key_cache[key] = (metric_name, name, labels, labels_key, help_text)
return val

for f in files:
Expand All @@ -71,11 +69,11 @@ def _parse_key(key):
continue
raise
for key, value, _ in file_values:
metric_name, name, labels, labels_key = _parse_key(key)
metric_name, name, labels, labels_key, help_text = _parse_key(key)

metric = metrics.get(metric_name)
if metric is None:
metric = Metric(metric_name, MP_METRIC_HELP, typ)
metric = Metric(metric_name, help_text, typ)
metrics[metric_name] = metric

if typ == 'gauge':
Expand Down
10 changes: 5 additions & 5 deletions prometheus_client/values.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ class MutexValue:

_multiprocess = False

def __init__(self, typ, metric_name, name, labelnames, labelvalues, **kwargs):
def __init__(self, typ, metric_name, name, labelnames, labelvalues, help_text, **kwargs):
self._value = 0.0
self._exemplar = None
self._lock = Lock()
Expand Down Expand Up @@ -57,8 +57,8 @@ class MmapedValue:

_multiprocess = True

def __init__(self, typ, metric_name, name, labelnames, labelvalues, multiprocess_mode='', **kwargs):
self._params = typ, metric_name, name, labelnames, labelvalues, multiprocess_mode
def __init__(self, typ, metric_name, name, labelnames, labelvalues, help_text, multiprocess_mode='', **kwargs):
self._params = typ, metric_name, name, labelnames, labelvalues, help_text, multiprocess_mode
# This deprecation warning can go away in a few releases when removing the compatibility
if 'prometheus_multiproc_dir' in os.environ and 'PROMETHEUS_MULTIPROC_DIR' not in os.environ:
os.environ['PROMETHEUS_MULTIPROC_DIR'] = os.environ['prometheus_multiproc_dir']
Expand All @@ -69,7 +69,7 @@ def __init__(self, typ, metric_name, name, labelnames, labelvalues, multiprocess
values.append(self)

def __reset(self):
typ, metric_name, name, labelnames, labelvalues, multiprocess_mode = self._params
typ, metric_name, name, labelnames, labelvalues, help_text, multiprocess_mode = self._params
if typ == 'gauge':
file_prefix = typ + '_' + multiprocess_mode
else:
Expand All @@ -81,7 +81,7 @@ def __reset(self):

files[file_prefix] = MmapedDict(filename)
self._file = files[file_prefix]
self._key = mmap_key(metric_name, name, labelnames, labelvalues)
self._key = mmap_key(metric_name, name, labelnames, labelvalues, help_text)
self._value = self._file.read_value(self._key)

def __check_for_pid_change(self):
Expand Down