[AO] Add API integration test scenarios to the Threshold rule #162501

fkanout · 2023-07-25T15:31:38Z

Summary

It fixes #162491

elasticmachine · 2023-07-25T15:31:53Z

Pinging @elastic/actionable-observability (Team: Actionable Observability)

apmmachine · 2023-07-25T15:31:59Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

…hold-rule

kibana-ci · 2023-07-27T14:48:58Z

💔 Build Failed

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #30 / security APIs - Kerberos Kerberos authentication API access with expired access token. post-authentication stage expired access token should be automatically refreshed by the start-contract client even for multiple concurrent requests
[job] [logs] FTR Configs #36 / security APIs - Kerberos Kerberos authentication API access with expired access token. post-authentication stage expired access token should be automatically refreshed by the start-contract client even for multiple concurrent requests
[job] [logs] FTR Configs #30 / security APIs - Kerberos Kerberos authentication API access with expired access token. post-authentication stage expired access token should be automatically refreshed by the start-contract client even for multiple concurrent requests
[job] [logs] FTR Configs #36 / security APIs - Kerberos Kerberos authentication API access with expired access token. post-authentication stage expired access token should be automatically refreshed by the start-contract client even for multiple concurrent requests
[job] [logs] FTR Configs #49 / security APIs - OIDC (Authorization Code Flow) OpenID Connect authentication API access with expired access token. post-authentication stage expired access token should be automatically refreshed by the start-contract client even for multiple concurrent requests
[job] [logs] FTR Configs #49 / security APIs - OIDC (Authorization Code Flow) OpenID Connect authentication API access with expired access token. post-authentication stage expired access token should be automatically refreshed by the start-contract client even for multiple concurrent requests
[job] [logs] FTR Configs #51 / security APIs - SAML SAML authentication API access with expired access token. should refresh access token even if multiple concurrent requests try to refresh it
[job] [logs] FTR Configs #51 / security APIs - SAML SAML authentication API access with expired access token. should refresh access token even if multiple concurrent requests try to refresh it
[job] [logs] FTR Configs #5 / security APIs - Token session API access with expired access token. post-authentication stage expired access token should be automatically refreshed by the start-contract client even for multiple concurrent requests
[job] [logs] FTR Configs #5 / security APIs - Token session API access with expired access token. post-authentication stage expired access token should be automatically refreshed by the start-contract client even for multiple concurrent requests
[job] [logs] FTR Configs #35 / Synthetics API Tests SyncGlobalParams added an integration for previously added monitor
[job] [logs] FTR Configs #35 / Synthetics API Tests SyncGlobalParams added an integration for previously added monitor
[job] [logs] FTR Configs #24 / task_manager check_registered_task_types should check changes on all registered task types
[job] [logs] FTR Configs #24 / task_manager check_registered_task_types should check changes on all registered task types

Metrics [docs]

✅ unchanged

History

💔 Build #144875 failed b472651
💚 Build #144693 succeeded 0b9adad

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

cc @fkanout

pmuellr

LGTM, left a few comments ...

pmuellr · 2023-07-27T18:42:51Z

x-pack/test/alerting_api_integration/observability/threshold_rule/avg_pct_fired.ts

+    });
+
+    after(async () => {
+      await supertest.delete(`/api/alerting/rule/${ruleId}`).set('kbn-xsrf', 'foo');


We have a utility ObjectRemover which may simplify some of this sort of stuff - or not! Example usage here.

pmuellr · 2023-07-27T18:47:31Z

x-pack/test/alerting_api_integration/observability/threshold_rule/avg_pct_no_data.ts

+      });
+
+      it('should be active', async () => {
+        const executionStatus = await waitForRuleStatus({


Kinda up to you, since you own these tests, but we generally treat each test as stand-alone, so that you could change it(... to it.only(... when diagnosing tests. I think we'd more generally write a function here to create the rule / connector, and call it from each test, and then delete it (somehow - ObjectRemover or other) in an afterEach().

@pmuellr, thank you for the feedback!
I wrote these tests as a file-per-scenario as we are sharing the same tests in serverless and stateful env to maintain them easily (copy/paste 😄) with minimum dependencies until we can share one test suite across ENVs (agnostic tests)

Ya, will be interesting to see how serverless / stateful test sharing will go!

maintain them easily (copy/paste 😄)

Probably more of a risk of these being problematic, in that each test depends on the previous to work. Small number of tests here though, so hard to imagine it going wrong in this file, now. Something to think about if these get more complex.

maryam-saeidi

Overall LGTM 👍🏻

Just added some clarification questions :)

maryam-saeidi · 2023-08-02T15:12:09Z

x-pack/test/alerting_api_integration/observability/threshold_rule/custom_eq_avg_bytes_fired.ts

+                timeSize: 1,
+                timeUnit: 'm',
+                customMetrics: [
+                  { name: 'A', field: 'system.network.in.bytes', aggType: Aggregators.AVERAGE },


For the system.network.in.bytes and system.network.out.bytes, do we have hard coded value in the @kbn/infra-forge, or are they a random number?

Neither, it's incremental.

So it means the reason will be different on each execution, right?
Probably, when we test action variables and check the reason, we need to make this value more predictable.

maryam-saeidi · 2023-08-02T15:17:10Z

x-pack/test/alerting_api_integration/observability/threshold_rule/avg_pct_fired.ts

+  const esDeleteAllIndices = getService('esDeleteAllIndices');
+  const logger = getService('log');
+
+  describe('Threshold rule - AVG - PCT - FIRED', () => {


What are the differences between Threshold rule - AVG - PCT - FIRED and Threshold rule - AVG - US - FIRED?
Initially, I was thinking about having multiple scenarios to check the reason to make sure they are properly formatted, I am curious to know what other differences we can test.

FYI, I created another ticket for the action variables, then we can also check the reason to ensure the correct formating is used.

What are the differences between Threshold rule - AVG - PCT - FIRED and Threshold rule - AVG - US - FIRED?

Field data type % and microseconds

Initially, I was thinking about having multiple scenarios to check the reason to make sure they are properly formatted, I am curious to know what other differences we can test.

I am not sure If understand what you mean, but please create a ticket with the check you want to do with the scenarios you have in mind.

My question is, what do we test for different field data types related to their type? Can you please point me to the code that is different between these two tests related to their field types?

@maryam-saeidi, I'm fulfilling the 1st scenario you requested in your issue:

Multiple conditions with different types of fields (such as number, string, percent) without grouping (Can we have one condition for every condition type? 😄)

I thought you knew the difference, so you asked to cover it.

Changing the field type will make many areas of code to be tested. I will list some files to answer the question
x-pack/plugins/observability/server/lib/rules/threshold/lib/get_data.ts
x-pack/plugins/observability/server/lib/rules/threshold/lib/metric_query.ts
x-pack/plugins/observability/server/lib/rules/threshold/threshold_executor.ts
x-pack/plugins/observability/server/lib/rules/threshold/lib/create_percentile_aggregation.ts
... and maybe others

As this is an integration test (not a unit test), asking about what lines of code are covered is inadequate and hard to answer.

I thought you knew the difference, so you asked to cover it.

Yes, the difference was the reason message that we are not checking at the moment.

As this is an integration test (not a unit test), asking about what lines of code are covered is inadequate and hard to answer.

By the line of code, I meant related to the new tests that you added, not the code behind the scene, something maybe like: the value that is saved in AAD is formatted differently for different field types.

maryam-saeidi

I've added some comments that probably we need to change when we test the action variables and specifically the reason message, but I think this PR is good to go!

Add tests

0b9adad

fkanout self-assigned this Jul 25, 2023

fkanout marked this pull request as ready for review July 25, 2023 15:31

fkanout requested a review from a team as a code owner July 25, 2023 15:31

fkanout and others added 2 commits July 26, 2023 11:41

Merge branch 'main' into 162491-add-integration-tests-scenarios-thres…

b472651

…hold-rule

add rule

9c81bd5

fkanout requested a review from a team as a code owner July 27, 2023 14:24

fix typo

7fab885

pmuellr approved these changes Jul 27, 2023

View reviewed changes

maryam-saeidi reviewed Aug 2, 2023

View reviewed changes

maryam-saeidi approved these changes Aug 4, 2023

View reviewed changes

fkanout merged commit 6902008 into elastic:main Aug 4, 2023

elasticmachine unassigned fkanout Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AO] Add API integration test scenarios to the Threshold rule #162501

[AO] Add API integration test scenarios to the Threshold rule #162501

fkanout commented Jul 25, 2023

elasticmachine commented Jul 25, 2023

apmmachine commented Jul 25, 2023

kibana-ci commented Jul 27, 2023 •

edited

Loading

pmuellr left a comment

pmuellr Jul 27, 2023

pmuellr Jul 27, 2023

fkanout Aug 2, 2023

pmuellr Aug 2, 2023

maryam-saeidi left a comment

maryam-saeidi Aug 2, 2023

fkanout Aug 3, 2023

maryam-saeidi Aug 4, 2023

maryam-saeidi Aug 2, 2023

fkanout Aug 3, 2023

maryam-saeidi Aug 3, 2023 •

edited

Loading

fkanout Aug 4, 2023 •

edited

Loading

fkanout Aug 4, 2023

maryam-saeidi Aug 4, 2023

maryam-saeidi Aug 4, 2023

maryam-saeidi left a comment

[AO] Add API integration test scenarios to the Threshold rule #162501

[AO] Add API integration test scenarios to the Threshold rule #162501

Conversation

fkanout commented Jul 25, 2023

Summary

elasticmachine commented Jul 25, 2023

apmmachine commented Jul 25, 2023

🤖 GitHub comments

kibana-ci commented Jul 27, 2023 • edited Loading

💔 Build Failed

Failed CI Steps

Test Failures

Metrics [docs]

History

pmuellr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maryam-saeidi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maryam-saeidi Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

fkanout Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maryam-saeidi left a comment

Choose a reason for hiding this comment

kibana-ci commented Jul 27, 2023 •

edited

Loading

maryam-saeidi Aug 3, 2023 •

edited

Loading

fkanout Aug 4, 2023 •

edited

Loading