[Reporting] Fix job notifications poller #177537

tsullivan · 2024-02-22T02:54:30Z

Summary

In previous code, Reporting fetched for a list of pending reports and compared the status of each result to a stored record of pending reports. The flakiness occurred because sometimes the results don't contain a record for a very "fresh" report since the refresh interval hadn't ticked and the stored change wasn't reflecting in search results.

With switching Reporting to use a data stream, there was new flakiness in serverless tests. The area of flakiness was in getting Reporting to show the "Job completed" toast notification. The main difference faced with data streams is a lower index refresh interval, which is lower even more for serverless.

This PR fixes the poller to loop through the list of stored pending reports and compare each to the result from the search. If a stored report isn't present in the search, we consider that to still be in the pending state.

Other changes:

Improvements the class responsible for managing updates to the stored list of pending reports, so that each operation performs atomically and go through a queue to mitigate the chance of race conditions.
Update function names and variable names for readability
Remove the unused removeJob function.
Move helper code into private methods of the StreamHandler class and simplify x-pack/plugins/reporting/public/plugin.ts

Release note

Fixed an issue in Reporting with consistently showing the toast message for completed report jobs.

tsullivan · 2024-02-22T15:00:34Z

x-pack/plugins/reporting/public/lib/stream_handler.ts

  /*
   * Use Kibana Toast API to show our messages
+   *
+   * Public for purposes of testing


Question: should we make the method public for the purpose of testing, or make it private and use // @ts-expect-error in the test where we need to access it?

...or create a new class for testing that extends this in the test file?

maybe we can extract most of the logic into a separate function and unit test it there?

@Dosant I refactored the methods to protected and created a test class to extend this one and expose testable methods

tsullivan · 2024-02-22T15:01:56Z

x-pack/plugins/reporting/public/lib/stream_handler.ts

+        const newFailed: JobSummary[] = [];
+        const newPending: JobId[] = [];
+
+        for (const pendingJobId of previousPending) {


The main change is here: loop through the set of stored job IDs rather than loop through the job IDs returned in the search

tsullivan · 2024-02-22T15:02:32Z

x-pack/plugins/reporting/public/lib/stream_handler.ts

+            // Keep job tracked in storage if is pending. It also
+            // may not be present in apiClient.findForJobIds
+            // response if index refresh is slow


The comment explains why the bug happened.

x-pack/plugins/reporting/public/lib/stream_handler.ts

x-pack/plugins/reporting/public/notifier/job_completion_notifications.ts

elasticmachine · 2024-02-22T15:07:15Z

Pinging @elastic/appex-sharedux (Team:SharedUX)

Dosant · 2024-02-23T09:29:24Z

packages/kbn-reporting/public/job_completion_notifications.ts

+// Operations on the localStorage key can happen from various
+// parts of code. Using a queue to manage async operations allows
+// operations to process one at a time
+let operationQueue = Promise.resolve();


Each plugin that imports the "kbn-reporting" package would receive its copy, hence different queues. Could this be a problem?

Good point - it probably would.

Created d771862

Dosant · 2024-02-23T09:34:33Z

packages/kbn-reporting/public/job_completion_notifications.ts

+      // add the new job
+      jobs.push(jobId);
+      // write back to local storage
+      localStorage.setItem(JOB_COMPLETION_NOTIFICATIONS_SESSION_KEY, JSON.stringify(jobs));


what if instead of managing the state as a single blob, we would write a job state for each job separately.
e.g.

localStorage.setItem(prefix-${jobId}, job),
would this keep things simpler?

It would complicate the process of retrieving all of the pending jobs. We can use a common prefix, with a dynamic suffix for the job ID. But to get all of the pending job IDs, we need to get all of the keys from local storage and check each one to see if it has the matching prefix. So, I think this would not make the process simpler.

But to get all of the pending job IDs, we need to get all of the keys from local storage and check each one to see if it has the matching prefix.

yes, but this keeps the code synchronous, and there is no need to manage the queue. it seems like this option would be simpler even with the need to read all keys (which would be hidden in a small function)

Since there won't be millions of keys in local storage, this seems fair. I think this is good feedback since the change to async wasn't needed for the important part of this fix.

Pushed a471462

Dosant · 2024-02-23T09:36:03Z

x-pack/plugins/reporting/public/lib/stream_handler.ts

  /*
   * Use Kibana Toast API to show our messages
+   *
+   * Public for purposes of testing


maybe we can extract most of the logic into a separate function and unit test it there?

Dosant · 2024-02-26T08:01:24Z

@elasticmachine merge upstream

Dosant

Looks good!
I'd still consider the other option with keeping the sync read/write #177537 (comment) . It seems simpler and smaller bug surface

kibana-ci · 2024-02-28T01:00:37Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: a471462

Failed CI Steps

FTR Configs #62

Test Failures

[job] [logs] FTR Configs #62 / Alerting transform alert rule types transform_health rule runs correctly

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
`@kbn/reporting-public`	107	104	-3

Async chunks

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

id	before	after	diff
`reporting`	74.1KB	74.1KB	-1.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`reporting`	42.8KB	43.0KB	+233.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/reporting-public`	113	110	-3

ESLint disabled line counts

id	before	after	diff
`reporting`	3	4	+1

Total ESLint disabled count

id	before	after	diff
`reporting`	3	4	+1

History

💚 Build #195406 succeeded c9d20e1
💚 Build #195338 succeeded 4fa2080
💛 Build #195126 was flaky 18016e8
💛 Build #195094 was flaky cc72631
💔 Build #195069 failed 4a7c2d2
💔 Build #195002 failed 641a4c2

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

cc @tsullivan

## Summary In previous code, Reporting fetched for a list of pending reports and compared the status of each result to a stored record of pending reports. The flakiness occurred because sometimes the results don't contain a record for a very "fresh" report since the refresh interval hadn't ticked and the stored change wasn't reflecting in search results. With [switching Reporting to use a data stream](elastic#176022), there was new flakiness in serverless tests. The area of flakiness was in getting Reporting to show the "Job completed" toast notification. The main difference faced with data streams is a lower index refresh interval, which is lower even more for serverless. This PR fixes the poller to loop through the list of stored pending reports and compare each to the result from the search. If a stored report isn't present in the search, we consider that to still be in the pending state. Other changes: - Improvements the class responsible for managing updates to the stored list of pending reports, so that each operation performs atomically and go through a queue to mitigate the chance of race conditions. - Update function names and variable names for readability - Remove the unused `removeJob` function. - Move helper code into private methods of the `StreamHandler` class and simplify `x-pack/plugins/reporting/public/plugin.ts` ## Release note Fixed an issue in Reporting with consistently showing the toast message for completed report jobs. --------- Co-authored-by: Kibana Machine <[email protected]>

Fix job notifications poller

bab5681

tsullivan marked this pull request as ready for review February 22, 2024 14:57

tsullivan requested a review from a team as a code owner February 22, 2024 14:57

tsullivan commented Feb 22, 2024

View reviewed changes

x-pack/plugins/reporting/public/lib/stream_handler.ts Outdated Show resolved Hide resolved

tsullivan commented Feb 22, 2024

View reviewed changes

x-pack/plugins/reporting/public/notifier/job_completion_notifications.ts Outdated Show resolved Hide resolved

updates to comments

641a4c2

tsullivan added the Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) label Feb 22, 2024

tsullivan added the release_note:fix label Feb 22, 2024

tsullivan mentioned this pull request Feb 22, 2024

Use Data Stream for Reporting storage #176022

Merged

7 tasks

Merge branch 'main' into reporting/public-job-poller-stabilization

4a7c2d2

tsullivan self-assigned this Feb 22, 2024

tsullivan and others added 4 commits February 22, 2024 11:27

fix a ts error from merge conflict

f35b856

Merge branch 'main' into reporting/public-job-poller-stabilization

a9c8fea

add unit test for update module

cc72631

Merge branch 'main' into reporting/public-job-poller-stabilization

18016e8

Dosant reviewed Feb 23, 2024

View reviewed changes

tsullivan added 3 commits February 23, 2024 09:32

Use revealing module pattern for jobCompletionNotifications

d771862

Update test using Sinon to jest mocks

f2268b2

Merge branch 'main' into reporting/public-job-poller-stabilization

4fa2080

Merge branch 'main' into reporting/public-job-poller-stabilization

c9d20e1

Dosant approved these changes Feb 26, 2024

View reviewed changes

tsullivan added 2 commits February 27, 2024 14:19

Merge branch 'main' into reporting/public-job-poller-stabilization

01815b9

Ditch the store mgmt queue and use synchronous loops

a471462

tsullivan force-pushed the reporting/public-job-poller-stabilization branch from 2a1cf2f to a471462 Compare February 27, 2024 22:45

tsullivan changed the title ~~Fix job notifications poller~~ [Reporting] Fix job notifications poller Feb 27, 2024

tsullivan enabled auto-merge (squash) February 27, 2024 22:51

tsullivan merged commit c1729c0 into elastic:main Feb 28, 2024
18 checks passed

kibanamachine added v8.14.0 backport:skip This commit does not require backporting labels Feb 28, 2024

tsullivan deleted the reporting/public-job-poller-stabilization branch April 30, 2024 21:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Reporting] Fix job notifications poller #177537

[Reporting] Fix job notifications poller #177537

tsullivan commented Feb 22, 2024 •

edited

Loading

tsullivan Feb 22, 2024

tsullivan Feb 22, 2024 •

edited

Loading

Dosant Feb 23, 2024

tsullivan Feb 23, 2024

tsullivan Feb 22, 2024 •

edited

Loading

tsullivan Feb 22, 2024

elasticmachine commented Feb 22, 2024

Dosant Feb 23, 2024

tsullivan Feb 23, 2024

Dosant Feb 23, 2024 •

edited

Loading

tsullivan Feb 23, 2024

Dosant Feb 26, 2024 •

edited

Loading

tsullivan Feb 27, 2024

Dosant Feb 23, 2024

Dosant commented Feb 26, 2024

Dosant left a comment

kibana-ci commented Feb 28, 2024

API count

ESLint disabled line counts

Total ESLint disabled count

[Reporting] Fix job notifications poller #177537

[Reporting] Fix job notifications poller #177537

Conversation

tsullivan commented Feb 22, 2024 • edited Loading

Summary

Release note

Choose a reason for hiding this comment

tsullivan Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsullivan Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Feb 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dosant Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dosant Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dosant commented Feb 26, 2024

Dosant left a comment

Choose a reason for hiding this comment

kibana-ci commented Feb 28, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Public APIs missing comments

Async chunks

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

tsullivan commented Feb 22, 2024 •

edited

Loading

tsullivan Feb 22, 2024 •

edited

Loading

tsullivan Feb 22, 2024 •

edited

Loading

Dosant Feb 23, 2024 •

edited

Loading

Dosant Feb 26, 2024 •

edited

Loading