decoupled alerts from Slack by IDoneShaveIt · Pull Request #1266 · elementary-data/elementary

IDoneShaveIt · 2023-11-07T08:39:28Z

No description provided.

linear · 2023-11-07T08:39:31Z

…le-1852-decouple-alerts-from-slack

…ts in client

…le-1852-decouple-alerts-from-slack

ellakz · 2023-11-13T15:18:15Z

 logger = get_logger(__name__)


+ALERT_TABLES = dict(


Isn't is simpler to just create a dict called TABLE_NAME_TO_RESOURCE_TYPE ?

It might. I actually just moved this part around 🙂
Checking for a simpler solution!

I removed the need of that const and improved the logic and moved it into the alerts fetcher (this is actually the only place that should be aware of the tables)

ellakz · 2023-11-13T15:27:41Z

+        self.sent_alert_count = 0
+        self.send_test_message_on_success = send_test_message_on_success
+        self.override_meta_slack_channel = override_config
+        self.alerts_integraion = self._get_integration_client()


should this be a class method in BaseIntegration? or somewhere over there?

This is the function that determines which integration we should use.
It can't be part of the integration itself, and once we will add more integrations it should be more dynamic (currently it just returns SlackIntegration)

I want to think about a way to move all its logic to the integrations area, I will think about it when I integrate PD

We can create a method / class (outside of the BaseIntegration) that determines which integration we should use (base on params / config) and we can call it at the data monitoring.
We will still have a method that calls it inside the data monitoring but all the logic won't be there.

ellakz · 2023-11-13T15:29:49Z

+        ],
+    ) -> Dict[str, Any]:
+        return dict(
+            channel=(


isn't this also a little slack-specific? I wonder if we can somehow pass on all the param handling to the integration itself

The issue is that currently we pass the params as part of our dbt package.
If I could I would have changed that, but currently users are relaying on it so its a bit problematic.
I will try to think of a way to move it to the integration itself but I a not sure this is that simple

Moved the get integration params to be part of the integration itself

ellakz · 2023-11-13T16:58:03Z

+            filter=self.filter.get_filter(),
+        )
+
+    def _format_alert(


It looks like the Pending alert and Alert class are pretty much overlapping... wouldn't it make more senes to have just one class, and if there's additional info or methods you can keep them in a helper class or something?
In any case, if you choose not to do that- I don't love this function, I think it can be nicer to create a to_formatted_alert method in every type of pending alert

In my philosophy of code, those are 2 different class (although they are overlapping).
One (Pending alerts schema) is a schema which is used to validate typing and the structure of the data we query from the db using the alerts fetcher.
This will not be used in the SaaS version because we are going to query it from other tables and the fields probably going to be different.

The Alerts models are the "normalized" schema of alerts that is used by the integrations. This is the data structure we "sign" on that is going to contain all the data an integration need on an alert.

Merging the 2 is not a good option IMO because they have a different purpose and use, they first one is changeable depends on the system, and currently they are overlapping but they can easily be changed and don't 🙂

ok, in that case I would move this method to the pending alerts classes instead of using ifs and isinstance, I think it's cleaner

I will move it 🙂
This is scares me a bit because it can create a couple between the 2 in the future (I really like to separate the 2 - fetcher should not be aware of the API that is calling it, and adding the format to the schema kinda makes it coupled).
So if I will see in the future that we are coupling them I will ask to revert it to that change 🙂

ellakz · 2023-11-13T17:17:42Z

+    ) == json.dumps(["alert_id_2", "alert_id_3", "alert_id_4"])
+    assert json.dumps(
+        [alert.id for alert in source_freshness_alerts], sort_keys=True
+    ) == json.dumps(["alert_id_2", "alert_id_3", "alert_id_4"])


maybe we want to check more fields are formatted properly? could be nice to add a verification that nothing is omitted in the formatting

So I don't check here that the fields got formatted properly, but that the right alerts got formatted to the relevant format.
Currently the Formatting is straight forward so I didn't add a test for that explicit

…c into the alerts fetcher (where it should be!)

… the data monitoring alerts flow

ellakz · 2023-11-16T08:30:26Z

+            filter=self.filter.get_filter(),
+        )
+
+    def _format_alert(


ok, in that case I would move this method to the pending alerts classes instead of using ifs and isinstance, I think it's cleaner

ellakz · 2023-11-16T08:31:30Z

@@ -0,0 +1,184 @@
+from datetime import datetime


I think pending_alert.py is a more suitable name for this file, no?

ellakz · 2023-11-16T08:39:18Z

+
+
+# Merge dicts attributes into a single list that contains all the values.
+def merge_dicts_attribute(dicts: List[Dict], attribute_key: str) -> List:


Suggested change

def merge_dicts_attribute(dicts: List[Dict], attribute_key: str) -> List:

from itertools import chain

def merge_dicts_attribute(dicts: List[Dict], attribute_key: str) -> List:

attributes = [d.get(attribute_key) for d in dicts]

return list(chain.from_iterable(attributes))

sorry I love python magic so couldn't resist myself, you don't have to use this :)

It will only work if the attributes are lists but we want to support attributes that are single item (like str, int, dict) and merge them into a list as well 🙂
So in order to make it work we will need to have 2 iterations to convert all to lists or have one big inline iteration with inline if statement inside it that uses the .get method on the same item 3 times:

attributes = [ d.get(attribute_key, []) if isinsatnce(d.get(attribute_key, []), list) else [d.get(attribute_key)] for d in dicts ] return list(chain.from_iterable(attributes))

no I checked, it works for both cases actually :)

ellakz · 2023-11-16T09:01:45Z

+# Flatten a nested dict by a given key, for example:
+# nested_dict = {"top1": "one", "nested": {"top1": "One", "top2": "Two"}}, flatten_by_key = "nested" -> {"top1": "One", "top2": "Two"}
+def flatten_dict_by_key(nested_dict: Dict, flatten_by_key: str) -> Dict:
+    flatten_dict = {**nested_dict, **nested_dict.get(flatten_by_key, {})}


pop also returns the value so you can do:

inner_dict = nested_dict.pop(flatten_by_key, {}) return {**nested_dict, **inner_dict}

True, but pop on nested_dict which is a variable of the method will pop out the key from the origin dict as well (outside of the method) which can cause some big voodoo 😱
(this is actually breaking the test, because we run the same dict with different scenarios and this is editing the dict outside of the method calls)

We can do:

def flatten_dict_by_key(nested_dict: Dict, flatten_by_key: str) -> Dict: nested_dict_copy = {**nested_dict} inner_dict = nested_dict_copy.pop(flatten_by_key, {}) return {**nested_dict_copy, **inner_dict}

Which IMO is quite the same with a different order 🙂

I can change to that if you prefer it though

ellakz · 2023-11-16T09:03:19Z

+
+
+def test_get_alert_meta_attrs():
+    base_alert = BasePendingAlertSchema(**{**BASE_ALERT, "model_meta": dict(attr="a")})


you can use Parametrize here :)

ellakz · 2023-11-16T09:10:10Z

+        self.sent_alert_count = 0
+        self.send_test_message_on_success = send_test_message_on_success
+        self.override_meta_slack_channel = override_config
+        self.alerts_integraion = self._get_integration_client()


I want to think about a way to move all its logic to the integrations area, I will think about it when I integrate PD

…s tests

…toringAlers to pending alerts

decoupled alerts from Slack

ad8bda4

IDoneShaveIt added 16 commits November 7, 2023 10:43

Reorderd code

1c3615b

Reorderd code

44fcb2b

Merged master

16df9de

Fixed imports

d250316

Fixed imports

6d4d032

Fix alerts api tests

fc4b96a

Fix alerts tests

166eff8

Merge branch 'master' of github.com:elementary-data/elementary into e…

4a5ceb4

…le-1852-decouple-alerts-from-slack

Fix pending test alerts schema

d009b29

Fixed alerts schemas

9f0b945

Fix bug with strip of Nonw

e493c6e

add integration params to GroupedByTableAlerts + Support grouped aler…

73c6eb6

…ts in client

Fix slack templates

0f56021

Fix slack grouped by table alert format

44c46ea

Fix typing for base integration

e426b65

Fix mypy

76805ec

IDoneShaveIt marked this pull request as ready for review November 9, 2023 09:23

IDoneShaveIt added 2 commits November 13, 2023 10:58

Merge branch 'master' of github.com:elementary-data/elementary into e…

d4601cb

…le-1852-decouple-alerts-from-slack

Add schema tests

dcb30ff

ellakz reviewed Nov 13, 2023

View reviewed changes

IDoneShaveIt added 4 commits November 14, 2023 10:43

Support test runs link at source freshness alerts

46ea464

Fix DataMonitoringAlertsMock

c8d82a6

Improved the use of ResourceType Enum and moved the alerts table logi…

315a295

…c into the alerts fetcher (where it should be!)

Moved get integration params to be part of the integration instead of…

96f9de6

… the data monitoring alerts flow

ellakz approved these changes Nov 16, 2023

View reviewed changes

IDoneShaveIt added 2 commits November 16, 2023 11:41

Moved get_integration_params from BaseIntegration + added integration…

da477a7

…s tests

Fixed Ella CR - cosmetic changes and moved format_alert from DataMoni…

bb8f81c

…toringAlers to pending alerts

IDoneShaveIt merged commit abf61d7 into master Nov 16, 2023

IDoneShaveIt deleted the ele-1852-decouple-alerts-from-slack branch November 16, 2023 13:02



		# Merge dicts attributes into a single list that contains all the values.
		def merge_dicts_attribute(dicts: List[Dict], attribute_key: str) -> List:

-def merge_dicts_attribute(dicts: List[Dict], attribute_key: str) -> List:
+from itertools import chain
+def merge_dicts_attribute(dicts: List[Dict], attribute_key: str) -> List:
+    attributes = [d.get(attribute_key) for d in dicts]
+    return list(chain.from_iterable(attributes))



		def test_get_alert_meta_attrs():
		base_alert = BasePendingAlertSchema({BASE_ALERT, "model_meta": dict(attr="a")})

Conversation

IDoneShaveIt commented Nov 7, 2023

Uh oh!

linear Bot commented Nov 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IDoneShaveIt Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

IDoneShaveIt Nov 16, 2023 •

edited

Loading