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

[Uptime] Default alert connectors email settings #123244

Merged
merged 23 commits into from
Jan 28, 2022

Conversation

shahzad31
Copy link
Contributor

@shahzad31 shahzad31 commented Jan 18, 2022

Summary

Fixes #116362
Fixes #121098

Added ability to select email connectors in uptime alert connectors settings.

When email connector is selected. Section to add default email appears. Where user can select, To, Cc, Bcc emails.

Those emails will be used to send alerts via selected email connectors in the uptime page.

image

Testing

  • Add an email connector using rule and management page or from the uptime setting page using Add connector button
  • Configure email connector via https://nodemailer.com/app/ , using this app you can start a dummy email server and specify settings in the email connector like this
    host: 'localhost',
    port: 1025,
    auth: {
        user: 'project.1',
        pass: 'secret.1'
    }

  • Once email connector is configure you can test it within the connector UI to make sure it works and sends an email, that email will appear in nodemailer app
  • Select that connector in the uptime setting default alert connectors
  • Fields to add To, Cc, Bcc should appear in the settings page
  • If any email connectors is selected To becomes a required field.
  • All email fields should check for valid emails

Note

Unit testing this component was hard, so i opted for more streamlined e2e tests.

@shahzad31 shahzad31 changed the title wip [Uptime] Default alert connectors email settings Jan 19, 2022
@shahzad31 shahzad31 marked this pull request as ready for review January 20, 2022 09:07
@shahzad31 shahzad31 requested a review from a team as a code owner January 20, 2022 09:07
@shahzad31 shahzad31 added release_note:enhancement v8.1.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Jan 20, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

description={
<FormattedMessage
id="xpack.uptime.sourceConfiguration.defaultConnectors.description.defaultEmail"
defaultMessage="Email settings required for selected email alert connectors."
Copy link
Contributor

Choose a reason for hiding this comment

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

Might need to adjust this content. Should connectors be plural? Isn't there just one email connector?

x-pack/plugins/uptime/public/pages/settings.tsx Outdated Show resolved Hide resolved
@dominiqueclarke dominiqueclarke self-requested a review January 20, 2022 17:23
Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

The page is crashing for me when I visit settings. Was on commit 103bf0c

@shahzad31
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

Screen Shot 2022-01-25 at 2 47 11 PM

Styles for the add connector button are a bit off. Default connectors and Add Connector isn't aligned, and the button is missing padding.

Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

I don't see the cc or bcc option for email connector when using the email connector from the alert flyout in Uptime or the Rules and Connectors page

Screen Shot 2022-01-25 at 3 46 06 PM

Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

LGTM. Receiving emails for simple alerts and regular alerts as expected.
Screen Shot 2022-01-27 at 10 53 58 AM

@dominiqueclarke dominiqueclarke self-requested a review January 27, 2022 16:37
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
uptime 690 694 +4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
triggersActionsUi 674.8KB 675.1KB +270.0B
uptime 704.2KB 709.3KB +5.1KB
total +5.4KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
uptime 24.9KB 25.1KB +200.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@paulb-elastic
Copy link
Contributor

The UI looks good to me (the ability to add the email connector as the default, and not being able to Apply changes until an email address is added)

image

@shahzad31 shahzad31 merged commit 957b9dc into elastic:main Jan 28, 2022
@shahzad31 shahzad31 deleted the email-connector branch January 28, 2022 11:15
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jan 28, 2022
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jan 28, 2022
…fix-potential-race-condition-when-screenshotting

* 'main' of github.com:elastic/kibana: (75 commits)
  [Reporting] Logging improvements while generating reports (elastic#123802)
  [Uptime] Default alert connectors email settings (elastic#123244)
  Update comparison series styles to match the main series (elastic#123858)
  [RAC][Uptime] remove extra dot from the uptime alert connector message (elastic#124000)
  [Exploratory view] Allow ability add extra actions in lens embeddable (elastic#123713)
  [SecuritySolution][Investigations] Add message about missing index in data view in analyzer (elastic#122859)
  [TSVB] Formatting in the left axis is not respected when I have two separate axis (elastic#123903)
  [Discover] Remove services from component dependencies (elastic#121691)
  Stop IM rule execution if there are no events (elastic#123811)
  [Security Solution][Endpoint] Update Fleet Trusted Apps and Host Isolation Exception cards to use exception list summary API (elastic#123900)
  [Security Solution][Exceptions] Switches modal to flyout component (elastic#123408)
  [Workplace Search] Fix bug where modal visible after deleting a group (elastic#123976)
  [Alerting] Remove state variables from action variable menu (elastic#123702)
  replace deprecated api usage (elastic#123970)
  Fix package policy merge logic for boolean values (elastic#123974)
  [Security Solution][Endpoint][Policy] Remove GET policy list api route (elastic#123873)
  Reenable alert_add test suite (elastic#123862)
  [Fleet] Remove usage of IFieldType in Fleet (elastic#123960)
  [Lists] Add an instance of `ExceptionListClient` with server extension points turned off to context object provided to callbacks (elastic#123885)
  [Maps] Add execution context (elastic#123651)
  ...

# Conflicts:
#	x-pack/plugins/screenshotting/server/browsers/chromium/driver_factory/index.ts
awahab07 pushed a commit to awahab07/kibana that referenced this pull request Jan 31, 2022
Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

Response Ops changes LGTM. Verified that email params still work as expected within the Rules Management UI

@lucasfcosta
Copy link
Contributor

lucasfcosta commented Feb 9, 2022

I've just tested this, and I've got a few comments and issues to highlight.

Testing the default connector settings

As @shahzad31 mentioned, I have:

  1. Downloaded NodeMailer and started an SMTP server with it
  2. Configured an email connector in Stack Management > Rules and Connectors
    Screenshot 2022-02-09 at 16 59 00
  3. Then, on the settings within Uptime, I've configured settings for a default connectors with all fields, trying to check whether the required and non-required fields affected the save button correctly, as shown in the GIF below.
    connectorsetup

Once I enabled alerts for a monitor I've created and made it fail, I also checked I did receive email alerts on the configured default connector ✅

➡️ Open Issue

As you can see above, the screen actually flashes once I select the email connector, and I can consistently reproduce it by just refreshing the page. I've created an issue for that here: #125124.

Creating a particular monitor alert

Once NodeMailer was setup, as per the test above, I've also tried to create a particular monitor alert, and I correctly got emails for that.

Screenshot 2022-02-09 at 16 54 59

Screenshot 2022-02-09 at 17 04 02

➡️ Open Issue

Typing into the field for the rule name is extremely slow. Whenever I type, I have to wait quite a bit for each character to appear in the input. It seems like there are a bunch of keydown callbacks making this experience slow.

It's currently a very poor experience as you can see below, so I've created an issue for that here: #125123.

slowkeydown

Permissions for viewers

It seems like #121098 is also solved.

As you can see below, when logging-in with a read-only user (one with the viewer role, for example), I can't click to create a connector anymore, and thus don't see the aforementioned issue.

Screenshot 2022-02-09 at 18 04 47

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:enhancement Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.1.0
Projects
None yet
8 participants