Skip to content

Commit

Permalink
[Reporting] Fix job notifications poller (elastic#177537)
Browse files Browse the repository at this point in the history
## 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]>
  • Loading branch information
2 people authored and fkanout committed Mar 4, 2024
1 parent 3830c8a commit f82ff65
Show file tree
Hide file tree
Showing 8 changed files with 391 additions and 339 deletions.
39 changes: 39 additions & 0 deletions packages/kbn-reporting/public/job_completion_notifications.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { jobCompletionNotifications } from './job_completion_notifications';

describe('Job completion notifications', () => {
const { setPendingJobIds, getPendingJobIds, addPendingJobId } = jobCompletionNotifications();

afterEach(async () => {
setPendingJobIds([]);
});

it('initially contains not job IDs', async () => {
expect(getPendingJobIds()).toEqual([]);
});

it('handles multiple job ID additions', async () => {
addPendingJobId('job-123');
addPendingJobId('job-456');
addPendingJobId('job-789');
expect(getPendingJobIds()).toEqual(['job-123', 'job-456', 'job-789']);
});

it('handles setting a total of amount of job ID', async () => {
setPendingJobIds(['job-abc', 'job-def', 'job-ghi']);
expect(getPendingJobIds()).toEqual(['job-abc', 'job-def', 'job-ghi']);
});

it('able to clear all jobIds', async () => {
setPendingJobIds(['job-abc', 'job-def', 'job-ghi']);
setPendingJobIds([]);
expect(getPendingJobIds()).toEqual([]);
});
});
57 changes: 36 additions & 21 deletions packages/kbn-reporting/public/job_completion_notifications.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,29 +9,44 @@
import { JOB_COMPLETION_NOTIFICATIONS_SESSION_KEY } from '@kbn/reporting-common';
import { JobId } from '@kbn/reporting-common/types';

const set = (jobs: string[]) => {
sessionStorage.setItem(JOB_COMPLETION_NOTIFICATIONS_SESSION_KEY, JSON.stringify(jobs));
};

const getAll = (): string[] => {
const sessionValue = sessionStorage.getItem(JOB_COMPLETION_NOTIFICATIONS_SESSION_KEY);
return sessionValue ? JSON.parse(sessionValue) : [];
};
export function jobCompletionNotifications() {
function getPendingJobIds(): JobId[] {
const jobs: JobId[] = [];
// get all current jobs
for (const key in localStorage) {
// check if key belongs to us
if (key.indexOf(JOB_COMPLETION_NOTIFICATIONS_SESSION_KEY) === 0) {
// get jobId from key
const jobId = key.replace(`${JOB_COMPLETION_NOTIFICATIONS_SESSION_KEY}-`, '');
jobs.push(jobId);
}
}
return jobs;
}

export const add = (jobId: JobId) => {
const jobs = getAll();
jobs.push(jobId);
set(jobs);
};
function addPendingJobId(jobId: JobId) {
// write back to local storage, value doesn't matter
localStorage.setItem(`${JOB_COMPLETION_NOTIFICATIONS_SESSION_KEY}-${jobId}`, jobId);
}

export const remove = (jobId: JobId) => {
const jobs = getAll();
const index = jobs.indexOf(jobId);
function setPendingJobIds(jobIds: JobId[]) {
// clear reporting jobIds
for (const key in localStorage) {
if (key.indexOf(JOB_COMPLETION_NOTIFICATIONS_SESSION_KEY) === 0) {
localStorage.removeItem(key);
}
}

if (!index) {
throw new Error('Unable to find job to remove it');
// write update jobs back to local storage
for (let j = 0; j < jobIds.length; j++) {
const jobId = jobIds[j];
localStorage.setItem(`${JOB_COMPLETION_NOTIFICATIONS_SESSION_KEY}-${jobId}`, jobId);
}
}

jobs.splice(index, 1);
set(jobs);
};
return {
getPendingJobIds,
addPendingJobId,
setPendingJobIds,
};
}
10 changes: 6 additions & 4 deletions packages/kbn-reporting/public/reporting_api_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,18 @@ import type { HttpFetchQuery } from '@kbn/core/public';
import { HttpSetup, IUiSettingsClient } from '@kbn/core/public';
import { i18n } from '@kbn/i18n';
import {
INTERNAL_ROUTES,
PUBLIC_ROUTES,
REPORTING_MANAGEMENT_HOME,
buildKibanaPath,
getRedirectAppPath,
INTERNAL_ROUTES,
PUBLIC_ROUTES,
} from '@kbn/reporting-common';
import { BaseParams, JobId, ManagementLinkFn, ReportApiJSON } from '@kbn/reporting-common/types';
import rison from '@kbn/rison';
import moment from 'moment';
import { stringify } from 'query-string';
import { Job, add } from '.';
import { Job } from '.';
import { jobCompletionNotifications } from './job_completion_notifications';

/*
* For convenience, apps do not have to provide the browserTimezone and Kibana version.
Expand Down Expand Up @@ -66,6 +67,7 @@ interface IReportingAPI {
*/
export class ReportingAPIClient implements IReportingAPI {
private http: HttpSetup;
private addPendingJobId = jobCompletionNotifications().addPendingJobId;

constructor(
http: HttpSetup,
Expand Down Expand Up @@ -182,7 +184,7 @@ export class ReportingAPIClient implements IReportingAPI {
body: JSON.stringify({ jobParams: jobParamsRison }),
}
);
add(resp.job.id);
this.addPendingJobId(resp.job.id);
return new Job(resp.job);
}

Expand Down
Loading

0 comments on commit f82ff65

Please sign in to comment.