Conversation
| if name.startswith(cls._PREFIX): | ||
| name = name[len(cls._PREFIX):] | ||
| if name.endswith(cls._TOTAL_SUFFIX): | ||
| name = name[:-len(cls._TOTAL_SUFFIX)] | ||
| return name |
There was a problem hiding this comment.
| if name.startswith(cls._PREFIX): | |
| name = name[len(cls._PREFIX):] | |
| if name.endswith(cls._TOTAL_SUFFIX): | |
| name = name[:-len(cls._TOTAL_SUFFIX)] | |
| return name | |
| return name.removeprefix(cls._PREFIX).removesuffix(cls._TOTAL_SUFFIX) |
| if len(parts) >= 2: | ||
| dropped_count = int(parts[1]) | ||
| break | ||
| ss = metrics.snapshot() |
There was a problem hiding this comment.
[ultra nit] I don't like ss. I guess the the less a variable is used the shorter it can be, but I would still prefer metric_snapshot here.
There was a problem hiding this comment.
I think ss is fine (fascist undertones aside), metric_snapshot is too long, if we want to change it we can go with either snap or snapshot.
I also see ss is only used in the next line, so it could all be shortened to a single line without needing to define ss at all.
|
Please rebase and remove |
There was a problem hiding this comment.
Have you considered using the parser included in the prometheus client_python package instead of building our own? https://prometheus.github.io/client_python/parser/
If you have and still decided to build our own I'll take a closer look at the code in this file.
| if len(parts) >= 2: | ||
| dropped_count = int(parts[1]) | ||
| break | ||
| ss = metrics.snapshot() |
There was a problem hiding this comment.
I think ss is fine (fascist undertones aside), metric_snapshot is too long, if we want to change it we can go with either snap or snapshot.
I also see ss is only used in the next line, so it could all be shortened to a single line without needing to define ss at all.
3 participants
Description
During the work on rate limiting tests in #499, I thought it might be useful to have a metrics fixture to get a snapshot of the metrics to reason about Fact's state.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
CI is enough.