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

add types for status alert #713

Merged
merged 5 commits into from
Mar 20, 2023
Merged

add types for status alert #713

merged 5 commits into from
Mar 20, 2023

Conversation

shahzad31
Copy link
Contributor

@shahzad31 shahzad31 commented Mar 6, 2023

add types for status alert

status alert can be enabled/disable using


    monitor.use({
      schedule: 10,
      alert: {
        status: {
          enabled: false,
        },
      },
    });

in lightweight:


heartbeat.monitors:
- type: http
  name: Todos Lightweight
  id: todos-lightweight
  enabled: true
  urls: "https://elastic.github.io/synthetics-demo/"
  schedule: '@every 3m'
  timeout: 16s
  alert.status.enabled: true

@apmmachine
Copy link

apmmachine commented Mar 6, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-03-20T17:01:45.723+0000

  • Duration: 14 min 43 sec

Test stats 🧪

Test Results
Failed 0
Passed 214
Skipped 2
Total 216

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

Copy link
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

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

Probably this is a bit of a different usage case, Do you foresee a future where we might want to disable individual alerts for monitor instead of just disabling them all which is what the PR is doing.

So its of the type here it would be like alert: []Alerts for a given monitor. /cc @dominiqueclarke

@@ -24,6 +24,7 @@
*/

import { mkdir, rm, readFile } from 'fs/promises';
import get from 'lodash/get';
Copy link
Member

Choose a reason for hiding this comment

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

not used.

@dominiqueclarke
Copy link
Contributor

Probably this is a bit of a different usage case, Do you foresee a future where we might want to disable individual alerts for monitor instead of just disabling them all which is what the PR is doing.

So its of the type here it would be like alert: []Alerts for a given monitor. /cc @dominiqueclarke

I think we envision the type being an object representing the different alerts, rather than an array, which we can add to later. So let's say we added a tls alert, the type would be

alerts: {
   status: {
       enabled: boolean
   },
   tls: {
       enabled: boolean
   },
}

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 but you may want to add a test.

Also, and this isn't a blocker for me, it doesn't support nested yaml for lightweight monitors, only alert.status.enabled

@shahzad31 shahzad31 merged commit 8f617ac into main Mar 20, 2023
@shahzad31 shahzad31 deleted the add-alerting-option branch March 20, 2023 17:38
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