[RAC] [APM] removes hardcoded alerts as data index from apm integration test by dhurley14 · Pull Request #109715 · elastic/kibana

dhurley14 · 2021-08-23T18:21:02Z

Summary

removes hardcoded alerts as data index from apm integration test and adds integration tests for getting alerts index from rule registry route.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

dhurley14 · 2021-08-23T20:56:24Z

x-pack/test/apm_api_integration/common/config.ts

  name: APMFtrConfigName;
  license: 'basic' | 'trial';
-  kibanaConfig?: Record<string, string>;
+  kibanaConfig?: Record<string, string | string[]>;


Added the above while I was debugging the integration tests using the following as part of the FTR kibana config and needed to pass in an array of values instead of just a string. I can remove this but figured it might be useful going forward for anybody who is debugging tests in the future.

--logging.verbose=true,
--logging.events.log=${JSON.stringify([ 'alerts', 'ruleRegistry', 'apm', 'securitySolution', 'info', 'warning', 'error', 'fatal', ])},
--logging.events.request=${JSON.stringify(['info', 'warning', 'error', 'fatal'])},
--logging.events.error='*',
--logging.events.ops=__no-ops__,

dhurley14 · 2021-08-23T20:56:39Z

x-pack/test/apm_api_integration/common/config.ts

+            ? Object.entries(kibanaConfig).map(([key, value]) =>
+                Array.isArray(value) ? `--${key}=${JSON.stringify(value)}` : `--${key}=${value}`
+              )


related to my above comment

dgieselaar · 2021-08-24T14:21:48Z

x-pack/test/apm_api_integration/tests/alerts/rule_registry.ts

+          index: targetIndices[0],
+        });
+        // eslint-disable-next-line no-empty
+      } catch (exc) {}


can you leave a comment here why it's okay for this to fail?

For sure I can add a comment to the code. Essentially it boils down to this:

When calling refresh on an index pattern .alerts-observability.apm.alerts* (as was originally the hard-coded string in this test) The response from Elasticsearch is a 200, even if no indices which match that index pattern have been created.

When calling refresh on a concrete index alias .alerts-observability.apm.alerts-default for instance, we receive a 404 error index_not_found_exception when no indices have been created which match that alias (obviously).

Since we are receiving a concrete index alias from the observability api instead of a kibana index pattern and we understand / expect that this index does not exist at certain points of the test, we can try-catch at certain points without caring if the call fails. There are points in the code where we do want to ensure we get the appropriate error message back (such as this expect you commented on)

Example..

# This request yields a 200 response GET thisdoesnotexist*/_refresh # Response { "_shards" : { "total" : 0, "successful" : 0, "failed" : 0 } } # This request yields a 404 error GET thisdoesnotexist/_refresh # Response { "error" : { "root_cause" : [ { "type" : "index_not_found_exception", "reason" : "no such index [thisdoesnotexist]", "resource.type" : "index_or_alias", "resource.id" : "thisdoesnotexist", "index_uuid" : "_na_", "index" : "thisdoesnotexist" } ], "type" : "index_not_found_exception", "reason" : "no such index [thisdoesnotexist]", "resource.type" : "index_or_alias", "resource.id" : "thisdoesnotexist", "index_uuid" : "_na_", "index" : "thisdoesnotexist" }, "status" : 404 }

Fixed here: 6c5e2bf

is allow_no_indices not good enough here?

or maybe we can just add an asterisk to the end?

dgieselaar · 2021-08-24T14:22:21Z

x-pack/test/apm_api_integration/tests/alerts/rule_registry.ts

+          allow_no_indices: true,
+        });
+      } catch (exc) {
+        expect(exc.statusCode).to.eql(404);


there's no assertion here when the request succeeds, no?

Good catch I'll update this to fail if a 200 response is received.

Fixed here: 6c5e2bf

dgieselaar · 2021-08-24T14:22:31Z

x-pack/test/apm_api_integration/tests/alerts/rule_registry.ts

+            index: APM_METRIC_INDEX_NAME,
+          });
+          // eslint-disable-next-line no-empty
+        } catch (exc) {}


same here, comment would be great

Ah yeah I think I can undo this.

Fixed here: 6c5e2bf

dgieselaar · 2021-08-24T14:22:46Z

x-pack/test/apm_api_integration/tests/alerts/rule_registry.ts

+            refresh: true,
+          });
+          // eslint-disable-next-line no-empty
+        } catch (exc) {}


here as well

Fixed here: 6c5e2bf

dgieselaar · 2021-08-24T14:23:19Z

x-pack/test/apm_api_integration/tests/alerts/rule_registry.ts

-        expect(beforeDataResponse.body.hits.hits.length).to.be(0);
+          });
+        } catch (exc) {
+          expect(exc.message).contain('index_not_found_exception');


same here, there's only an assertion when the request fails

Yup I'll update this to fail if a 200 is received as well.

Fixed here: 6c5e2bf

I am wondering if this is necessary. I think the intent here was to ensure there was no data written before the alert executed, and before violations were indexed, I don't think we need to test whether the index exists here, as long as we have a different test for that (which I think we do?). My concern here would be a) that these assertions are adding to the complexity of the test, b) might cause some false negatives to trigger in certain scenarios (e.g. when the index is not cleaned up after a test run, or the implementation changes).

…x refresh catch block is empty

kibanamachine · 2021-08-24T17:13:21Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #147936 succeeded 4044431
💚 Build #147908 succeeded 0880e81
💚 Build #147877 succeeded eb85af6

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

cc @dhurley14

banderror

Changes in x-pack/test/apm_api_integration/tests/alerts/rule_registry.ts LGTM 👍

banderror · 2021-08-24T17:57:24Z

x-pack/test/rule_registry/security_and_spaces/tests/basic/get_alerts_index.ts

+  const TEST_URL = '/internal/rac/alerts';
+  const ALERTS_INDEX_URL = `${TEST_URL}/index`;
+  const SPACE1 = 'space1';
+  const APM_ALERT_INDEX = '.alerts-observability-apm';


Note for myself: I will need to fix this name in #109567

dgieselaar · 2021-08-25T16:59:38Z

x-pack/test/apm_api_integration/tests/alerts/rule_registry.ts

      .get(`/api/alerts/alert/${alert.id}`)
      .set('kbn-xsrf', 'foo');

+    const { body: targetIndices, status: targetIndicesStatus } = await getAlertsTargetIndices();


what's the purpose of this exactly? This function only cares about whether the rule was executed. There might be a scenario where the alert index does not exist, but it's okay because no alerts were written.

dgieselaar

LGTM, some questions/concerns but nothing blocking. Thanks!

…on test (elastic#109715) * removes hardcoded index from apm integration tests * adds integration tests for rule registry's get index route * more cleanup * fail try-catch if response is not empty, adds comments as to why index refresh catch block is empty

kibanamachine · 2021-08-25T17:11:16Z

💚 Backport successful

Status	Branch	Result
✅	7.15
✅	7.x

The backport PRs will be merged automatically after passing CI.

…tegration test (#109715) (#110108) * [RAC] [APM] removes hardcoded alerts as data index from apm integration test (#109715) * removes hardcoded index from apm integration tests * adds integration tests for rule registry's get index route * more cleanup * fail try-catch if response is not empty, adds comments as to why index refresh catch block is empty * fix name of index Co-authored-by: Devin W. Hurley <devin.hurley@elastic.co>

…egration test (#109715) (#110109) * [RAC] [APM] removes hardcoded alerts as data index from apm integration test (#109715) * removes hardcoded index from apm integration tests * adds integration tests for rule registry's get index route * more cleanup * fail try-catch if response is not empty, adds comments as to why index refresh catch block is empty * fix index name Co-authored-by: Devin W. Hurley <devin.hurley@elastic.co>

removes hardcoded index from apm integration tests

eb85af6

dhurley14 force-pushed the integration_tests_o11y branch from 936eeba to eb85af6 Compare August 23, 2021 18:22

dhurley14 changed the title ~~cleanup code~~ [RAC] [APM] removes hardcoded alerts as data index from apm integration test Aug 23, 2021

dhurley14 added 2 commits August 23, 2021 15:32

adds integration tests for rule registry's get index route

0880e81

more cleanup

4044431

dhurley14 self-assigned this Aug 23, 2021

dhurley14 added apm:rac Feature:Observability RAC Feature:RAC label obsolete review Theme: rac label obsolete v7.15.0 v7.16.0 v8.0.0 labels Aug 23, 2021

dhurley14 marked this pull request as ready for review August 23, 2021 20:48

dhurley14 requested a review from a team August 23, 2021 20:48

dhurley14 added the release_note:skip Skip the PR/issue when compiling release notes label Aug 23, 2021

dhurley14 commented Aug 23, 2021

View reviewed changes

dhurley14 added the auto-backport Deprecated - use backport:version if exact versions are needed label Aug 24, 2021

dgieselaar reviewed Aug 24, 2021

View reviewed changes

fail try-catch if response is not empty, adds comments as to why inde…

6c5e2bf

…x refresh catch block is empty

Kerry350 mentioned this pull request Aug 24, 2021

[Observability] [RAC] Add basic functional tests for observability alerts #109876

Merged

dhurley14 enabled auto-merge (squash) August 24, 2021 16:18

banderror approved these changes Aug 24, 2021

View reviewed changes

dgieselaar reviewed Aug 25, 2021

View reviewed changes

dgieselaar approved these changes Aug 25, 2021

View reviewed changes

dhurley14 merged commit 2fea917 into elastic:master Aug 25, 2021

kibanamachine mentioned this pull request Aug 25, 2021

[7.15] [RAC] [APM] removes hardcoded alerts as data index from apm integration test (#109715) #110108

Merged

kibanamachine mentioned this pull request Aug 25, 2021

[7.x] [RAC] [APM] removes hardcoded alerts as data index from apm integration test (#109715) #110109

Merged

banderror mentioned this pull request Aug 25, 2021

[RAC][APM] Fix alerts index in APM tests #110158

Closed

9 tasks

Conversation

dhurley14 commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Risk Matrix

For maintainers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhurley14 Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Aug 24, 2021

💚 Build Succeeded

Metrics [docs]

History

Uh oh!

banderror left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgieselaar left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Aug 25, 2021

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dhurley14 commented Aug 23, 2021 •

edited

Loading

dhurley14 Aug 24, 2021 •

edited

Loading