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

Detect rule param changes for rolling upgrades and rollback assessment #173936

Merged
merged 24 commits into from
Feb 5, 2024

Conversation

mikecote
Copy link
Contributor

@mikecote mikecote commented Dec 22, 2023

In this PR, I'm adding a test in the alerting framework to detect changes in a rule type's params schema that will require a snapshot to be updated. This snapshot will provide a centralized place to view history on alerting rule params in case we need to asses risk for rolling upgrades or rollbacks of a release (serverless). The only rule types affected are those running in serverless in any of the three project types.

When a rule type is used in serverless, it must provide one of the following configuration to their rule type on top of everything else:

// Zod schema
schemas: {
  params: {
    type: 'zod',
    schema: UnifiedQueryRuleParams
  },
},

// config-schema
schemas: {
  params: {
    type: 'config-schema',
    schema: EsQueryRuleParamsSchema,
  },
},

We are working on documenting guidelines so engineers and response ops can ensure a change to rule parameters will work properly in rolling upgrade and rollback scenarios and be part of the PR review process.

NOTE to rule type owners: I pass the same schema used to validate to the schemas.params attribute in the rule type. It will be important to keep them in sync. Down the road, we plan to make validate.params optional and use the schema as a starting point so it's easier to have a single variable passed in.

To verify

  1. Make changes to the params schema of the ES query rule type.
diff --git a/x-pack/plugins/stack_alerts/server/rule_types/es_query/rule_type_params.ts b/x-pack/plugins/stack_alerts/server/rule_types/es_query/rule_type_params.ts
index 73e8eae32cf..09ec74104ec 100644
--- a/x-pack/plugins/stack_alerts/server/rule_types/es_query/rule_type_params.ts
+++ b/x-pack/plugins/stack_alerts/server/rule_types/es_query/rule_type_params.ts
@@ -39,6 +39,7 @@ export type EsQueryRuleParamsExtractedParams = Omit<EsQueryRuleParams, 'searchCo
 };

 const EsQueryRuleParamsSchemaProperties = {
+  foo: schema.boolean(),
   size: schema.number({ min: 0, max: ES_QUERY_MAX_HITS_PER_EXECUTION }),
   timeWindowSize: schema.number({ min: 1 }),
   excludeHitsFromPreviousRun: schema.boolean({ defaultValue: true }),
  1. Run the jest integration test to update the snapshot file
node scripts/jest_integration.js x-pack/plugins/alerting/server/integration_tests/serverless_upgrade_and_rollback_checks.test.ts -u
  1. Notice the x-pack/plugins/alerting/server/integration_tests/__snapshots__/serverless_upgrade_and_rollback_checks.test.ts.snap file got updated
    "foo": Object {
      "flags": Object {
        "error": [Function],
      },
      "type": "boolean",
    },

@mikecote mikecote added Feature:Alerting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Dec 22, 2023
@mikecote mikecote force-pushed the poc/detect-rule-param-changes branch from 69d8613 to bc465a3 Compare December 27, 2023 15:21
@mikecote mikecote force-pushed the poc/detect-rule-param-changes branch from 97a94c9 to 7c3b37f Compare December 28, 2023 13:56
@mikecote
Copy link
Contributor Author

mikecote commented Jan 5, 2024

/ci

@mikecote
Copy link
Contributor Author

mikecote commented Jan 5, 2024

@elasticmachine merge upstream

@mikecote
Copy link
Contributor Author

mikecote commented Jan 5, 2024

/ci

@mikecote
Copy link
Contributor Author

mikecote commented Jan 5, 2024

/ci

@mikecote
Copy link
Contributor Author

mikecote commented Jan 9, 2024

/ci

@mikecote
Copy link
Contributor Author

/ci

@mikecote mikecote requested a review from a team as a code owner January 17, 2024 00:09
@mikecote mikecote requested a review from rylnd January 17, 2024 00:09
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@mikecote
Copy link
Contributor Author

@elasticmachine merge upstream

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

@botelastic botelastic bot added Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team Team:obs-ux-management Observability Management User Experience Team labels Jan 17, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-management-team (Team:obs-ux-management)

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

kibana-gis changes LGTM
code review only

Copy link
Contributor

@rylnd rylnd left a comment

Choose a reason for hiding this comment

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

I reviewed changes to Detection Engine code. Both the referenced schema and their associated types look correct, as well as the relevant snapshot tests. LGTM!

@jennypavlova jennypavlova self-requested a review January 22, 2024 09:37
Copy link
Member

@jennypavlova jennypavlova left a comment

Choose a reason for hiding this comment

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

Obs UX changes LGTM 💯 I checked the rules/alerts locally and tested it as described.

Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

ML and Transform changes LGTM

@simianhacker simianhacker self-requested a review February 1, 2024 16:43
@mikecote mikecote enabled auto-merge (squash) February 5, 2024 19:53
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #70 / Reporting APIs CSV Generation from SearchSource "before all" hook in "CSV Generation from SearchSource"

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
alerting 803 804 +1
Unknown metric groups

API count

id before after diff
alerting 834 835 +1

History

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

@mikecote mikecote merged commit 9f1f142 into elastic:main Feb 5, 2024
39 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Feb 5, 2024
fkanout pushed a commit to fkanout/kibana that referenced this pull request Feb 7, 2024
elastic#173936)

In this PR, I'm adding a test in the alerting framework to detect
changes in a rule type's params schema that will require a snapshot to
be updated. This snapshot will provide a centralized place to view
history on alerting rule params in case we need to asses risk for
rolling upgrades or rollbacks of a release (serverless). The only rule
types affected are those running in serverless in any of the three
project types.

When a rule type is used in serverless, it must provide one of the
following configuration to their rule type on top of everything else:

```
// Zod schema
schemas: {
  params: {
    type: 'zod',
    schema: UnifiedQueryRuleParams
  },
},

// config-schema
schemas: {
  params: {
    type: 'config-schema',
    schema: EsQueryRuleParamsSchema,
  },
},
```

We are working on documenting guidelines so engineers and response ops
can ensure a change to rule parameters will work properly in rolling
upgrade and rollback scenarios and be part of the PR review process.

NOTE to rule type owners: I pass the same schema used to validate to the
`schemas.params` attribute in the rule type. It will be important to
keep them in sync. Down the road, we plan to make `validate.params`
optional and use the schema as a starting point so it's easier to have a
single variable passed in.

## To verify
1. Make changes to the params schema of the ES query rule type.
```
diff --git a/x-pack/plugins/stack_alerts/server/rule_types/es_query/rule_type_params.ts b/x-pack/plugins/stack_alerts/server/rule_types/es_query/rule_type_params.ts
index 73e8eae32cf..09ec74104ec 100644
--- a/x-pack/plugins/stack_alerts/server/rule_types/es_query/rule_type_params.ts
+++ b/x-pack/plugins/stack_alerts/server/rule_types/es_query/rule_type_params.ts
@@ -39,6 +39,7 @@ export type EsQueryRuleParamsExtractedParams = Omit<EsQueryRuleParams, 'searchCo
 };

 const EsQueryRuleParamsSchemaProperties = {
+  foo: schema.boolean(),
   size: schema.number({ min: 0, max: ES_QUERY_MAX_HITS_PER_EXECUTION }),
   timeWindowSize: schema.number({ min: 1 }),
   excludeHitsFromPreviousRun: schema.boolean({ defaultValue: true }),
```
2. Run the jest integration test to update the snapshot file
```
node scripts/jest_integration.js x-pack/plugins/alerting/server/integration_tests/serverless_upgrade_and_rollback_checks.test.ts -u
```
3. Notice the
`x-pack/plugins/alerting/server/integration_tests/__snapshots__/serverless_upgrade_and_rollback_checks.test.ts.snap`
file got updated
```
    "foo": Object {
      "flags": Object {
        "error": [Function],
      },
      "type": "boolean",
    },
```

---------

Co-authored-by: kibanamachine <[email protected]>
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
elastic#173936)

In this PR, I'm adding a test in the alerting framework to detect
changes in a rule type's params schema that will require a snapshot to
be updated. This snapshot will provide a centralized place to view
history on alerting rule params in case we need to asses risk for
rolling upgrades or rollbacks of a release (serverless). The only rule
types affected are those running in serverless in any of the three
project types.

When a rule type is used in serverless, it must provide one of the
following configuration to their rule type on top of everything else:

```
// Zod schema
schemas: {
  params: {
    type: 'zod',
    schema: UnifiedQueryRuleParams
  },
},

// config-schema
schemas: {
  params: {
    type: 'config-schema',
    schema: EsQueryRuleParamsSchema,
  },
},
```

We are working on documenting guidelines so engineers and response ops
can ensure a change to rule parameters will work properly in rolling
upgrade and rollback scenarios and be part of the PR review process.

NOTE to rule type owners: I pass the same schema used to validate to the
`schemas.params` attribute in the rule type. It will be important to
keep them in sync. Down the road, we plan to make `validate.params`
optional and use the schema as a starting point so it's easier to have a
single variable passed in.

## To verify
1. Make changes to the params schema of the ES query rule type.
```
diff --git a/x-pack/plugins/stack_alerts/server/rule_types/es_query/rule_type_params.ts b/x-pack/plugins/stack_alerts/server/rule_types/es_query/rule_type_params.ts
index 73e8eae32cf..09ec74104ec 100644
--- a/x-pack/plugins/stack_alerts/server/rule_types/es_query/rule_type_params.ts
+++ b/x-pack/plugins/stack_alerts/server/rule_types/es_query/rule_type_params.ts
@@ -39,6 +39,7 @@ export type EsQueryRuleParamsExtractedParams = Omit<EsQueryRuleParams, 'searchCo
 };

 const EsQueryRuleParamsSchemaProperties = {
+  foo: schema.boolean(),
   size: schema.number({ min: 0, max: ES_QUERY_MAX_HITS_PER_EXECUTION }),
   timeWindowSize: schema.number({ min: 1 }),
   excludeHitsFromPreviousRun: schema.boolean({ defaultValue: true }),
```
2. Run the jest integration test to update the snapshot file
```
node scripts/jest_integration.js x-pack/plugins/alerting/server/integration_tests/serverless_upgrade_and_rollback_checks.test.ts -u
```
3. Notice the
`x-pack/plugins/alerting/server/integration_tests/__snapshots__/serverless_upgrade_and_rollback_checks.test.ts.snap`
file got updated
```
    "foo": Object {
      "flags": Object {
        "error": [Function],
      },
      "type": "boolean",
    },
```

---------

Co-authored-by: kibanamachine <[email protected]>
fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024
elastic#173936)

In this PR, I'm adding a test in the alerting framework to detect
changes in a rule type's params schema that will require a snapshot to
be updated. This snapshot will provide a centralized place to view
history on alerting rule params in case we need to asses risk for
rolling upgrades or rollbacks of a release (serverless). The only rule
types affected are those running in serverless in any of the three
project types.

When a rule type is used in serverless, it must provide one of the
following configuration to their rule type on top of everything else:

```
// Zod schema
schemas: {
  params: {
    type: 'zod',
    schema: UnifiedQueryRuleParams
  },
},

// config-schema
schemas: {
  params: {
    type: 'config-schema',
    schema: EsQueryRuleParamsSchema,
  },
},
```

We are working on documenting guidelines so engineers and response ops
can ensure a change to rule parameters will work properly in rolling
upgrade and rollback scenarios and be part of the PR review process.

NOTE to rule type owners: I pass the same schema used to validate to the
`schemas.params` attribute in the rule type. It will be important to
keep them in sync. Down the road, we plan to make `validate.params`
optional and use the schema as a starting point so it's easier to have a
single variable passed in.

## To verify
1. Make changes to the params schema of the ES query rule type.
```
diff --git a/x-pack/plugins/stack_alerts/server/rule_types/es_query/rule_type_params.ts b/x-pack/plugins/stack_alerts/server/rule_types/es_query/rule_type_params.ts
index 73e8eae32cf..09ec74104ec 100644
--- a/x-pack/plugins/stack_alerts/server/rule_types/es_query/rule_type_params.ts
+++ b/x-pack/plugins/stack_alerts/server/rule_types/es_query/rule_type_params.ts
@@ -39,6 +39,7 @@ export type EsQueryRuleParamsExtractedParams = Omit<EsQueryRuleParams, 'searchCo
 };

 const EsQueryRuleParamsSchemaProperties = {
+  foo: schema.boolean(),
   size: schema.number({ min: 0, max: ES_QUERY_MAX_HITS_PER_EXECUTION }),
   timeWindowSize: schema.number({ min: 1 }),
   excludeHitsFromPreviousRun: schema.boolean({ defaultValue: true }),
```
2. Run the jest integration test to update the snapshot file
```
node scripts/jest_integration.js x-pack/plugins/alerting/server/integration_tests/serverless_upgrade_and_rollback_checks.test.ts -u
```
3. Notice the
`x-pack/plugins/alerting/server/integration_tests/__snapshots__/serverless_upgrade_and_rollback_checks.test.ts.snap`
file got updated
```
    "foo": Object {
      "flags": Object {
        "error": [Function],
      },
      "type": "boolean",
    },
```

---------

Co-authored-by: kibanamachine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:review backport:skip This commit does not require backporting Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team Team:obs-ux-management Observability Management User Experience Team Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.13.0
Projects
No open projects
Development

Successfully merging this pull request may close these issues.