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

chore: remove instrumentation for email performance monitoring and SES migration #5766

Merged
merged 3 commits into from
Feb 14, 2023

Conversation

justynoh
Copy link
Contributor

@justynoh justynoh commented Feb 14, 2023

Problem

We created additional instrumentation over our mailing system in #5019 and #5369 and #5442. This PR tears them down, since they are not needed anymore.

Closes https://github.com/opengovsg/formsg-private/issues/130 and closes https://github.com/opengovsg/formsg-private/issues/132.

Cleaning up old SES config will also close https://github.com/opengovsg/formsg-private/issues/91.

Solution

Delete code that references datadog traces and SES in the US.

Breaking Changes

  • No - this PR is backwards compatible

Tests

  • OTP emails can be sent to admins to log in.
  • Email responses can be sent to admins after submitting an email mode form.
  • OTP emails can be sent to respondents when verifying an email in a public form.
  • Confirmation emails can be sent to respondents when submitting and email mode form.

Deploy Notes

New environment variables:

for x = HOST, PORT, USER, PASS:

  • SES_{x} : Should be the value of the old variable SES_{x}_SG
  • SES_{x}_US and SES_{x}_SG: Should be deleted

@justynoh
Copy link
Contributor Author

(This PR must be released before we tear down the infra for SES US)

@justynoh justynoh force-pushed the chore/remove-mail-tracing branch from 5e9181b to c2a5f1e Compare February 14, 2023 05:13
Copy link
Contributor

@timotheeg timotheeg left a comment

Choose a reason for hiding this comment

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

Looking good!

Unless the tracing of the email sending is costing a lot (I assume no right now), let's keep them in for now (traces for email sending + pdf generation), it gives us visibility in processes we are otherwise blind to.

@justynoh justynoh requested a review from timotheeg February 14, 2023 07:17
@justynoh justynoh temporarily deployed to staging-al2 February 14, 2023 07:20 — with GitHub Actions Inactive
@justynoh justynoh force-pushed the chore/remove-mail-tracing branch from 99e4898 to 579c351 Compare February 14, 2023 07:22
@justynoh justynoh temporarily deployed to staging-al2 February 14, 2023 07:23 — with GitHub Actions Inactive
@@ -1,4 +1,4 @@
import { tracer } from 'dd-trace'
import tracer from 'dd-trace'
Copy link
Contributor

Choose a reason for hiding this comment

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

is this change intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops no, but it shouldn't make a difference! it was probably introduced when I reinstated the tracer spans. But it still works, tested on staging + the default import was already used in mail.utils.ts before

Copy link
Contributor

@timotheeg timotheeg left a comment

Choose a reason for hiding this comment

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

LGTM

@justynoh justynoh merged commit 2708059 into develop Feb 14, 2023
@justynoh justynoh deleted the chore/remove-mail-tracing branch February 14, 2023 11:54
@justynoh justynoh mentioned this pull request Feb 15, 2023
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.

2 participants