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] migrate legacy alert apis #107863

Conversation

dominiqueclarke
Copy link
Contributor

@dominiqueclarke dominiqueclarke commented Aug 9, 2021

Summary

Migrates away to the updated Rule APIs

These APIs are responsible for 4 functions in Uptime, all of which should be tested for regression.

  1. Creating and delete the default monitor status rule in the status alert toggle

Screen Shot 2021-08-09 at 1 34 49 PM

2. Display currently enabled rules

image

  1. Displaying connector options

Screen Shot 2021-08-09 at 1 35 37 PM

4. Displaying available configured connectors

Screen Shot 2021-08-09 at 1 35 44 PM

Fixes #94089
Fixes #95839

@dominiqueclarke dominiqueclarke added enhancement New value added to drive a business result v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v7.15.0 labels Aug 9, 2021
@dominiqueclarke dominiqueclarke requested review from a team as code owners August 9, 2021 02:50
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@dominiqueclarke dominiqueclarke marked this pull request as draft August 9, 2021 02:50
const response = (await apiService.get(API_URLS.RULE_CONNECTORS)) as Array<
AsApiContract<ActionConnector>
>;
/* eslint-disable @typescript-eslint/naming-convention */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@justinkambic Any better ideas for how to handle this?

@dominiqueclarke dominiqueclarke marked this pull request as ready for review August 9, 2021 17:44
Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

LGTM!

@dominiqueclarke
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@justinkambic justinkambic left a comment

Choose a reason for hiding this comment

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

Looking pretty good. Not approving yet as I've only done code review so far; I will post another review with my smoke-test results.

Comment on lines 27 to 37
/* eslint-disable @typescript-eslint/naming-convention */
return response.map(
({ connector_type_id, referenced_by_count, is_preconfigured, is_missing_secrets, ...res }) => ({
...res,
actionTypeId: connector_type_id,
referencedByCount: referenced_by_count,
isPreconfigured: is_preconfigured,
isMissingSecrets: is_missing_secrets,
})
);
/* eslint-enable @typescript-eslint/naming-convention */
Copy link
Contributor

@justinkambic justinkambic Aug 12, 2021

Choose a reason for hiding this comment

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

How about we do this instead, we can make the return object a little cleaner and it avoids the eslint messiness in exchange for some added verbiage in the parameter destructuring.

Suggested change
/* eslint-disable @typescript-eslint/naming-convention */
return response.map(
({ connector_type_id, referenced_by_count, is_preconfigured, is_missing_secrets, ...res }) => ({
...res,
actionTypeId: connector_type_id,
referencedByCount: referenced_by_count,
isPreconfigured: is_preconfigured,
isMissingSecrets: is_missing_secrets,
})
);
/* eslint-enable @typescript-eslint/naming-convention */
return response.map(
({
connector_type_id: actionTypeId,
referenced_by_count: referencedByCount,
is_preconfigured: isPreconfigured,
is_missing_secrets: isMissingSecrets,
...res
}) => ({
...res,
actionTypeId,
referencedByCount,
isPreconfigured,
isMissingSecrets,
})
);

Comment on lines 138 to 152
/* eslint-disable @typescript-eslint/naming-convention */
return response.map<ActionType>(
({
enabled_in_config,
enabled_in_license,
minimum_license_required,
...res
}: AsApiContract<ActionType>) => ({
...res,
enabledInConfig: enabled_in_config,
enabledInLicense: enabled_in_license,
minimumLicenseRequired: minimum_license_required,
})
);
/* eslint-enable @typescript-eslint/naming-convention */
Copy link
Contributor

Choose a reason for hiding this comment

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

Same recommendation here; this allows us to remove the eslint-disable.

Suggested change
/* eslint-disable @typescript-eslint/naming-convention */
return response.map<ActionType>(
({
enabled_in_config,
enabled_in_license,
minimum_license_required,
...res
}: AsApiContract<ActionType>) => ({
...res,
enabledInConfig: enabled_in_config,
enabledInLicense: enabled_in_license,
minimumLicenseRequired: minimum_license_required,
})
);
/* eslint-enable @typescript-eslint/naming-convention */
return response.map<ActionType>(
({
enabled_in_config: enabledInConfig,
enabled_in_license: enabledInLicense,
minimum_license_required: minimumLicenseRequired,
...res
}: AsApiContract<ActionType>) => ({
...res,
enabledInConfig,
enabledInLicense,
minimumLicenseRequired,
})
);

Copy link
Contributor

@justinkambic justinkambic left a comment

Choose a reason for hiding this comment

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

Smoke test results

Connectors are displayed

image

Defined connectors displayed:

image

Create simple monitor alert:

image

Enabled rules:

image

Everything's looking good on my side.

@dominiqueclarke
Copy link
Contributor Author

@elasticmachine merge upstream

@dominiqueclarke
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
uptime 954.5KB 955.0KB +534.0B
Unknown metric groups

API count

id before after diff
triggersActionsUi 237 238 +1

API count missing comments

id before after diff
triggersActionsUi 228 229 +1

History

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

Copy link
Contributor

@justinkambic justinkambic left a comment

Choose a reason for hiding this comment

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

LGTM

@dominiqueclarke dominiqueclarke merged commit c5ec546 into elastic:master Aug 16, 2021
@dominiqueclarke dominiqueclarke deleted the feature/94089-uptime-migrate-legacy-alert-apis branch August 16, 2021 18:02
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Aug 16, 2021
* uptime - migrate legacy alert apis

* update alert fetch actions

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Aug 16, 2021
* uptime - migrate legacy alert apis

* update alert fetch actions

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Dominique Clarke <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed enhancement New value added to drive a business result release_note:skip Skip the PR/issue when compiling release notes Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate Uptime to new Alerting APIs Migrate Uptime to new Connectors APIs in place of deprecated APIs
5 participants