Introduce the possibility to send elementary alerts to Microsoft Teams using an incoming webhook in Teams

[FrankTub]

dbt Test

When incorporated into your project you can run something like:

dbt test ...
edr monitor  --teams-webhook <TEAMS_WEBHOOK>

For a normal dbt test the result in Teams looks like:

Note: I took the liberty to format the test results to a markdown table and I think that is quite usefull for end users to read it much easier. Think something similar could be done for slack? Have no way of trying it out so did not pursue this.

Grouped by table

Running something like:

dbt test ...
edr monitor  --teams-webhook <TEAMS_WEBHOOK> --group-by table

Results in:

Report link

I noticed that you guys were able to directly link an alert to the UI. Have not tested it end to end but think it works. What I have done is added a Potential Action in teams, where a user can directly navigate based on a button to the UI.

This is done with something like:

dbt test ...
edr monitor  --teams-webhook <TEAMS_WEBHOOK> --report-link <REPORT_LINK>

The results look like:

Clicking on the button links me to <REPORT_LINK>/report/test-runs/test.beequip_etl_dbt.unique_organizations_organization_no_sk.a65d0992b0.organization_no_sk.generic/. Which I think is correct. Have not set it up all the way so not verified if this is working as expected.

Opening the details and it looks just as expected:

Edge cases

Teams has a maximum number of characters that you can send to the webhook before it fails to display your results. It however still returns a 200 return code.. The maximum number of characters that can be applied in your payload (the message that will be sent to teams) is somewhere between 18000 and 40000, see this for more info. The resulting message looks like:

It is adviced to lower test_sample_row_count in your dbt_project.yml if you run into such issues.

Multiple channels

The functionality to send to multiple channels with different webhooks is not (directly) configurable per model.

I've tested the following scenario of code and that sents everything to the correct channel in Teams.

dbt test ...
edr monitor  --select config.meta.owner:@accounting --teams-webhook <TEAMS_WEBHOOK>
edr monitor  --select config.meta.owner:@data --teams-webhook <ANOTHER_TEAMS_WEBHOOK>

These are both sent to different channels as expected 😄

Webhook config file

It is also possible to set the teams webhook in the config file ~/.edr/config.yml. If you set it up like:

teams:
  teams_webhook: <TEAMS_WEBHOOK>

You can now run the code like:

dbt test ...
edr monitor

Known Issue

In the slack alerting solution there is the option to set on a model or dbt project level the fields that will be displayed in the alerts. This does not work in the team alerting part of the code.

For example if you set something in your model like below:

- version: 2
- 
- models:
-   - name: lease_contracts
-    meta:
-      alert_fields: ["description", "owners"]

And debugging the code it shows on run time 2024-02-04 11:33:56 — INFO — This is the value of alert.alert_fields: []. The resulting alert in Teams contains all the fields. Will have to look into it in another PR as discussed with @ellakz

Potential enhancement

I think it would be really cool if we could mention an owner / subscriber in Teams. I have taken a look at this issue in pymsteams and there is already some sort of hacky wacky workaround available. A possibility would be to create your own class in the elementary repo that modifies the payload of pymsteams and build a proper solution for mentions.

[FrankTub]

Found out that the issue with the alert_fields was due to a variable set in my dbt_project.yml:

vars:
  test_sample_row_count: 10
  disable_dbt_artifacts_autoupload: "{{ target.name != 'production' }}"
  disable_dbt_invocation_autoupload: "{{ target.name != 'production' }}"

Disabling this to:

vars:
  test_sample_row_count: 10
  # disable_dbt_artifacts_autoupload: "{{ target.name != 'production' }}"
  # disable_dbt_invocation_autoupload: "{{ target.name != 'production' }}"

And the resulting alert_fields are correctly displayed in the alert:

[FrankTub]

Also enforced that the behavior that passing in a webhook in the CLI takes precedence over the ~/.edr/config.yml. If you pass in both on the CLI you will get an error. See this for the changes.

[ellakz]

Looks like there should be 2 classes here rather than inheritance - one for building messages (TeamsMessageBuilder) and one for interacting with the API (TeamsWebhookClient), but what do you get from having the inheritance here?

[FrankTub]

The way pymsteams is setup is that there is no way to create a message without having a client connection. This does not allow for a similar setup as is done in Slack. This was the only way I could get it to work for now.

The ideal solution would have a TeamsMessageBuilder class that directly modifies the payload, a good starting place is the pymsteams code.

Since this was a MVP I still think this works good enough.

[ellakz]

Yeah I understand, still don't you think it makes more sense that way:

class TeamsWebhookMessageBuilder(ABC):
    def __init__(client: TeamsWebhookClient):
        self.client = client

?

[FrankTub]

Aah now I understand what you propose, yeah I think that it makes more sense to do it in that way. Think that it is more neat. Will do that, but probably tomorrow

[FrankTub]

I've been struggling with it for some time now. And now I'm starting to doubt if it makes sense to have a seperate class to create the message on at all.

Now I think about it for some time, I would like to suggest to only create a class TeamsClient, and this class does handle the connection to teams and sending the message, as well as creating the json payload. This is completely in line with pymsteams and leads to the least amount of redundant code.

Do you agree with this?

[IDoneShaveIt]

Hey @FrankTub, chiming in here 🙂
I saw this thread only now after I commented on that part as well.
I see the problem you are raising. Another approach could be to create a TeamsMessageBuilder class that is getting the client / connectcard as a param and return a connectcard.
That way you can build everything in the message builder scope, and send the connectcard using the client.
I know this could sound a bit overkill, but the idea of the message builder is that you can create different ones for the same integration and control the messages templates behaviours differently and easily.

In case this is not working well and problematic to do, I think that at least we should move the title / text / etc methods into the TeamsWebhookClient class and remove them from the abstract TeamsClient class as they might not be relevant for the other clients (if the will we will add them back to the abstract one) 🙂

[FrankTub]

@IDoneShaveIt regarding:

I see the problem you are raising. Another approach could be to create a TeamsMessageBuilder class that is getting the client / connectcard as a param and return a connectcard.

I think this leads to a lot of duplicated code. Since you need to add the functions of pymsteams in the TeamsClient class, as well as in the TeamsMessageBuilder class. And basicly it is all just copy pasting code of what pymsteams already does for you.

Regarding:

In case this is not working well and problematic to do, I think that at least we should move the title / text / etc methods into the TeamsWebhookClient class and remove them from the abstract TeamsClient class as they might not be relevant for the other clients (if the will we will add them back to the abstract one)

I implemented it in that way initially, however mypy does not approve.

Proposal

The way I see it we could do two things and would like your opinion what you think is best to do.

1. Remove all TeamsClient classes and just use pymsteams where the TeamsClient was used up till this point. This leads to the least amount of redundant code, but is not in line with how the rest of the code base.
2. Just have a TeamsWebhookMessageBuilderClient class without a TeamsClient. This is just a wrapper around pymsteams and makes it more similar to the way the Slack client is working.

It would be something like:

class TeamsWebhookMessageBuilderClient(ABC):
    def __init__(
        self,
        webhook: str,
        tracking: Optional[Tracking] = None,
    ):
        self.webhook = webhook
        self.tracking = tracking
        self.client = self._initial_client()

    @staticmethod
    def create_client(
        config: Config, tracking: Optional[Tracking] = None
    ) -> Optional["TeamsWebhookMessageBuilderClient"]:
        if not config.has_teams:
            return None
        if config.teams_webhook:
            return TeamsWebhookMessageBuilderClient(
                webhook=config.teams_webhook, tracking=tracking
            )
        return None

    @retry(tries=3, delay=1, backoff=2, max_delay=5)
    def send_message(self, **kwargs) -> bool:
        self.client.send()
        response = self.client.last_http_response

        if response.status_code == OK_STATUS_CODE:
            return True
        else:
            logger.error(
                "Could not post message to teams via webhook - %s. Error: %s",
                {self.webhook},
                {response.body},
            )
        return False

    def _initial_client(self):
        return connectorcard(self.webhook)

    def title(self, title: str):
        self.client.title(title)

    def text(self, text: str):
        self.client.text(text)

    def addSection(self, section: cardsection):
        self.client.addSection(section)

    def addPotentialAction(self, action: potentialaction):
        self.client.addPotentialAction(action)

Would like to know your view on this! 🙏

[IDoneShaveIt]

I think this leads to a lot of duplicated code. Since you need to add the functions of pymsteams in the TeamsClient class, as well as in the TeamsMessageBuilder class. And basicly it is all just copy pasting code of what pymsteams already does for you.

You don't have to create title / text / etc at the TeamsClient. Because you pass the TeamsClient to the message builder (or a connectcard) you can just do client/connectcard.text for example.
That way you can create all the title / text / etc methods only at the message builder.

Remove all TeamsClient classes and just use pymsteams where the TeamsClient was used up till this point. This leads to the least amount of redundant code, but is not in line with how the rest of the code base.

The reason I do want to have a TeamsClient is that in case teams changes their sdk it will contain all the changes we need to do in one place (instead across the platform). Even if it just a wrapper for pymsteams, it makes it more maintainable 🙂

Just have a TeamsWebhookMessageBuilderClient class without a TeamsClient. This is just a wrapper around pymsteams and makes it more similar to the way the Slack client is working.

In that case I prefer having TeamsClient and TeamsWebhookClient and skip the TeamsWebhookMessageBuilder. It will look the same as the current implementation, but with a different name 🙂

If it sounds valid to you, I think that passing the connectcard to the message builder is better, but we can go with the second suggestion as well and skip the TeamsWebhookMessageBuilder for now 🙂

[FrankTub]

@IDoneShaveIt I got it working as per your preferred way. Created a seperate TeamsAlertMessageBuilder. The thing I don't like about it is that in the client TeamsClient and TeamsWebhookClient it is required to define the methods to modify your message as well. That being said I think it makes more sense this way.

Curious to hear what you think about it!

[IDoneShaveIt]

Thank you for this awesome Teams integration contribution!
Really nice job done here!
I left some comments (mostly regarding the structure of the code) - Feel free to reach out and ask anything 🙂

[IDoneShaveIt]

pretty similar to the previous implementation of has_slack.
I think we can keep has_slack as it was, and make sure that has_teams only check for teams_vebhook.

The main reason for that is that in the future we will have X integrations supported, so start making sure that we only pass one at each has_<something> will be a bit messy.

[FrankTub]

I understand your point, however the intended behavior is that if you pass something to the command line it should take precedence over what you have in ~edr.config.yml. So in my opinion if you pass one webhook to the CLI and in your config you have a slack webhook the command should run just fine. In the proposed solution it raises an error I think.

Might be that we need to rewrite it a bit, but don't see any other way for now. If you have a suggestion please let me know.

[FrankTub]

@IDoneShaveIt , think it could be argued that you just should not supply multiple types of integration methods. In that case you also don't have the issue with precedence of the CLI over the config file.

Could you let me know if you agree and then I will apply you suggestions?

[IDoneShaveIt]

@FrankTub I think is fair enough to assume and guide the users to use config / CLI args and not both.
There is some work that could be done around the connection between the two regardless this contribution.
For now I think that leaving things as simple as possible is the way to go, and if someone will provide 2 integrations (one from the config and the other from the CLI args) he will just get an error 🙂

So I think we can apply the suggestions for now 🙂

[FrankTub]

I implemented the suggestions. Let me know what you think of the Config class in this way!

[IDoneShaveIt]

Because TeamsClient is an abstract class that is used to generate the right teams client (currently only webhook is supported as you know 😉), the send_message method should be an abstractmethod and be implemented at the client itself (TeamsWebhookClient) because each client might have a different implementation for this method 🙂

You can see Slack clients for more reference

[FrankTub]

You are absolutely right! Have updated this. Just like the way the _initial_client should also be an abstractclass here. Updated them both!

[IDoneShaveIt]

Suggested change

	logger.error(e)
	logger.error(
	f"Unable to send alert via Teams: {e}\nSending fallback template."
	)

To make it a bit clearer for the user and the support 🙂

[FrankTub]

Nice suggestion! Is added

[IDoneShaveIt]

Lets add here a logger as well 🙂
logger.error(f"Unable to send alert fallback via Teams: {e}")

[FrankTub]

Nice suggestion! Is added

[IDoneShaveIt]

Looking great!
Thanks again for this awesome contribution 😎

[FrankTub]

Create some follow up tickets on this subject to enhance it a bit more. Created #1404 and #1405.