Skip to content

feat: allow more structured slack messages#2144

Merged
vchan merged 5 commits intoSQLMesh:mainfrom
plaflamme:better-slack
Feb 20, 2024
Merged

feat: allow more structured slack messages#2144
vchan merged 5 commits intoSQLMesh:mainfrom
plaflamme:better-slack

Conversation

@plaflamme
Copy link
Contributor

This change allows more structure in slack messages.

Specifically, audit failures and general failures are now formatted using a code block. Furthermore, audit failures will show the audit name, model and count failures more prominently.

Here's an example of what an audit failure message should look like

The other thing I'd like to improve is to have the audit's SQL be in the correct dialect. Not sure what the recommended approach for that would be: any guidance would be appreciated.

Slack allows more structured notifications than a console or an email.
This changes the base class so it passes down more information from the
notification to concrete implementations. For slack, it will convert
this information into a more structure message.

For other text-based targets, the anther base class is now available.
@tobymao tobymao requested a review from vchan February 20, 2024 04:17
f"*Model*: `{audit_error.model_name}`",
f"*Count*: `{audit_error.count}`",
),
slack.preformatted_rich_text_block(str(audit_error.query)),
Copy link
Contributor

@izeigerman izeigerman Feb 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing

str(audit_error.query)

You can do:

audit_error.sql(dialect=audit_error.audit.dialect)

If you also want to format the query you can do:

audit_error.sql(dialect=audit_error.model.dialect, pretty=True)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note that audit_error.model is optional, so the None check might be required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@izeigerman thanks for the guidance.

AuditError doesn't seem to hold on to Audit (see here). Should I:

  1. add an audit: Audit field
  2. add a dialect: str field and populate it from audit.dialect
  3. add a dialect: str field and populate it from engine_adapter.dialect
  4. add a dialect: str field and populate it from snapshot.model.dialect

All of these options would happen around here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest the engine_adapter dialect since that would be the dialect used to actually run the audit. Another option would be to stored the rendered audit query in the error, but then you'd lose the flexibility to format it (I guess you could parse it back into a sqlglot expression again).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @vchan

I've added 2 commits:

  • 77461fb which captures the engine adapter's dialect and exposes a method to print the sql statement to a string
  • 5ef9f6e that uses this new feature for the slack message

PTAL

@izeigerman
Copy link
Contributor

@plaflamme this looks great, thank you!

The other thing I'd like to improve is to have the audit's SQL be in the correct dialect. Not sure what the recommended approach for that would be: any guidance would be appreciated.

Left a comment with the explanation.

Also adds a `sql` helper method on `AuditError` to obtain the sql
statement.
Copy link
Contributor

@vchan vchan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@vchan vchan merged commit f2264d1 into SQLMesh:main Feb 20, 2024
@plaflamme plaflamme deleted the better-slack branch February 20, 2024 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants