Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EHPR-117 | Slack Alerting #124

Merged
merged 27 commits into from
Jan 7, 2022
Merged

EHPR-117 | Slack Alerting #124

merged 27 commits into from
Jan 7, 2022

Conversation

arranfw
Copy link
Contributor

@arranfw arranfw commented Jan 5, 2022

  • Winston error logging sends to Slack
  • Sample error endpoint
  • Tested console + slack for regular, HTTP, axios, and validation exceptions
  • Improved some error handling
  • Error logger assumes you're passing an error object

image
![image](https://user-images.githubusercontent.com/71518072/148295052-744a024e-a8ac-4e91-bf4a-76cffe92bfcb.png
image
image-
image
image

image

@arranfw arranfw marked this pull request as draft January 5, 2022 19:24
Base automatically changed from EHPR-184-Logging-API-Validations to main January 5, 2022 22:23
@arranfw arranfw marked this pull request as ready for review January 5, 2022 23:03
@arranfw arranfw temporarily deployed to dev January 6, 2022 16:04 Inactive
@arranfw arranfw temporarily deployed to dev January 6, 2022 16:09 Inactive
@arranfw arranfw temporarily deployed to dev January 6, 2022 16:55 Inactive
@arranfw arranfw temporarily deployed to dev January 6, 2022 16:55 Inactive
@arranfw arranfw temporarily deployed to dev January 6, 2022 17:23 Inactive
@arranfw arranfw temporarily deployed to dev January 6, 2022 17:24 Inactive
@arranfw arranfw temporarily deployed to dev January 6, 2022 17:49 Inactive
@arranfw arranfw temporarily deployed to dev January 6, 2022 17:49 Inactive
@arranfw arranfw temporarily deployed to dev January 6, 2022 20:27 Inactive
@arranfw arranfw temporarily deployed to dev January 6, 2022 21:19 Inactive
Copy link
Contributor

@fwpushan fwpushan left a comment

Choose a reason for hiding this comment

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

Please check my comments.

.github/workflows/deploy-dev.yml Outdated Show resolved Hide resolved
apps/api/src/common/slack.ts Show resolved Hide resolved
apps/api/src/app.controller.ts Outdated Show resolved Hide resolved
apps/api/src/common/logger.service.ts Show resolved Hide resolved
apps/api/src/mail/mail.service.ts Show resolved Hide resolved
@arranfw arranfw temporarily deployed to dev January 7, 2022 18:24 Inactive
@arranfw arranfw temporarily deployed to dev January 7, 2022 19:46 Inactive
message = {
url: config.url,
method: config.method,
...(response?.data ? { data: response.data } : {}),
Copy link
Contributor

Choose a reason for hiding this comment

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

what if data contains any sensitive info or PI?

Copy link
Contributor Author

@arranfw arranfw Jan 7, 2022

Choose a reason for hiding this comment

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

This response would be what the external API is returning as error data so it shouldn't have PII
In this implementation the logger isn't responsible for filtering

We've added a bit of filtering in the error filter to prevent logging of the submission payload
We should also be diligent in not explicitly logging PII through info/warn methods

Copy link
Contributor

@fwpushan fwpushan left a comment

Choose a reason for hiding this comment

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

LGTM just one comment

@arranfw arranfw merged commit a52d40b into main Jan 7, 2022
@arranfw arranfw deleted the EHPR-117-Alerting branch January 7, 2022 20:07
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.

3 participants