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.

Sign up for GitHub

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

Jump to bottom

[Reporting/Tests] Improvements for task stability in serverless tests #195841

Merged
merged 25 commits into from
Oct 24, 2024
Merged

[Reporting/Tests] Improvements for task stability in serverless tests #195841

merged 25 commits into from
Oct 24, 2024

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Oct 10, 2024 •
edited by kibanamachine
Loading

Summary

Continuation of #192417. This PR attempts to further improve task stability of the reporting task. The original goals were:

  1. Ensure the test data that is needed for the report gets loaded
  2. Wait for report jobs to finish before the test completes. Errors in task success metrics also occur if the task triggers after resources for the report, such as a saved search, are removed before the task triggers.

During development of this PR, more issues were discovered:

  1. Requests to internal endpoints should use cookie credentials
  2. The CSV export from ES|QL test was hitting a 404 error when it tried to download the CSV. That error was included in the test. In other words, that test was fundamentaly broken.

Testing locally

  1. Run the serverless functional tests:
    1. Reporting management app: node scripts/functional_tests.js --config=x-pack/test_serverless/functional/test_suites/observability/common_configs/config.group1.ts --grep=Reporting
    2. CSV export in Discover: node scripts/functional_tests.js --config=x-pack/test_serverless/functional/test_suites/observability/common_configs/config.group6.ts --grep=CSV
    3. Reporting API integration tests: node scripts/functional_tests.js --config=x-pack/test_serverless/api_integration/test_suites/search/common_configs/config.group1.ts --grep=Reporting
  2. Ensure that there are no error logs from Task Manager regarding task failure

@tsullivan tsullivan force-pushed the reporting/tests-serverless-improvements-ii branch 4 times, most recently from 7a3169d to 02579c9 Compare October 16, 2024 20:54
@tsullivan tsullivan force-pushed the reporting/tests-serverless-improvements-ii branch from 02579c9 to 11351b7 Compare October 16, 2024 22:45
@tsullivan tsullivan changed the title [Reporting] Test improvements for task stability in serverless tests [Reporting/Tests] Improvements for task stability in serverless tests Oct 17, 2024
@tsullivan tsullivan marked this pull request as ready for review October 17, 2024 20:36
@tsullivan tsullivan requested review from a team as code owners October 17, 2024 20:36
@tsullivan tsullivan added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Oct 17, 2024
dmlemeshko
dmlemeshko reviewed Oct 18, 2024
View reviewed changes
Copy link
Member

@dmlemeshko dmlemeshko left a comment

Choose a reason for hiding this comment

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

Good improvement, I changed test to use cookieHeader for internal API call and left similar question about another service call

x-pack/test_serverless/api_integration/test_suites/common/reporting/management.ts Outdated Show resolved Hide resolved
x-pack/test_serverless/shared/services/svl_reporting.ts Outdated Show resolved Hide resolved
dmlemeshko
dmlemeshko reviewed Oct 22, 2024
View reviewed changes
Copy link
Member

@dmlemeshko dmlemeshko left a comment

Choose a reason for hiding this comment

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

Hey Tim, I left few more questions.

x-pack/test_serverless/api_integration/test_suites/common/reporting/management.ts Outdated
Comment on lines 73 to 78
it(`user can delete a report they've created`, async () => {
const response = await supertestWithoutAuth
.delete(`${INTERNAL_ROUTES.JOBS.DELETE_PREFIX}/${reportJob.id}`)
.set(...API_HEADER)
.set(...INTERNAL_HEADER)
.set(cookieCredentials);
Copy link
Member

Choose a reason for hiding this comment

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

Good one!

x-pack/test_serverless/shared/services/svl_reporting.ts Show resolved Hide resolved
@tsullivan tsullivan added backport:version Backport to applied version labels v8.17.0 and removed backport:skip This commit does not require backporting labels Oct 22, 2024
@tsullivan tsullivan force-pushed the reporting/tests-serverless-improvements-ii branch from 98f137c to 2735259 Compare October 23, 2024 23:17
@tsullivan tsullivan force-pushed the reporting/tests-serverless-improvements-ii branch from 2735259 to 8e4ee0f Compare October 23, 2024 23:18
tsullivan
tsullivan commented Oct 23, 2024
View reviewed changes
x-pack/test_serverless/api_integration/test_suites/common/reporting/csv_v2_esql.ts
@@ -84,7 +89,7 @@ export default ({ getService }: FtrProviderContext) => {
};
before(async () => {
await loadTimelessData();
roleAuthc = await svlUserManager.createM2mApiKeyWithRoleScope('admin');
cookieCredentials = await samlAuth.getM2MApiCookieCredentialsWithRoleScope('admin');
Copy link
Member Author

Choose a reason for hiding this comment

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

NOTE: cookie credentials do not need to be invalidated after the test

tsullivan
tsullivan commented Oct 23, 2024
View reviewed changes
x-pack/test_serverless/api_integration/test_suites/common/reporting/csv_v2_esql.ts
await reportingAPI.waitForJobToFinish(path, roleAuthc, internalReqHeader);
response = await supertest.get(path);
await reportingAPI.waitForJobToFinish(path, cookieCredentials, internalReqHeader);
response = await supertestWithoutAuth.get(path).set(cookieCredentials);
Copy link
Member Author

Choose a reason for hiding this comment

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

This test was BROKEN since the original PR. The snapshot showed a 404 error. This is now fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh no :( typical for snapshot tests in a hidden file

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

@tsullivan tsullivan merged commit 4c0b5c5 into elastic:main Oct 24, 2024
31 checks passed
@tsullivan tsullivan deleted the reporting/tests-serverless-improvements-ii branch October 24, 2024 13:59
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11500876238

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
❌ 8.x Backport failed because of merge conflicts

You might need to backport the following PRs to 8.x:
- [DataUsage][Serverless] Handle usage metrics errors (#197056)
- [Inventory] List k8s entities in the grid (#197292)
- fix flaky test - counts dashboard views (#197381)
- fix: dynamically update timestamps based on the chosen timezone in the Advanced Settings (#196977)
- [Cloud Security] Sending the Agentless API the deployment_mode information (#196955)
- chore(o11y): add missing access options to routes (#197511)
- [Onboarding]Update manage indices button in index management to navigate to search_indices details page (#196787)
- [SecuritySolution] Fix issue of disappearing columns in the alerts table (#197043)
- [AI Assistant] Set scope and rename to Observability and Search (#196322)
- Migrate Custom threshold > AVG - PCT - FIRED test to the deployment agnostic framework (#195902)

Manual backport

To create the backport manually run:

node scripts/backport --pr 195841

Questions ?

Please refer to the Backport tool documentation

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 25, 2024
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 195841 locally

8 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 195841 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 195841 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 195841 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 195841 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 195841 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 195841 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 195841 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 195841 locally

@tsullivan tsullivan added backport:skip This commit does not require backporting and removed v9.0.0 backport:version Backport to applied version labels v8.17.0 labels Nov 7, 2024
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmlemeshko dmlemeshko dmlemeshko approved these changes

@Dosant Dosant Dosant approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tsullivan @elasticmachine @kibanamachine @Dosant @dmlemeshko