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] Add context to logging about Space ID handling #80106

Merged
merged 10 commits into from
Oct 13, 2020
Merged

[Reporting] Add context to logging about Space ID handling #80106

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
372ea5d
[Reporting] Logger Fixes
tsullivan Oct 9, 2020
e293e9d
Merge branch 'master' into reporting/logging-context-fixes
kibanamachine Oct 12, 2020
0f7b13a
info level to debug level for default space message
tsullivan Oct 12, 2020
d87c014
fix context for custom logo
tsullivan Oct 12, 2020
5fd2a19
fix tags
tsullivan Oct 12, 2020
cdf6514
Merge branch 'master' into reporting/logging-context-fixes
tsullivan Oct 13, 2020
c7bd5ca
Update x-pack/plugins/reporting/server/core.ts
tsullivan Oct 13, 2020
bfe0775
Merge branch 'reporting/logging-context-fixes' of github.com:tsulliva…
tsullivan Oct 13, 2020
fac1ddb
rm whitespace
tsullivan Oct 13, 2020
e8cbdb5
one more backtick string
tsullivan Oct 13, 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
16 changes: 8 additions & 8 deletions x-pack/plugins/reporting/server/core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,21 +195,21 @@ export class ReportingCore {
return scopedUiSettingsService;
}

public getSpaceId(request: KibanaRequest): string | undefined {
public getSpaceId(request: KibanaRequest, logger = this.logger): string | undefined {
const spacesService = this.getPluginSetupDeps().spaces?.spacesService;
if (spacesService) {
const spaceId = spacesService?.getSpaceId(request);

if (spaceId !== DEFAULT_SPACE_ID) {
this.logger.info(`Request uses Space ID: ` + spaceId);
logger.info(`Request uses Space ID: ` + spaceId);
tsullivan marked this conversation as resolved.
Show resolved Hide resolved
return spaceId;
} else {
this.logger.info(`Request uses default Space`);
logger.debug(`Request uses default Space`);
Copy link
Member Author

Choose a reason for hiding this comment

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

I find it better for this recently added log line to be debug rather than info.

}
}
}

public getFakeRequest(baseRequest: object, spaceId?: string) {
public getFakeRequest(baseRequest: object, spaceId: string | undefined, logger = this.logger) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Even though a logger is always passed in, I wanted to allow this.logger to be the default since this will now be the only reference to the logger member of the ReportingCore class. If there were no longer references to it, then it should not be declared. However - that process would create churn since we would want to add the logger back in as a member of the class when we need it while registering the task types, in an upcoming PR.

So, it might not seem important to keep the private logger class member, but it helps create a churn of subtractions and additions between different PRs in the code

const fakeRequest = KibanaRequest.from({
path: '/',
route: { settings: {} },
Expand All @@ -221,19 +221,19 @@ export class ReportingCore {
const spacesService = this.getPluginSetupDeps().spaces?.spacesService;
if (spacesService) {
if (spaceId && spaceId !== DEFAULT_SPACE_ID) {
this.logger.info(`Generating request for space: ` + spaceId);
logger.info(`Generating request for space: ` + spaceId);
tsullivan marked this conversation as resolved.
Show resolved Hide resolved
this.getPluginSetupDeps().basePath.set(fakeRequest, `/s/${spaceId}`);
}
}

return fakeRequest;
}

public async getUiSettingsClient(request: KibanaRequest) {
public async getUiSettingsClient(request: KibanaRequest, logger = this.logger) {
const spacesService = this.getPluginSetupDeps().spaces?.spacesService;
const spaceId = this.getSpaceId(request);
const spaceId = this.getSpaceId(request, logger);
if (spacesService && spaceId) {
this.logger.info(`Creating UI Settings Client for space: ${spaceId}`);
logger.info(`Creating UI Settings Client for space: ${spaceId}`);
}
const savedObjectsClient = await this.getSavedObjectsClient(request);
return await this.getUiSettingsServiceFactory(savedObjectsClient);
Expand Down
7 changes: 5 additions & 2 deletions x-pack/plugins/reporting/server/export_types/csv/create_job.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,17 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { CSV_JOB_TYPE } from '../../../constants';
import { cryptoFactory } from '../../lib';
import { CreateJobFn, CreateJobFnFactory } from '../../types';
import { IndexPatternSavedObject, JobParamsCSV, TaskPayloadCSV } from './types';

export const createJobFnFactory: CreateJobFnFactory<CreateJobFn<
JobParamsCSV,
TaskPayloadCSV
>> = function createJobFactoryFn(reporting) {
>> = function createJobFactoryFn(reporting, parentLogger) {
const logger = parentLogger.clone([CSV_JOB_TYPE, 'create-job']);

const config = reporting.getConfig();
const crypto = cryptoFactory(config.get('encryptionKey'));

Expand All @@ -26,7 +29,7 @@ export const createJobFnFactory: CreateJobFnFactory<CreateJobFn<

return {
headers: serializedEncryptedHeaders,
spaceId: reporting.getSpaceId(request),
spaceId: reporting.getSpaceId(request, logger),
indexPatternSavedObject,
...jobParams,
};
Expand Down
9 changes: 4 additions & 5 deletions x-pack/plugins/reporting/server/export_types/csv/execute_job.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,16 @@ export const runTaskFnFactory: RunTaskFnFactory<RunTaskFn<
TaskPayloadCSV
>> = function executeJobFactoryFn(reporting, parentLogger) {
const config = reporting.getConfig();
const logger = parentLogger.clone([CSV_JOB_TYPE, 'execute-job']);

return async function runTask(jobId, job, cancellationToken) {
const elasticsearch = reporting.getElasticsearchService();
const jobLogger = logger.clone([jobId]);
const generateCsv = createGenerateCsv(jobLogger);
const logger = parentLogger.clone([CSV_JOB_TYPE, 'execute-job', jobId]);
const generateCsv = createGenerateCsv(logger);

const encryptionKey = config.get('encryptionKey');
const headers = await decryptJobHeaders(encryptionKey, job.headers, logger);
const fakeRequest = reporting.getFakeRequest({ headers }, job.spaceId);
const uiSettingsClient = await reporting.getUiSettingsClient(fakeRequest);
const fakeRequest = reporting.getFakeRequest({ headers }, job.spaceId, logger);
const uiSettingsClient = await reporting.getUiSettingsClient(fakeRequest, logger);

const { callAsCurrentUser } = elasticsearch.legacy.client.asScoped(fakeRequest);
const callEndpoint = (endpoint: string, clientParams = {}, options = {}) =>
Expand Down
9 changes: 4 additions & 5 deletions x-pack/plugins/reporting/server/export_types/csv_from_savedobject/execute_job.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,10 @@ export const runTaskFnFactory: RunTaskFnFactory<ImmediateExecuteFn> = function e
const logger = parentLogger.clone([CSV_FROM_SAVEDOBJECT_JOB_TYPE, 'execute-job']);

return async function runTask(jobId, jobPayload, context, req) {
const jobLogger = logger.clone(['immediate']);
const generateCsv = createGenerateCsv(jobLogger);
const generateCsv = createGenerateCsv(logger);
const { panel, visType } = jobPayload;

jobLogger.debug(`Execute job generating [${visType}] csv`);
logger.debug(`Execute job generating [${visType}] csv`);

const savedObjectsClient = context.core.savedObjects.client;
const uiSettingsClient = await reporting.getUiSettingsServiceFactory(savedObjectsClient);
Expand All @@ -54,11 +53,11 @@ export const runTaskFnFactory: RunTaskFnFactory<ImmediateExecuteFn> = function e
);

if (csvContainsFormulas) {
jobLogger.warn(`CSV may contain formulas whose values have been escaped`);
logger.warn(`CSV may contain formulas whose values have been escaped`);
}

if (maxSizeReached) {
jobLogger.warn(`Max size reached: CSV output truncated to ${size} bytes`);
logger.warn(`Max size reached: CSV output truncated to ${size} bytes`);
}

return {
Expand Down
6 changes: 4 additions & 2 deletions x-pack/plugins/reporting/server/export_types/png/create_job/index.ts
View file
Open in desktop
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 { PNG_JOB_TYPE } from '../../../../constants';
import { cryptoFactory } from '../../../lib';
import { CreateJobFn, CreateJobFnFactory } from '../../../types';
import { validateUrls } from '../../common';
Expand All @@ -12,7 +13,8 @@ import { JobParamsPNG, TaskPayloadPNG } from '../types';
export const createJobFnFactory: CreateJobFnFactory<CreateJobFn<
JobParamsPNG,
TaskPayloadPNG
>> = function createJobFactoryFn(reporting) {
>> = function createJobFactoryFn(reporting, parentLogger) {
const logger = parentLogger.clone([PNG_JOB_TYPE, 'execute-job']);
const config = reporting.getConfig();
const crypto = cryptoFactory(config.get('encryptionKey'));

Expand All @@ -27,7 +29,7 @@ export const createJobFnFactory: CreateJobFnFactory<CreateJobFn<

return {
headers: serializedEncryptedHeaders,
spaceId: reporting.getSpaceId(req),
spaceId: reporting.getSpaceId(req, logger),
objectType,
title,
relativeUrl,
Expand Down
6 changes: 4 additions & 2 deletions x-pack/plugins/reporting/server/export_types/printable_pdf/create_job/index.ts
View file
Open in desktop
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 { PDF_JOB_TYPE } from '../../../../constants';
import { cryptoFactory } from '../../../lib';
import { CreateJobFn, CreateJobFnFactory } from '../../../types';
import { validateUrls } from '../../common';
Expand All @@ -12,9 +13,10 @@ import { JobParamsPDF, TaskPayloadPDF } from '../types';
export const createJobFnFactory: CreateJobFnFactory<CreateJobFn<
JobParamsPDF,
TaskPayloadPDF
>> = function createJobFactoryFn(reporting) {
>> = function createJobFactoryFn(reporting, parentLogger) {
const config = reporting.getConfig();
const crypto = cryptoFactory(config.get('encryptionKey'));
const logger = parentLogger.clone([PDF_JOB_TYPE, 'create-job']);

return async function createJob(
{ title, relativeUrls, browserTimezone, layout, objectType },
Expand All @@ -27,7 +29,7 @@ export const createJobFnFactory: CreateJobFnFactory<CreateJobFn<

return {
headers: serializedEncryptedHeaders,
spaceId: reporting.getSpaceId(req),
spaceId: reporting.getSpaceId(req, logger),
browserTimezone,
forceNow: new Date().toISOString(),
layout,
Expand Down
7 changes: 4 additions & 3 deletions x-pack/plugins/reporting/server/export_types/printable_pdf/execute_job/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,8 @@ export const runTaskFnFactory: RunTaskFnFactory<RunTaskFn<
const config = reporting.getConfig();
const encryptionKey = config.get('encryptionKey');

const logger = parentLogger.clone([PDF_JOB_TYPE, 'execute']);

return async function runTask(jobId, job, cancellationToken) {
const logger = parentLogger.clone([PDF_JOB_TYPE, 'execute-job', jobId]);
const apmTrans = apm.startTransaction('reporting execute_job pdf', 'reporting');
const apmGetAssets = apmTrans?.startSpan('get_assets', 'setup');
let apmGeneratePdf: { end: () => void } | null | undefined;
Expand All @@ -40,7 +39,9 @@ export const runTaskFnFactory: RunTaskFnFactory<RunTaskFn<
mergeMap(() => decryptJobHeaders(encryptionKey, job.headers, logger)),
map((decryptedHeaders) => omitBlockedHeaders(decryptedHeaders)),
map((filteredHeaders) => getConditionalHeaders(config, filteredHeaders)),
mergeMap((conditionalHeaders) => getCustomLogo(reporting, conditionalHeaders, job.spaceId)),
mergeMap((conditionalHeaders) =>
getCustomLogo(reporting, conditionalHeaders, job.spaceId, logger)
),
mergeMap(({ logo, conditionalHeaders }) => {
const urls = getFullUrls(config, job);

Expand Down
10 changes: 9 additions & 1 deletion x-pack/plugins/reporting/server/export_types/printable_pdf/lib/get_custom_logo.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { ReportingConfig, ReportingCore } from '../../../';
import {
createMockConfig,
createMockConfigSchema,
createMockLevelLogger,
createMockReportingCore,
} from '../../../test_helpers';
import { getConditionalHeaders } from '../../common';
Expand All @@ -16,6 +17,8 @@ import { getCustomLogo } from './get_custom_logo';
let mockConfig: ReportingConfig;
let mockReportingPlugin: ReportingCore;

const logger = createMockLevelLogger();

beforeEach(async () => {
mockConfig = createMockConfig(createMockConfigSchema());
mockReportingPlugin = await createMockReportingCore(mockConfig);
Expand All @@ -40,7 +43,12 @@ test(`gets logo from uiSettings`, async () => {

const conditionalHeaders = getConditionalHeaders(mockConfig, permittedHeaders);

const { logo } = await getCustomLogo(mockReportingPlugin, conditionalHeaders);
const { logo } = await getCustomLogo(
mockReportingPlugin,
conditionalHeaders,
'spaceyMcSpaceIdFace',
logger
);

expect(mockGet).toBeCalledWith('xpackReporting:customPdfLogo');
expect(logo).toBe('purple pony');
Expand Down
12 changes: 9 additions & 3 deletions x-pack/plugins/reporting/server/export_types/printable_pdf/lib/get_custom_logo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,21 @@

import { ReportingCore } from '../../../';
import { UI_SETTINGS_CUSTOM_PDF_LOGO } from '../../../../common/constants';
import { LevelLogger } from '../../../lib';
import { ConditionalHeaders } from '../../common';

export const getCustomLogo = async (
reporting: ReportingCore,
conditionalHeaders: ConditionalHeaders,
spaceId?: string
spaceId: string | undefined,
logger: LevelLogger
) => {
const fakeRequest = reporting.getFakeRequest({ headers: conditionalHeaders.headers }, spaceId);
const uiSettingsClient = await reporting.getUiSettingsClient(fakeRequest);
const fakeRequest = reporting.getFakeRequest(
{ headers: conditionalHeaders.headers },
spaceId,
logger
);
const uiSettingsClient = await reporting.getUiSettingsClient(fakeRequest, logger);

const logo: string = await uiSettingsClient.get(UI_SETTINGS_CUSTOM_PDF_LOGO);

Expand Down