[agent] Ray metrics use the name parsing as Prometheus #50443
Conversation
Closes #40586 Signed-off-by: Ibrahim Rabbani <[email protected]>
a private member of the Metric class Signed-off-by: Ibrahim Rabbani <[email protected]>
| # Copied from Prometheus Python Client. While the regex is not part of the public API | ||
| # for Prometheus, it's not expected to change | ||
| # https://github.com/prometheus/client_python/blob/46eae7bae88f76951f7246d9f359f2dd5eeff110/prometheus_client/validation.py#L4 | ||
| METRIC_NAME_RE = re.compile(r"^[a-zA-Z_:][a-zA-Z0-9_:]*$") |
There was a problem hiding this comment.
| METRIC_NAME_RE = re.compile(r"^[a-zA-Z_:][a-zA-Z0-9_:]*$") | |
| _METRIC_NAME_RE = re.compile(r"^[a-zA-Z_:][a-zA-Z0-9_:]*$") |
to indicate it's not a public API (since this is in public API namespace)
| with pytest.raises(ValueError): | ||
| Metric("faulty-metric") | ||
| with pytest.raises(ValueError): | ||
| Metric("1cannotstartwithnumber") |
There was a problem hiding this comment.
| with pytest.raises(ValueError): | |
| Metric("faulty-metric") | |
| with pytest.raises(ValueError): | |
| Metric("1cannotstartwithnumber") | |
| with pytest.raises(ValueError, match="string to match in the exception message"): | |
| Metric("faulty-metric") | |
| with pytest.raises(ValueError, match="string to match in the exception message"): | |
| Metric("1cannotstartwithnumber") |
There was a problem hiding this comment.
Admittedly I've never written much python before so I'm learning.
Doesn't this pattern effectively make the exception message part of the class API i.e. if you change the message, the tests break? Is that the intention?
There was a problem hiding this comment.
yes -- it's mostly a way to enforce it to ourselves that we're providing good error messages (for example if someone in the future added a new validation clause this would force them to explicitly acknowledge they changed the user-facing error message)
| raise ValueError("Empty name is not allowed. Please provide a metric name.") | ||
| if not METRIC_NAME_RE.match(name): | ||
| raise ValueError( | ||
| f"Invalid metric name: {name}. Does not match regex: {METRIC_NAME_RE.pattern}" |
There was a problem hiding this comment.
regex's are really hard to read. maybe for common error types such as starting with an int or special character, we can also add handle-rolled validation to provide a better error message
There was a problem hiding this comment.
How about changing the error message to:
Invalid metric name: {name}. Names can only start with letters or _. Names can only contain letters, numbers, and _
|
Validation failure in Ray Data tests! Interesting... |
|
It looks like our integration tests with Mars are failing. Apparently they create ray metrics with names that contain What do next steps for this look like? |
We will fail fast when trying to create metrics that do not pass validation in Prometheus. Added a unit test to make sure invalid metric names are not allowed.
Closes #40586