Support metric help text in multiprocess mode #804
Conversation
butlerx
force-pushed
the
multi-process-help
branch
from
39f0d37
to
0337a8f
Compare
Issue prometheus#211 Add support for storing the metrics help text in the multiprocess map. The help will come from the first process read, but it should be the same for all metrics. Signed-off-by: Cian Butler <butlerx@notthe.cloud>
butlerx
force-pushed
the
multi-process-help
branch
from
0337a8f
to
f6a2b4a
Compare
|
Thanks for the PR! I just got back from holiday and will put this on the list to review soon. |
| """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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ive not had time to get back to this but looks like @evgenymarkov has written some tests in #866
There was a problem hiding this comment.
Oh sorry. I didn't find this pull request. You can cherry-pick 89e3193 and then I will close my PR.
Signed-off-by: Evgeny Markov <evgenymarkov@yandex.com>
|
Hi @csmarchbanks! Sorry for ping, but can you recheck this PR? It would be cool to get a release with this code 🙏 |
|
Thanks for updating this PR, I will take another look soon and give it a bit more thought! It will probably be next week as this week is pretty busy. |
Pass through extra arguments to mmap_key after prometheus/client_python#804
Pass through extra arguments to mmap_key after prometheus/client_python#804
* chore(deps): update minor updates * fix(deps): prometheus_client 0.16 compat Pass through extra arguments to mmap_key after prometheus/client_python#804 --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Jakob van Santen <jvansanten@gmail.com>
Issue #211
Add support for storing the metrics help text in the multiprocess map.
The help will come from the first process read, but it should be the
same for all metrics.
@csmarchbanks not sure if i missed anything but let me know if you need anything changed