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.

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

[Reporting/Config] allow string representations of duration settings #74202

Merged
merged 41 commits into from
Sep 17, 2020
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
6ecd79f
[Reporting/Config] use better schema methods
tsullivan Aug 3, 2020
0472d45
add createMockConfig
tsullivan Aug 17, 2020
a432276
update documentation
tsullivan Aug 19, 2020
ff00738
fix observable.test
tsullivan Aug 19, 2020
ce6eccb
Merge branch 'master' into reporting/config-use-duration
elasticmachine Aug 20, 2020
a40626b
add docs links to common options page
tsullivan Aug 20, 2020
88af5e2
make the schema match the docs
tsullivan Aug 20, 2020
2fd26c5
Merge branch 'master' into reporting/config-use-duration
tsullivan Aug 20, 2020
4273afc
Merge branch 'master' into reporting/config-use-duration
elasticmachine Aug 20, 2020
2157889
Merge branch 'master' into reporting/config-use-duration
elasticmachine Aug 21, 2020
f35ce37
Merge branch 'master' into reporting/config-use-duration
elasticmachine Aug 24, 2020
43f5ce0
Merge branch 'master' into reporting/config-use-duration
elasticmachine Aug 27, 2020
477ce75
Merge branch 'master' into reporting/config-use-duration
elasticmachine Aug 28, 2020
618ae45
Merge branch 'master' into reporting/config-use-duration
elasticmachine Aug 31, 2020
7aecbdc
Merge branch 'master' into reporting/config-use-duration
tsullivan Aug 31, 2020
7b221f5
wording edits per feedback
tsullivan Aug 31, 2020
7afa565
self edits
tsullivan Aug 31, 2020
1d6f4f9
Merge branch 'master' into reporting/config-use-duration
elasticmachine Sep 3, 2020
05debe2
todo comment
tsullivan Sep 3, 2020
2bb2566
Merge branch 'master' into reporting/config-use-duration
tsullivan Sep 9, 2020
556c71c
Merge commit '2bb25663528' into reporting/config-use-duration
tsullivan Sep 9, 2020
0d0f298
Merge branch 'master' into reporting/config-use-duration
tsullivan Sep 9, 2020
a2fab88
fix tests
tsullivan Sep 10, 2020
7515e09
Merge branch 'master' into reporting/config-use-duration
tsullivan Sep 15, 2020
bfcf1d8
Merge branch 'master' into reporting/config-use-duration
tsullivan Sep 16, 2020
5319844
feedback change 1
tsullivan Sep 16, 2020
2fdaf88
schema utils
tsullivan Sep 16, 2020
d9bed34
fix goof
tsullivan Sep 16, 2020
89c0b89
use objects for the defaults when available
tsullivan Sep 16, 2020
d06391e
fix pollInterval
tsullivan Sep 16, 2020
5bc26d9
fix snapshots
tsullivan Sep 16, 2020
79aef77
Update report_listing.tsx
tsullivan Sep 16, 2020
5fdcdea
Merge branch 'master' into reporting/config-use-duration
tsullivan Sep 16, 2020
e5f6a51
call new ByteSizeValue on server side only
tsullivan Sep 17, 2020
729d1e7
revert xpack.reporting.poll
tsullivan Sep 17, 2020
a5138e4
Merge branch 'master' into reporting/config-use-duration
tsullivan Sep 17, 2020
494fd05
Merge branch 'master' into reporting/config-use-duration
tsullivan Sep 17, 2020
3134289
fix ts
tsullivan Sep 17, 2020
a09f956
fix snapshot
tsullivan Sep 17, 2020
e78fae2
use correct input for duration
tsullivan Sep 17, 2020
7956662
revert reorganize imports
tsullivan Sep 17, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 20 additions & 28 deletions docs/settings/reporting-settings.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -104,15 +104,13 @@ security is enabled, `xpack.security.encryptionKey`.
[cols="2*<"]
|===
| `xpack.reporting.queue.pollInterval`
| Specifies the number of milliseconds that the reporting poller waits between polling the
index for any pending Reporting jobs. Defaults to `3000` (3 seconds).
| ({ref}/common-options.html#time-units[time value]) Specifies the time that the reporting poller waits between polling the index for any
tsullivan marked this conversation as resolved.
Show resolved Hide resolved
pending Reporting jobs. Can be specified as number of milliseconds. Defaults to `3s`.

| [[xpack-reporting-q-timeout]] `xpack.reporting.queue.timeout`
| How long each worker has to produce a report. If your machine is slow or under
heavy load, you might need to increase this timeout. Specified in milliseconds.
If a Reporting job execution time goes over this time limit, the job will be
marked as a failure and there will not be a download available.
Defaults to `120000` (two minutes).
| ({ref}/common-options.html#time-units[time value]) How long each worker has to produce a report. If your machine is slow or under heavy
load, you might need to increase this timeout. If a Reporting job execution time goes over this time limit, the job will be marked as a
tsullivan marked this conversation as resolved.
Show resolved Hide resolved
failure and there will not be a download available. Can be specified as number of milliseconds. Defaults to `2m`.
tsullivan marked this conversation as resolved.
Show resolved Hide resolved

|===

Expand All @@ -126,22 +124,19 @@ control the capturing process.
[cols="2*<"]
|===
| `xpack.reporting.capture.timeouts.openUrl`
| Specify how long to allow the Reporting browser to wait for the "Loading..." screen
to dismiss and find the initial data for the Kibana page. If the time is
exceeded, a page screenshot is captured showing the current state, and the download link shows a warning message.
Defaults to `60000` (1 minute).
| ({ref}/common-options.html#time-units[time value]) Specify how long to allow the Reporting browser to wait for the "Loading..." screen
to dismiss and find the initial data for the Kibana page. If the time is exceeded, a page screenshot is captured showing the current
tsullivan marked this conversation as resolved.
Show resolved Hide resolved
state, and the download link shows a warning message. Can be specified as number of milliseconds. Defaults to `1m`.

| `xpack.reporting.capture.timeouts.waitForElements`
| Specify how long to allow the Reporting browser to wait for all visualization
panels to load on the Kibana page. If the time is exceeded, a page screenshot
is captured showing the current state, and the download link shows a warning message. Defaults to `30000` (30
seconds).
| ({ref}/common-options.html#time-units[time value]) Specify how long to allow the Reporting browser to wait for all visualization panels
to load on the Kibana page. If the time is exceeded, a page screenshot is captured showing the current state, and the download link shows
tsullivan marked this conversation as resolved.
Show resolved Hide resolved
a warning message. Can be specified as number of milliseconds. Defaults to `30s`.

| `xpack.reporting.capture.timeouts.renderComplete`
| Specify how long to allow the Reporting browser to wait for all visualizations to
fetch and render the data. If the time is exceeded, a
page screenshot is captured showing the current state, and the download link shows a warning message. Defaults to
`30000` (30 seconds).
| ({ref}/common-options.html#time-units[time value]) Specify how long to allow the Reporting browser to wait for all visualizations to
fetch and render the data. If the time is exceeded, a page screenshot is captured showing the current state, and the download link shows a
warning message. Can be specified as number of milliseconds. Defaults to `30s`.

|===

Expand All @@ -160,11 +155,9 @@ available, but there will likely be errors in the visualizations in the report.
job, as many times as this setting. Defaults to `3`.

| `xpack.reporting.capture.loadDelay`
| When visualizations are not evented, this is the amount of time before
taking a screenshot. All visualizations that ship with {kib} are evented, so this
setting should not have much effect. If you are seeing empty images instead of
visualizations, try increasing this value.
Defaults to `3000` (3 seconds).
| ({ref}/common-options.html#time-units[time value]) When visualizations are not evented, this is the amount of time before taking a
tsullivan marked this conversation as resolved.
Show resolved Hide resolved
screenshot. All visualizations that ship with {kib} are evented, so this setting should not have much effect. If you are seeing empty
images instead of visualizations, try increasing this value. Defaults to `3s`.

| [[xpack-reporting-browser]] `xpack.reporting.capture.browser.type`
| Specifies the browser to use to capture screenshots. This setting exists for
Expand Down Expand Up @@ -206,17 +199,16 @@ When `xpack.reporting.capture.browser.type` is set to `chromium` (default) you c
[cols="2*<"]
|===
| [[xpack-reporting-csv]] `xpack.reporting.csv.maxSizeBytes`
| The maximum size of a CSV file before being truncated. This setting exists to prevent
large exports from causing performance and storage issues.
Defaults to `10485760` (10mB).
| ({ref}/common-options.html#byte-units[bytes value]) The maximum size of a CSV file before being truncated. This setting exists to
prevent large exports from causing performance and storage issues. Can be specified as number of bytes. Defaults to `10mb`.

| `xpack.reporting.csv.scroll.size`
| Number of documents retrieved from {es} for each scroll iteration during a CSV
export.
Defaults to `500`.

| `xpack.reporting.csv.scroll.duration`
| Amount of time allowed before {kib} cleans the scroll context during a CSV export.
| ({ref}/common-options.html#time-units[time value]) Amount of time allowed before {kib} cleans the scroll context during a CSV export.
Defaults to `30s`.

| `xpack.reporting.csv.checkForFormulas`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import moment from 'moment';
import React from 'react';
import { Observable } from 'rxjs';
import { mountWithIntl } from 'test_utils/enzyme_helpers';
Expand Down Expand Up @@ -53,11 +54,11 @@ const toasts = {

const mockPollConfig = {
jobCompletionNotifier: {
interval: 5000,
interval: moment.duration(5, 's'),
intervalErrorMultiplier: 3,
},
jobsRefresh: {
interval: 5000,
interval: moment.duration(5, 's'),
intervalErrorMultiplier: 3,
},
};
tsullivan marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
12 changes: 9 additions & 3 deletions x-pack/plugins/reporting/public/components/report_listing.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -165,17 +165,23 @@ class ReportListingUi extends Component<Props, State> {

public componentDidMount() {
this.mounted = true;
const { pollConfig, license$ } = this.props;

const pollFrequencyInMillis =
typeof pollConfig.jobsRefresh.interval === 'number'
? pollConfig.jobsRefresh.interval
: pollConfig.jobsRefresh.interval.asMilliseconds();
this.poller = new Poller({
functionToPoll: () => {
return this.fetchJobs();
},
pollFrequencyInMillis: this.props.pollConfig.jobsRefresh.interval,
pollFrequencyInMillis,
trailing: false,
continuePollingOnError: true,
pollFrequencyErrorMultiplier: this.props.pollConfig.jobsRefresh.intervalErrorMultiplier,
pollFrequencyErrorMultiplier: pollConfig.jobsRefresh.intervalErrorMultiplier,
});
this.poller.start();
this.licenseSubscription = this.props.license$.subscribe(this.licenseHandler);
this.licenseSubscription = license$.subscribe(this.licenseHandler);
}

private licenseHandler = (license: ILicense) => {
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/reporting/public/plugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,8 @@ export class ReportingPublicPlugin implements Plugin<void, void> {
const { http, notifications } = core;
const apiClient = new ReportingAPIClient(http);
const streamHandler = new StreamHandler(notifications, apiClient);
const { interval } = this.config.poll.jobsRefresh;

const { interval: intervalRaw } = this.config.poll.jobsRefresh;
const interval = typeof intervalRaw === 'number' ? intervalRaw : intervalRaw.asMilliseconds();
tsullivan marked this conversation as resolved.
Show resolved Hide resolved
Rx.timer(0, interval)
.pipe(
takeUntil(this.stop$), // stop the interval when stop method is called
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,9 @@ export class HeadlessChromiumDriverFactory {

// Set the default timeout for all navigation methods to the openUrl timeout (30 seconds)
// All waitFor methods have their own timeout config passed in to them
page.setDefaultTimeout(this.captureConfig.timeouts.openUrl);
const timeoutRaw = this.captureConfig.timeouts.openUrl;
const timeout = typeof timeoutRaw === 'number' ? timeoutRaw : timeoutRaw.asMilliseconds();
page.setDefaultTimeout(timeout);

logger.debug(`Browser page driver created`);
} catch (err) {
Expand Down
31 changes: 31 additions & 0 deletions x-pack/plugins/reporting/server/config/schema.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,37 @@ describe('Reporting Config Schema', () => {
});
});

it('allows Duration values for certain keys', () => {
expect(ConfigSchema.validate({ queue: { timeout: '2m' } }).queue.timeout).toMatchInlineSnapshot(
`"PT2M"`
);

expect(
ConfigSchema.validate({ capture: { loadDelay: '3s' } }).capture.loadDelay
).toMatchInlineSnapshot(`"PT3S"`);

expect(
ConfigSchema.validate({
capture: { timeouts: { openUrl: '1m', waitForElements: '30s', renderComplete: '10s' } },
}).capture.timeouts
).toMatchInlineSnapshot(`
Object {
"openUrl": "PT1M",
"renderComplete": "PT10S",
"waitForElements": "PT30S",
}
`);
});

it('allows ByteSizeValue values for certain keys', () => {
expect(ConfigSchema.validate({ csv: { maxSizeBytes: '12mb' } }).csv.maxSizeBytes)
.toMatchInlineSnapshot(`
ByteSizeValue {
"valueInBytes": 12582912,
}
`);
});

it(`allows optional settings`, () => {
// encryption key
expect(
Expand Down
29 changes: 11 additions & 18 deletions x-pack/plugins/reporting/server/config/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { schema, TypeOf } from '@kbn/config-schema';
import moment from 'moment';
import { schema, ByteSizeValue, TypeOf } from '@kbn/config-schema';

const KibanaServerSchema = schema.object({
hostname: schema.maybe(
Expand Down Expand Up @@ -35,7 +34,7 @@ const QueueSchema = schema.object({
pollEnabled: schema.boolean({ defaultValue: true }),
pollInterval: schema.number({ defaultValue: 3000 }),
pollIntervalErrorMultiplier: schema.number({ defaultValue: 10 }),
timeout: schema.number({ defaultValue: moment.duration(2, 'm').asMilliseconds() }),
timeout: schema.oneOf([schema.number(), schema.duration()], { defaultValue: 120000 }),
Copy link
Member Author

@tsullivan tsullivan Aug 19, 2020

Choose a reason for hiding this comment

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

defaultValue has to be a value that satisfies both schemas, so it must be a number.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's odd, seems like it should just need to satisfy one

});

const RulesSchema = schema.object({
Expand All @@ -46,9 +45,9 @@ const RulesSchema = schema.object({

const CaptureSchema = schema.object({
timeouts: schema.object({
openUrl: schema.number({ defaultValue: 60000 }),
waitForElements: schema.number({ defaultValue: 30000 }),
renderComplete: schema.number({ defaultValue: 30000 }),
openUrl: schema.oneOf([schema.number(), schema.duration()], { defaultValue: 30000 }),
waitForElements: schema.oneOf([schema.number(), schema.duration()], { defaultValue: 30000 }),
renderComplete: schema.oneOf([schema.number(), schema.duration()], { defaultValue: 30000 }),
}),
networkPolicy: schema.object({
enabled: schema.boolean({ defaultValue: true }),
Expand All @@ -68,9 +67,7 @@ const CaptureSchema = schema.object({
width: schema.number({ defaultValue: 1950 }),
height: schema.number({ defaultValue: 1200 }),
}),
loadDelay: schema.number({
defaultValue: moment.duration(3, 's').asMilliseconds(),
}), // TODO: use schema.duration
loadDelay: schema.oneOf([schema.number(), schema.duration()], { defaultValue: 3000 }),
browser: schema.object({
autoDownload: schema.conditional(
schema.contextRef('dist'),
Expand Down Expand Up @@ -116,9 +113,9 @@ const CsvSchema = schema.object({
checkForFormulas: schema.boolean({ defaultValue: true }),
escapeFormulaValues: schema.boolean({ defaultValue: false }),
enablePanelActionDownload: schema.boolean({ defaultValue: true }),
maxSizeBytes: schema.number({
defaultValue: 1024 * 1024 * 10, // 10MB
}), // TODO: use schema.byteSize
maxSizeBytes: schema.oneOf([schema.number(), schema.byteSize()], {
defaultValue: ByteSizeValue.parse('10mb').getValueInBytes(),
}),
useByteOrderMarkEncoding: schema.boolean({ defaultValue: false }),
scroll: schema.object({
duration: schema.string({
Expand Down Expand Up @@ -148,15 +145,11 @@ const IndexSchema = schema.string({ defaultValue: '.reporting' });

const PollSchema = schema.object({
jobCompletionNotifier: schema.object({
interval: schema.number({
defaultValue: moment.duration(10, 's').asMilliseconds(),
}), // TODO: use schema.duration
interval: schema.oneOf([schema.number(), schema.duration()], { defaultValue: 10000 }),
intervalErrorMultiplier: schema.number({ defaultValue: 5 }),
}),
jobsRefresh: schema.object({
interval: schema.number({
defaultValue: moment.duration(5, 's').asMilliseconds(),
}), // TODO: use schema.duration
interval: schema.oneOf([schema.number(), schema.duration()], { defaultValue: 5000 }),
intervalErrorMultiplier: schema.number({ defaultValue: 5 }),
}),
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,28 +4,24 @@
* you may not use this file except in compliance with the Elastic License.
*/

import sinon from 'sinon';
import { ReportingConfig } from '../../';
import { ReportingCore } from '../../core';
import { createMockReportingCore } from '../../test_helpers';
import {
createMockConfig,
createMockConfigSchema,
createMockReportingCore,
} from '../../test_helpers';
import { ScheduledTaskParams } from '../../types';
import { ScheduledTaskParamsPDF } from '../printable_pdf/types';
import { getConditionalHeaders, getCustomLogo } from './';

let mockConfig: ReportingConfig;
let mockReportingPlugin: ReportingCore;

const getMockConfig = (mockConfigGet: sinon.SinonStub) => ({
get: mockConfigGet,
kbnConfig: { get: mockConfigGet },
});

beforeEach(async () => {
const mockConfigGet = sinon
.stub()
.withArgs('kibanaServer', 'hostname')
.returns('custom-hostname');
mockConfig = getMockConfig(mockConfigGet);
const reportingConfig = { kibanaServer: { hostname: 'custom-hostname' } };
const mockSchema = createMockConfigSchema(reportingConfig);
mockConfig = createMockConfig(mockSchema);
mockReportingPlugin = await createMockReportingCore(mockConfig);
});

Expand All @@ -36,7 +32,7 @@ describe('conditions', () => {
baz: 'quix',
};

const conditionalHeaders = await getConditionalHeaders({
const conditionalHeaders = getConditionalHeaders({
job: {} as ScheduledTaskParams<any>,
filteredHeaders: permittedHeaders,
config: mockConfig,
Expand All @@ -63,7 +59,7 @@ test('uses basePath from job when creating saved object service', async () => {
foo: 'bar',
baz: 'quix',
};
const conditionalHeaders = await getConditionalHeaders({
const conditionalHeaders = getConditionalHeaders({
job: {} as ScheduledTaskParams<any>,
filteredHeaders: permittedHeaders,
config: mockConfig,
Expand All @@ -84,16 +80,15 @@ test(`uses basePath from server if job doesn't have a basePath when creating sav
const mockGetSavedObjectsClient = jest.fn();
mockReportingPlugin.getSavedObjectsClient = mockGetSavedObjectsClient;

const mockConfigGet = sinon.stub();
mockConfigGet.withArgs('kibanaServer', 'hostname').returns('localhost');
mockConfigGet.withArgs('server', 'basePath').returns('/sbp');
mockConfig = getMockConfig(mockConfigGet);
const reportingConfig = { kibanaServer: { hostname: 'localhost' }, server: { basePath: '/sbp' } };
const mockSchema = createMockConfigSchema(reportingConfig);
mockConfig = createMockConfig(mockSchema);

const permittedHeaders = {
foo: 'bar',
baz: 'quix',
};
const conditionalHeaders = await getConditionalHeaders({
const conditionalHeaders = getConditionalHeaders({
job: {} as ScheduledTaskParams<any>,
filteredHeaders: permittedHeaders,
config: mockConfig,
Expand Down Expand Up @@ -134,25 +129,12 @@ test(`uses basePath from server if job doesn't have a basePath when creating sav
});

describe('config formatting', () => {
test(`lowercases server.host`, async () => {
const mockConfigGet = sinon.stub().withArgs('server', 'host').returns('COOL-HOSTNAME');
Copy link
Member Author

@tsullivan tsullivan Aug 19, 2020

Choose a reason for hiding this comment

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

This test runs the same code as the "lowercase" test right below. It was only passing because of the an odd way the stub function worked.

mockConfig = getMockConfig(mockConfigGet);

const conditionalHeaders = await getConditionalHeaders({
job: {} as ScheduledTaskParams<any>,
filteredHeaders: {},
config: mockConfig,
});
expect(conditionalHeaders.conditions.hostname).toEqual('cool-hostname');
});

test(`lowercases kibanaServer.hostname`, async () => {
const mockConfigGet = sinon
.stub()
.withArgs('kibanaServer', 'hostname')
.returns('GREAT-HOSTNAME');
mockConfig = getMockConfig(mockConfigGet);
const conditionalHeaders = await getConditionalHeaders({
const reportingConfig = { kibanaServer: { hostname: 'GREAT-HOSTNAME' } };
const mockSchema = createMockConfigSchema(reportingConfig);
mockConfig = createMockConfig(mockSchema);

const conditionalHeaders = getConditionalHeaders({
job: {
title: 'cool-job-bro',
type: 'csv',
Expand Down
Loading