Break the Rules monolith. Add support for recording rules. #433
Conversation
rbarry82
requested review from
sed-i,
Abuelodelanada,
lucabello,
PietroPasotti,
dstathis and
simskij
as code owners
…o 3.8 now that ops is
| @@ -36,6 +37,7 @@ | |||
| ) | |||
| from ops.framework import EventBase, EventSource, Object, ObjectEvents | |||
| from ops.model import Relation | |||
| from typing_extensions import Literal | |||
There was a problem hiding this comment.
Literal is a 3.8 feature.
Perhaps this was added before changing mypy --python-version to 3.8?
There was a problem hiding this comment.
Probably. I'll re-check...
| _rule_type = "alert" # type: _RuleType | ||
|
|
||
| @property | ||
| def rule_type(self) -> _RuleType: |
There was a problem hiding this comment.
Probably wouldn't need a property if we use ClassVar?
There was a problem hiding this comment.
Well, it would, because there's no similar ClassVar on the parent, so self.rule_type won't resolve on AlertRules(ABC) without at least an @abstractmethod, hence the property. Parents can't use the attributes of their children unless they're defined, and the ABC parent has no reason to have a _rule_type
|
The plot thickens? name: MyGroupName
source_tenants: ["tenant-a", "tenant-b"]
rules:
- record: sum:metric
expr: sum(metric) |
Probably only if we ever get to the point of multi-tenancy, but at least |
Co-authored-by: Leon <[email protected]>
Co-authored-by: Leon <[email protected]>
|
Closing to move this to |
Issue
prometheus_scrapedoesn't currently support recording rules via any intentional mechanism -- they can be accidentally loaded via theAlertRulespathway, but none of the log messages or databags will be meaningful. We should support this use case, separated.Solution
The actual handling of the rules is mostly identical, outside of keys to check for if we're validating single rule formats, and a string to use in log messages/alerts.
Make
AlertRulesintoRules(ABC)with an@abstractmethod+@propertywhich returns aLiteral_RulesType, which child classes implement to indicate the string to interpolate where needed. Similarly, extract the handling foralerts()into a more generalized method which takes the type as an argument in addition and maps it to a databag. All of the handling there is also identical (for now).Remove every reference I could find to
alertin docstrings, docs, and variable names where it should not be present, substituting the interpolated value or appending/amending docstrings to speak about both types. RenameCosTool.validate_alert_rules()toCosTool.validate_rules()(it isn't specific). Rename exceptions to more generic names.Add constructor args and events for recording rules.
In this way, if/when we ever do need different handling (such as if we do not want to support "single rule file" formats for recording rules, or Loki, or other) or the relation data needs/APIs for either type diverge, they are already cleanly separated without breaking the current API.
In addition, this restructuring makes it possible to, once pydeps for charmcraft is implemented, move the
[T]Rulesmachinery elsewhere, rather than trying to keepprometheus_scrape,prometheus_remote_write,loki_push_api, and others in sync, with independent patches needed any time a bug or feature comes up in any of them (with a little handling to smashmetadataintoscrape_metadata, even as just an optionalmetadata_keyarg in the constructor forRules())Closes #428
Context
Also,
mock.create_autospec()exists, and it's great._dedupe_...was moved back into the class it belongs to (the only place that uses it), andtest_functionsusescreate_autospecto test that function in an isolated way. We don't need to extract methods to the top level to make writing unit tests easier, and, as a common refrain, doing so makes new readers/authors or anyone writing a patch juggle more mental state while trying to keep track of whether it's really doing something generalized, used elsewhere, etc.The problem domain of
_dedupe_...is in the only class which ever calls it, andcreate_autospec()makes unit testing it in an isolated fashion just as easy as importing it directly from the top-level.Testing Instructions
Deploy. Make sure there aren't regressions.
Release Notes
Add support for recording rules.