diff --git a/promgen/admin.py b/promgen/admin.py index 6c0681b25..1d90c12fb 100644 --- a/promgen/admin.py +++ b/promgen/admin.py @@ -104,20 +104,11 @@ def has_add_permission(self, request): readonly_fields = ("project",) -class RuleLabelInline(admin.TabularInline): - model = models.RuleLabel - - -class RuleAnnotationInline(admin.TabularInline): - model = models.RuleAnnotation - - @admin.register(models.Rule) class RuleAdmin(admin.ModelAdmin): list_display = ("name", "clause", "duration", "content_object") list_filter = ("duration",) list_select_related = ("content_type",) - inlines = [RuleLabelInline, RuleAnnotationInline] def get_queryset(self, request): qs = super().get_queryset(request) diff --git a/promgen/fixtures/extras.yaml b/promgen/fixtures/extras.yaml index cada7fbcc..ff3abc871 100644 --- a/promgen/fixtures/extras.yaml +++ b/promgen/fixtures/extras.yaml @@ -9,18 +9,10 @@ duration: 1s content_type: ["promgen", "site"] object_id: 1 -- model: promgen.ruleannotation - pk: 1 - fields: - name: summary - value: Example rule summary - rule_id: 1 -- model: promgen.rulelabel - pk: 1 - fields: - name: severity - value: high - rule_id: 1 + labels: + severity: "high" + annotations: + summary: "Example rule summary" - model: promgen.alert pk: 1 fields: diff --git a/promgen/forms.py b/promgen/forms.py index 97908548d..35a41e2b2 100644 --- a/promgen/forms.py +++ b/promgen/forms.py @@ -2,7 +2,7 @@ # These sources are released under the terms of the MIT license: see LICENSE import re - +from functools import partial from dateutil import parser from promgen import models, plugins, prometheus, validators @@ -119,7 +119,6 @@ class Meta: class AlertRuleForm(forms.ModelForm): class Meta: model = models.Rule - exclude = ["parent", "content_type", "object_id"] widgets = { "name": forms.TextInput(attrs={"class": "form-control"}), "duration": forms.TextInput(attrs={"class": "form-control"}), @@ -127,6 +126,9 @@ class Meta: "enabled": forms.CheckboxInput(attrs={"data-toggle": "toggle", "data-size": "mini"}), "description": forms.Textarea(attrs={"rows": 5, "class": "form-control"}), } + # We define a custom widget for each of our fields, so we just take the + # keys here to avoid manually updating a list of fields. + fields = widgets.keys() def clean(self): # Check our cleaned data then let Prometheus check our rule @@ -142,6 +144,45 @@ def clean(self): prometheus.check_rules([rule]) +class _KeyValueForm(forms.Form): + key = forms.CharField(widget=forms.TextInput(attrs={"class": "form-control"})) + value = forms.CharField(widget=forms.TextInput(attrs={"class": "form-control"})) + +# We need a custom KeyValueSet because we need to be able to convert between the single dictionary +# form saved to our models, and the list of models used by +class _KeyValueSet(forms.BaseFormSet): + def __init__(self, initial=None, **kwargs): + if initial: + kwargs["initial"] = [{"key": key, "value": initial[key]} for key in initial] + super().__init__(**kwargs, form_kwargs={"empty_permitted": True}) + + def to_dict(self): + return {x["key"]: x["value"] for x in self.cleaned_data if x and not x["DELETE"]} + +# For both LabelFormSet and AnnotationFormSet we always want to have a prefix assigned, but it's +# awkward if we need to specify it in multiple places. We use a partial here, so that it is the same +# as always passing prefix as part of our __init__ call. +LabelFormSet = partial( + forms.formset_factory( + form=_KeyValueForm, + formset=_KeyValueSet, + can_delete=True, + extra=1, + ), + prefix="labels", +) + +AnnotationFormSet = partial( + forms.formset_factory( + form=_KeyValueForm, + formset=_KeyValueSet, + can_delete=True, + extra=1, + ), + prefix="annotations", +) + + class RuleCopyForm(forms.Form): content_type = forms.ChoiceField(choices=[(x, x) for x in ["service", "project"]]) object_id = forms.IntegerField() @@ -185,25 +226,3 @@ def clean(self): if not hosts: raise ValidationError("No valid hosts") self.cleaned_data["hosts"] = list(hosts) - - -LabelFormset = forms.inlineformset_factory( - models.Rule, - models.RuleLabel, - fields=("name", "value"), - widgets={ - "name": forms.TextInput(attrs={"class": "form-control"}), - "value": forms.TextInput(attrs={"rows": 5, "class": "form-control"}), - }, -) - - -AnnotationFormset = forms.inlineformset_factory( - models.Rule, - models.RuleAnnotation, - fields=("name", "value"), - widgets={ - "name": forms.TextInput(attrs={"class": "form-control"}), - "value": forms.Textarea(attrs={"rows": 2, "class": "form-control"}), - }, -) diff --git a/promgen/migrations/0022_rule_labels_annotations.py b/promgen/migrations/0022_rule_labels_annotations.py new file mode 100644 index 000000000..a0d7e41a3 --- /dev/null +++ b/promgen/migrations/0022_rule_labels_annotations.py @@ -0,0 +1,56 @@ +# Generated by Django 3.2.13 on 2023-07-11 03:02 + +from django.db import migrations, models + + +def forward(apps, schema_editor): + Rule = apps.get_model("promgen", "Rule") + Label = apps.get_model("promgen", "RuleLabel") + Annotation = apps.get_model("promgen", "RuleAnnotation") + + # For our forward migration, we want to loop through all the old Label and Annotation entries + # and convert them to a dictionary property on our Rule model + for rule in Rule.objects.all(): + rule.labels = {l.name: l.value for l in Label.objects.filter(rule_id=rule.id)} + rule.annotations = {l.name: l.value for l in Annotation.objects.filter(rule_id=rule.id)} + rule.save() + + +def reverse(apps, schema_editor): + Rule = apps.get_model("promgen", "Rule") + Label = apps.get_model("promgen", "RuleLabel") + Annotation = apps.get_model("promgen", "RuleAnnotation") + for rule in Rule.objects.all(): + Label.objects.bulk_create( + [Label(rule_id=rule.id, name=x, value=rule.labels[x]) for x in rule.labels] + ) + Annotation.objects.bulk_create( + [Annotation(rule_id=rule.id, name=x, value=rule.annotations[x]) for x in rule.annotations] + ) + + +class Migration(migrations.Migration): + + dependencies = [ + ("promgen", "0021_shard_load"), + ] + + operations = [ + migrations.AddField( + model_name="rule", + name="annotations", + field=models.JSONField(default=dict), + ), + migrations.AddField( + model_name="rule", + name="labels", + field=models.JSONField(default=dict), + ), + migrations.RunPython(forward, reverse), + migrations.DeleteModel( + name="RuleAnnotation", + ), + migrations.DeleteModel( + name="RuleLabel", + ), + ] diff --git a/promgen/models.py b/promgen/models.py index a71a6615f..9dd0d0a14 100644 --- a/promgen/models.py +++ b/promgen/models.py @@ -415,27 +415,12 @@ class Rule(models.Model): content_object = GenericForeignKey("content_type", "object_id", for_concrete_model=False) description = models.TextField(blank=True) + labels = models.JSONField(default=dict) + annotations = models.JSONField(default=dict) + class Meta: ordering = ["content_type", "object_id", "name"] - @cached_property - def labels(self): - return {obj.name: obj.value for obj in self.rulelabel_set.all()} - - def add_label(self, name, value): - return RuleLabel.objects.get_or_create(rule=self, name=name, value=value) - - def add_annotation(self, name, value): - return RuleAnnotation.objects.get_or_create(rule=self, name=name, value=value) - - @cached_property - def annotations(self): - _annotations = {obj.name: obj.value for obj in self.ruleannotation_set.all()} - # Skip when pk is not set, such as when test rendering a rule - if self.pk and "rule" not in _annotations: - _annotations["rule"] = resolve_domain("rule-detail", pk=self.pk) - return _annotations - def __str__(self): return f"{self.name} [{self.content_object.name}]" @@ -481,41 +466,14 @@ def copy_to(self, content_type, object_id): macro.EXCLUSION_MACRO, f'{content_type.model}="{content_object.name}",{macro.EXCLUSION_MACRO}', ) - self.save() # Add a label to our new rule by default, to help ensure notifications # get routed to the notifier we expect - self.add_label(content_type.model, content_object.name) - - for label in RuleLabel.objects.filter(rule_id=orig_pk): - # Skip service labels from our previous rule - if label.name in ["service", "project"]: - logger.debug("Skipping %s: %s", label.name, label.value) - continue - logger.debug("Copying %s to %s", label, self) - label.pk = None - label.rule = self - label.save() - - for annotation in RuleAnnotation.objects.filter(rule_id=orig_pk): - logger.debug("Copying %s to %s", annotation, self) - annotation.pk = None - annotation.rule = self - annotation.save() - - return self - - -class RuleLabel(models.Model): - name = models.CharField(max_length=128) - value = models.CharField(max_length=128) - rule = models.ForeignKey("Rule", on_delete=models.CASCADE) + self.labels[content_type.model] = content_object.name + self.save() -class RuleAnnotation(models.Model): - name = models.CharField(max_length=128) - value = models.TextField() - rule = models.ForeignKey("Rule", on_delete=models.CASCADE) + return self class AlertLabel(models.Model): diff --git a/promgen/prometheus.py b/promgen/prometheus.py index f3b05de7e..2ce950dd0 100644 --- a/promgen/prometheus.py +++ b/promgen/prometheus.py @@ -160,33 +160,29 @@ def import_rules_v2(config, content_object=None): counters = collections.defaultdict(int) for group in config["groups"]: for r in group["rules"]: - labels = r.get("labels", {}) - annotations = r.get("annotations", {}) defaults = { "clause": r["expr"], "duration": r["for"], + "labels": r.get("labels", {}), + "annotations": r.get("annotations", {}), } # Check our labels to see if we have a project or service # label set and if not, default it to a global rule if content_object: defaults["obj"] = content_object - elif "project" in labels: - defaults["obj"] = models.Project.objects.get(name=labels["project"]) - elif "service" in labels: - defaults["obj"] = models.Service.objects.get(name=labels["service"]) + elif "project" in defaults["labels"]: + defaults["obj"] = models.Project.objects.get(name=defaults["labels"]["project"]) + elif "service" in defaults["labels"]: + defaults["obj"] = models.Service.objects.get(name=defaults["labels"]["service"]) else: defaults["obj"] = models.Site.objects.get_current() - rule, created = models.Rule.objects.get_or_create(name=r["alert"], defaults=defaults) + _, created = models.Rule.objects.get_or_create(name=r["alert"], defaults=defaults) if created: counters["Rules"] += 1 - for k, v in labels.items(): - rule.add_label(k, v) - for k, v in annotations.items(): - rule.add_annotation(k, v) return dict(counters) diff --git a/promgen/serializers.py b/promgen/serializers.py index 364975518..97bd4e927 100644 --- a/promgen/serializers.py +++ b/promgen/serializers.py @@ -6,6 +6,7 @@ import promgen.templatetags.promgen as macro from promgen import models, shortcuts +from promgen.shortcuts import resolve_domain class WebLinkField(serializers.Field): @@ -88,16 +89,17 @@ def many_init(cls, queryset, *args, **kwargs): "content_type", "overrides__content_object", "overrides__content_type", - "ruleannotation_set", - "rulelabel_set", ) return AlertRuleList(queryset, *args, **kwargs) def to_representation(self, obj): + annotations = obj.annotations + annotations["rule"] = resolve_domain("rule-detail", pk=obj.pk if obj.pk else 0) + return { "alert": obj.name, "expr": macro.rulemacro(obj), "for": obj.duration, "labels": obj.labels, - "annotations": obj.annotations, + "annotations": annotations, } diff --git a/promgen/templates/promgen/rule_formset_table.html b/promgen/templates/promgen/rule_formset_table.html index 9f6784262..8265701d8 100644 --- a/promgen/templates/promgen/rule_formset_table.html +++ b/promgen/templates/promgen/rule_formset_table.html @@ -5,16 +5,16 @@ Value Delete? - {% for row in formset %} + {% for form in formset %} - {{ row.id }} {{ row.name }} - {{ row.value }} - {{ row.DELETE }} + {{ form.id }} {{ form.key }} + {{ form.value }} + {{ form.DELETE }} - {% for k,v in row.errors.items %} + {% if form.errors %} - {{v}} + {{form.errors}} - {% endfor %} + {% endif %} {% endfor %} diff --git a/promgen/templates/promgen/rule_update.html b/promgen/templates/promgen/rule_update.html index 793c51f9c..d6019e2e8 100644 --- a/promgen/templates/promgen/rule_update.html +++ b/promgen/templates/promgen/rule_update.html @@ -27,8 +27,7 @@

Rule: {{ rule.name }}

{% csrf_token %}
- - {% if form.non_field_errors %} + {% if form.non_field_errors %}
Errors
{% for error in form.non_field_errors %} @@ -46,7 +45,7 @@

Rule: {{ rule.name }}

Labels - Control routing through AlertManager and Promgen
- {% include 'promgen/rule_formset_table.html' with formset=formset_labels only %} + {% include 'promgen/rule_formset_table.html' with formset=label_form only %}
  • Examples
  • @@ -57,7 +56,7 @@

    Rule: {{ rule.name }}

    Annotations - Provide extra details for notifications
    - {% include 'promgen/rule_formset_table.html' with formset=formset_annotations only %} + {% include 'promgen/rule_formset_table.html' with formset=annotation_form only %}
    • Examples
    • @@ -101,7 +100,7 @@

      Rule: {{ rule.name }}

      - + {% csrf_token %} diff --git a/promgen/tests/test_alert_rules.py b/promgen/tests/test_alert_rules.py index ce45cc62e..5138bd0cb 100644 --- a/promgen/tests/test_alert_rules.py +++ b/promgen/tests/test_alert_rules.py @@ -45,13 +45,6 @@ def test_copy(self, mock_post): # Test that our copy has the same labels and annotations self.assertIn("severity", copy.labels) self.assertIn("summary", copy.annotations) - # and test that we actually duplicated them and not moved them - self.assertCount( - models.RuleLabel, - 3, - "Copied rule has exiting labels + service label", - ) - self.assertCount(models.RuleAnnotation, 2) @override_settings(PROMGEN=TEST_SETTINGS) @mock.patch("django.dispatch.dispatcher.Signal.send") @@ -67,8 +60,6 @@ def test_import_v2(self, mock_post): # Includes count of our setUp rule + imported rules self.assertRoute(response, views.RuleImport, status=200) self.assertCount(models.Rule, 3, "Missing Rule") - self.assertCount(models.RuleLabel, 4, "Missing labels") - self.assertCount(models.RuleAnnotation, 9, "Missing annotations") @override_settings(PROMGEN=TEST_SETTINGS) @mock.patch("django.dispatch.dispatcher.Signal.send") @@ -83,8 +74,6 @@ def test_import_project_rule(self, mock_post): ) self.assertRoute(response, views.ProjectDetail, status=200) self.assertCount(models.Rule, 3, "Missing Rule") - self.assertCount(models.RuleLabel, 4, "Missing labels") - self.assertCount(models.RuleAnnotation, 9, "Missing annotations") @override_settings(PROMGEN=TEST_SETTINGS) @mock.patch("django.dispatch.dispatcher.Signal.send") @@ -101,8 +90,6 @@ def test_import_service_rule(self, mock_post): ) self.assertRoute(response, views.ServiceDetail, status=200) self.assertCount(models.Rule, 3, "Missing Rule") - self.assertCount(models.RuleLabel, 4, "Missing labels") - self.assertCount(models.RuleAnnotation, 9, "Missing annotations") @mock.patch("django.dispatch.dispatcher.Signal.send") def test_missing_permission(self, mock_post): @@ -145,6 +132,6 @@ def test_macro(self, mock_post): def test_invalid_annotation(self, mock_post): rule = models.Rule.objects.get(pk=1) # $label.foo is invalid (should be $labels) so make sure we raise an exception - models.RuleAnnotation.objects.create(name="summary", value="{{$label.foo}}", rule=rule) + rule.annotations["summary"] = "{{$label.foo}}" with self.assertRaises(ValidationError): prometheus.check_rules([rule]) diff --git a/promgen/views.py b/promgen/views.py index c3310265d..5fd501f43 100644 --- a/promgen/views.py +++ b/promgen/views.py @@ -500,19 +500,13 @@ def get_context_data(self, **kwargs): site_rules = models.Rule.objects.filter( content_type__model="site", content_type__app_label="promgen" - ).prefetch_related( - "content_object", - "rulelabel_set", - "ruleannotation_set", - ) + ).prefetch_related("content_object") service_rules = models.Rule.objects.filter( content_type__model="service", content_type__app_label="promgen" ).prefetch_related( "content_object", "content_object", - "rulelabel_set", - "ruleannotation_set", "parent", ) @@ -522,8 +516,6 @@ def get_context_data(self, **kwargs): "content_object", "content_object__service", "content_object__service", - "rulelabel_set", - "ruleannotation_set", "parent", ) @@ -738,11 +730,7 @@ class RuleDetail(LoginRequiredMixin, DetailView): queryset = models.Rule.objects.prefetch_related( "content_object", "content_type", - "ruleannotation_set", - "rulelabel_set", "overrides", - "overrides__ruleannotation_set", - "overrides__rulelabel_set", "overrides__content_object", "overrides__content_type", ) @@ -770,10 +758,12 @@ def get_permission_required(self): def get_context_data(self, **kwargs): context = super().get_context_data(**kwargs) - context.setdefault("formset_labels", forms.LabelFormset(instance=self.object)) - context.setdefault("formset_annotations", forms.AnnotationFormset(instance=self.object)) context["macro"] = macro.EXCLUSION_MACRO context["rules"] = [self.object.parent] if self.object.parent else [self.object] + # We use setdefault here, because we may have received existing forms in the case of a POST + # that has errors to be corrected + context.setdefault("label_form", forms.LabelFormSet(initial=self.object.labels)) + context.setdefault("annotation_form", forms.AnnotationFormSet(initial=self.object.annotations)) return context def form_invalid(self, **kwargs): @@ -786,41 +776,27 @@ def post(self, request, *args, **kwargs): # Save a copy of our forms into a context var that we can use # to re-render our form properly in case of errors context = {} - context["form"] = form = self.get_form() - context["formset_labels"] = form_labels = forms.LabelFormset( - request.POST, request.FILES, instance=self.object - ) - context["formset_annotations"] = form_annotations = forms.AnnotationFormset( - request.POST, request.FILES, instance=self.object - ) - - # Check validity of our labels and annotations in Django before we try to render - if not all([form_labels.is_valid(), form_annotations.is_valid()]): - return self.form_invalid(**context) - - # Populate our cached_properties so we can render a test - # populate only rows with a 'value' so that we skip fields we're deleting - # see Django docs on cached_property and promgen.forms.RuleForm.clean() - form.instance.labels = { - l["name"]: l["value"] for l in form_labels.cleaned_data if "value" in l - } - form.instance.annotations = { - a["name"]: a["value"] for a in form_annotations.cleaned_data if "value" in a - } + context["form"] = self.get_form() + context["label_form"] = forms.LabelFormSet(data=request.POST) + context["annotation_form"] = forms.AnnotationFormSet(data=request.POST) # With our labels+annotations manually cached we can test - if not form.is_valid(): + if not all( + [ + context["form"].is_valid(), + context["label_form"].is_valid(), + context["annotation_form"].is_valid(), + ] + ): return self.form_invalid(**context) - # Save our labels - for instance in form_labels.save(): - messages.info(request, f"Added {instance.name} to {self.object}") - - # Save our annotations - for instance in form_annotations.save(): - messages.info(request, f"Added {instance.name} to {self.object}") + # After we validate that our forms are valid, we can just copy over the + # cleaned data into our instance so that it can be saved in the call to + # form_valid. + context["form"].instance.labels = context["label_form"].to_dict() + context["form"].instance.annotations = context["annotation_form"].to_dict() - return self.form_valid(form) + return self.form_valid(context["form"]) class AlertRuleRegister(mixins.PromgenPermissionMixin, mixins.RuleFormMixin, FormView): @@ -918,7 +894,7 @@ def get_context_data(self, **kwargs): context = super().get_context_data(**kwargs) context["rule_list"] = models.Rule.objects.filter( content_type__model="site", content_type__app_label="promgen" - ).prefetch_related("content_object", "rulelabel_set", "ruleannotation_set") + ).prefetch_related("content_object") return context @@ -1152,7 +1128,7 @@ def get(self, request): "rule_list": { "field": ("name__icontains", "clause__icontains"), "model": models.Rule, - "prefetch": ("content_object", "ruleannotation_set", "rulelabel_set"), + "prefetch": ("content_object"), "query": ("search",), }, "service_list": { diff --git a/pyproject.toml b/pyproject.toml index 26a9b5c3a..e30396f40 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -15,3 +15,6 @@ known_first_party = "promgen" sections = "FUTURE,STDLIB,DJANGO,THIRDPARTY,FIRSTPARTY,LOCALFOLDER" profile = "black" float_to_top = true + +[tool.ruff] +ignore = ['E501']