feat: Introduce a new GenericRules class to replace Rules class

[swetha1654]

Issue

POC for #191

Tandem PRs

1. charmlibs - feat(otlp): Update otlp to use generic rules class
2. otel-collector: test latest otlp interface

Visualization

The diagram visualizing the changes in this PR can be found in the issue

Solution

Some notes about the new proposal:

1. The existing AlertRules, RecordingRules and Rules class are used by several downstream libraries and repos including: cos-proxy-operator, prometheus_scrape, prometheus_remote_write, grafana-agent-operator. Rewriting the Rules class would break all of them hence I've proposed a AbstractRules class.
2. The Rules class implements both the public facing APIs (add, add_path, as_dict) as well the backend specific logic (topology injection, rules validation, group name definition logic). I've broken this down into AbstractRules class that exposes the public APIs and a generic RuleBackend class that every component uses to implement their own validation and injection logic.
3. inject_and_validate from the old Rules class has been adopted and modified in AbstractRules class to just validate. I don't see value in running the injection again on a set of rules that are already injected with juju topology (note that the add method calls _from_dict which does the injection. The add method is called by the requirer class before dumping the data into a relation databag.)

Checklist

• I have added or updated relevant documentation.
• PR title makes an appropriate release note and follows conventional commits syntax.
• Merge target is the correct branch, and relevant tandem backport PRs opened.
• This change warrants a release, so I have updated the project.version field in the pyproject.toml file.

Context

This PR makes the Rules class generic enough to accept new Rule types in the future. (for eg: Sigma rules)

Testing Instructions

Upgrade Notes

Changes for downstream users

1. The generic_alert_groups, HOST_METRICS_MISSING_RULE_NAME has been moved to cosl.prometheus hence downstream imports of it will fail, but this should be a very easy fix.

There should ideally not be any other breaking changes.

[MichaelThamm]

Deprecating AlertRules is certainly a major version bump requirement. We need to be careful because this will break our libs.

[swetha1654]

What I don't like about this design

The usage of kwargs to pass backend specific inputs via the public facing add API. Here are the other alternatives I considered -

1. Adding group_name/ group_prefix as parameters in the add API and simply ignoring it in the SigmaBackend implementation.
   Problem: This leads to leakage of backend specific inputs to other backends that don't need them.
2. Creating seperate classes called PrometheusLokiInputs and SigmaRuleInputs to pass the rule-specific information to the add API.
   Problem: Users of the GenericRules class will have yet another class to import and define before they can call the add API. The current design is already introducing an extra class for people to have to import. This solution will lead to 3 classes.

Hence I ended up using kwargs. Using kwargs sacrifies type safety but its the standard python pattern for this situation. WDYT?

[MichaelThamm]

Any downstream code using the Rules class will break

I thought we said this was not the case? Also, the passing utets indicate that this is not true. Can you update the PR description?

[MichaelThamm]

from cosl.rules import generic_alert_groups, HOST_METRICS_MISSING_RULE_NAME
# or
from cosl import generic_alert_groups

will now fail, and these are used in many places. We need to be careful with backwards compatibility here.

You can add this to cosl.rules:

# rules.py — backward-compat re-exports (deprecated, remove in next major)
from .prometheus import generic_alert_groups, HOST_METRICS_MISSING_RULE_NAME  # noqa: F401

[MichaelThamm]

In the old Rules.add_path:

else:
    logger.debug("Rules path does not exist: %s", path)

In the new GenericRules.add_path:

else:
    raise InvalidRulePathError(...)

I think this difference is fine, because we said that we would allow Rules and GenericRules to exist alongside one another and deprecate in the future.

[swetha1654]

I thought we said this was not the case? Also, the passing utets indicate that this is not true. Can you update the PR description?

@MichaelThamm my bad i forgot to remove it, I've updated the PR description

[swetha1654]

In the old Rules.add_path:

else:
    logger.debug("Rules path does not exist: %s", path)

In the new GenericRules.add_path:

else:
    raise InvalidRulePathError(...)

I think this difference is fine, because we said that we would allow Rules and GenericRules to exist alongside one another and deprecate in the future.

Yes this shouldn't be a breaking change or problem

[swetha1654]

from cosl.rules import generic_alert_groups, HOST_METRICS_MISSING_RULE_NAME
# or
from cosl import generic_alert_groups

will now fail, and these are used in many places. We need to be careful with backwards compatibility here.

You can add this to cosl.rules:

# rules.py — backward-compat re-exports (deprecated, remove in next major)
from .prometheus import generic_alert_groups, HOST_METRICS_MISSING_RULE_NAME  # noqa: F401

@MichaelThamm I dont think from cosl import generic_alert_groups would even work in the current state since generic_alert_groups is not defined in the __init__.py file. Now that you mention that generic_alert_groups are not specific to Prometheus and can theoretically include Loki rules or sigma rules in the future, I don't know if i want to keep it in the prometheus module anymore so I will move it to common_rules.py and import them inside rules.py to avoid breaking changes.

[MichaelThamm]

To gain extra confidence that this PR is not breaking and providing the features we want, we should:

1. Create a tandem PR in OTLP charmlib, so we can review the API usage
2. @MichaelThamm reference this cosl PR from branch in Prometheus and run tox to test that there are no breaking changes.

[swetha1654]

@MichaelThamm I have created a tandem PR in charmlib to update the otlp interface to use the GenericRules class.

[swetha1654]

@MichaelThamm reference this cosl PR from branch in Prometheus and run tox to test that there are no breaking changes.

I think the linting will most likely fail. You would need to add # pyright: ignore[reportPrivateImportUsage] to wherever HOST_METRICS_MISSING_RULE_NAME and generic_alert_groups is imported. Or you would have to change the import line to import these from cosl.common_rules instead.

[MichaelThamm]

I think this PR is pretty much ready to be merged content-wise after my recent comments are resolved. However, there is one thing which makes me nervous: we are lacking a lot of rules coverage for the new rules class. You added some tests: test_backend and test_generic_rules, but these provide little coverage compared to the existing set of tests we had for the Rules class.

One could argue that since this lib will only be used by the OTLP charmlib, we there is minimal risk. I think this is a good way to faze in this feature into the rules ecosystem without too much friction for our users. We will have to migrate the tests slowly to use the new rules classes to gain the same confidence as before. I don't think this is your responsibility, and given that you will be an early adopter of this feature, I think we can proceed with merging. I will discuss this briefly with @sed-i just to be safe.

[sed-i]

Thanks for this @swetha1654!
As you discovered, our impl of the rules has some flaws, and I see how the current approach sets up the stage for adding more rule types.

Before we go all in on Generic[T] and abc, I was wondering if we could explore a simpler approach, where we avoid generalizing via base classes.

As you outlined in the comparison table in #191, *QL-rules have little in common with sigma rules:

	Prometheus	Loki	Tempo
Alerting rule	PromQL, mimir ruler, sent to alertmanager (ref)	LogQL, evaluated by loki ruler, sent to alertmanager (ref)	TraceQL, via metrics-generator, experimental (ref)
Recording rule	PromQL, mimir rules, "pushed to self" (ref)	LogQL, evaluated by loki ruler, produces metric that is persisted to WAL, sent to mimir (ref)	N/A

Looking at _otlp.py, is there conceptually much more to do than the following?

-from cosl.rules import HOST_METRICS_MISSING_RULE_NAME, Rules, generic_alert_groups
+from cosl.rules import HOST_METRICS_MISSING_RULE_NAME, Rules, SigmaRules, generic_alert_groups

@dataclass
class RuleStore:
    topology: JujuTopology
    logql: Rules = field(init=False)
    promql: Rules = field(init=False)
+   sigma: SigmaRules = field(init=False)

class _RulesModel(BaseModel):

    logql: OfficialRuleFileFormat = Field(...)
    promql: OfficialRuleFileFormat = Field(...)
+   sigma: SigmaRuleFileFormat = Field(
+       description='...',
+       default_factory=SigmaRuleFileFormat,
+   )

Then, for sigma rules, instead of inheriting ready-made implementations from parent classes, we could extract logic into free function that could be reused by SigmaRules.

Wdyt?

[MichaelThamm]

@sed-i I do think your suggestion is the quickest way to get Sigma rules into otelcol. The main updates are required in the rules method for topology injection (if desired), and the RuleStore object.

cos-lib abstracts a lot of the rules effort for PromQL and LogQL (e.g., injection, validation, adding, as_dict, etc.). As long as we still have Sigma implementation detail abstracted in cos-lib (like we do for promQL and logQL), then I think this is a great solution. Similar to how promql are added to the rules store: self.promql.add(...) using methods from a Sigma rules class in cos-lib, would be trivial.

[swetha1654]

@sed-i @MichaelThamm If i understand correctly, what you're proposing is a seperate SigmaRules class in cos-lib instead of having an abstract class and then implementing it for Sigma / Prometheus / Loki.

As you outlined in the comparison table in #191, *QL-rules have little in common with sigma rules

I agree that Sigma rules themselves don't have a lot in common with *QL-rules, but there is still a lot in common between what we want to do with these rules. For eg:

1. we want to expose add/add_path APIs for users to add these rules, regardless of rule type.
2. We want to expose a method to validate these rules, regardless of their type.
3. We want a way for the users to export these rules to a dict to dump them into relation data, regardless of type.
4. We want each rule-type to specify what type of file extensions are accepted, so different backend implementations don't miss out specifying all allowed extensions.

Implementing a seperate SigmaRules class would mean repeating a lot of common logic between the user facing APIs, possibilities of divergence in the expected behaviour of add/add_path APIs, and many more issues. I don't see any added value in implementing SigmaRules as a separate class. If anything, it could make future maintenance of these *Rules classes more difficult in the future as we now have multiple classes to maintain.

Let me know if you have any further questions. I have visualized what I am trying to do with this PR in a diagram in the original issue for better understanding.

[MichaelThamm]

Closing this PR in favor of:

• feat: Sigma rules pt.3 #197