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/New Platform] Use the logger service from core #55442

Merged
merged 9 commits into from
Jan 23, 2020

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Jan 21, 2020

Summary

Part of #53898

This PR integrates the core New Platform server-side logger with the legacy Reporting plugin.

NOTE: This breaks a technique that users have used to print verbose logs for Reporting.
This config:

logging.events.log: ['warning', 'error', 'fatal', 'info', 'reporting']

Will no longer show debug messages for Reporting. The workaround is to just use:

logging.verbose: true

If that becomes a breaking change when this code is released, then it should be documented. We'll have to keep an eye on this because maybe there will be a better solution before this code is released.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

}

public debug(msg: string, tags: string[] = []) {
this._logger([...this._tags, ...tags, 'debug'], trimStr(msg));
this.getLogger(tags).debug(msg);
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 am finding that no debug messages are getting logged with the changes from this branch.

@joshdover you asked me to track this for you. I'll give it another run and file a bug if needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #55456

@tsullivan tsullivan added enhancement New value added to drive a business result (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes Team:Reporting Services labels Jan 21, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

@tsullivan tsullivan force-pushed the reporting/np-server-logger branch 2 times, most recently from 08adc15 to 604dd2c Compare January 22, 2020 03:34
@tsullivan tsullivan marked this pull request as ready for review January 22, 2020 03:35
import { fieldFormatMapFactory } from './lib/field_format_map';
import { createGenerateCsv } from './lib/generate_csv';
Copy link
Member Author

Choose a reason for hiding this comment

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

Apologies, but this PR contains some automated changes of an IDE tool that organize the imports

@pgayvallet
Copy link
Contributor

Ack: will review tomorrow

await expectRejectedPromise(executeJob('job123', jobParams, cancellationToken));
await expect(
executeJob('job123', jobParams, cancellationToken)
).rejects.toMatchInlineSnapshot(`[TypeError: Cannot read property 'indexOf' of undefined]`);
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 seems a little strange. It's worth a look to see if this is the actual expected error

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

LGTM

Some NITs and comments:

Comment on lines +30 to +36
const mockLogger = new LevelLogger({
get: () => ({
debug: jest.fn(),
warn: jest.fn(),
error: jest.fn(),
}),
});
Copy link
Contributor

Choose a reason for hiding this comment

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

You can probably use core mocked logger implementation

import { loggingServiceMock } from '[..]/src/core/server/mocks';

 const mockLogger = new LevelLogger({
    get: () => loggingServiceMock.createLogger(),
  });

Comment on lines +89 to +90
const logger = new LevelLogger(this.initializerContext.logger.get('reporting'));
const browserDriverFactory = await createBrowserDriverFactory(__LEGACY, logger);
Copy link
Contributor

Choose a reason for hiding this comment

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

NOTE: the legacy initializerContext.logger.get returns a logger factory with empty context.

You have to be aware what once migrated to NP and using the actual NP plugin's initializerContext, this will be prefixed with the plugins context, meaning that this.initializerContext.logger.get('reporting') will returns the plugins.reporting-prefixed factory instead of just reporting as of now.

Comment on lines +30 to +32
function isResponse(response: Boom<null> | ResponseObject): response is ResponseObject {
return !(response as Boom<unknown>).isBoom;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: use isBoom instead (import { isBoom } from 'boom'; + return !isBoom(response);)

Comment on lines +157 to +160
if (statusCode !== 200) {
if (statusCode === 500) {
logger.error(`Report ${docId} has failed: ${JSON.stringify(response.source)}`);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: would remove one nesting level with if (statusCode === 500) elif (statusCode !== 200)

Comment on lines -23 to +24
const reportingFeatureId = getReportingFeatureId(request);
const reportingFeatureId = getReportingFeatureId(request) as string;
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: the signature seems to already define the return type, why was the force-cast necessary?

export type GetReportingFeatureIdFn = (request: Legacy.Request) => string;

Comment on lines +18 to +20
constructor(logger: LoggerFactory, tags?: string[]) {
this._logger = logger;
this._tags = tags;
this._tags = tags || [];
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: avoid || with constructor(logger: LoggerFactory, tags: string[] = []) {


const trimStr = (toTrim: string) => {
return typeof toTrim === 'string' ? toTrim.trim() : toTrim;
};

export class LevelLogger {
private _logger: any;
private _logger: LoggerFactory;
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: would name is _loggerFactory or _factory to be explicit about the type

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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


export const executeJobFactory: ExecuteJobFactory<ESQueueWorkerExecuteFn<
JobDocPayloadDiscoverCsv
>> = function executeJobFactoryFn(server: ServerFacade) {
>> = function executeJobFactoryFn(server: ServerFacade, parentLogger: Logger) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Man the >> threw me for a second. Smelled of merge conflict :/

@tsullivan tsullivan merged commit e102ae6 into elastic:master Jan 23, 2020
tsullivan added a commit to tsullivan/kibana that referenced this pull request Jan 23, 2020
)

* More Typescript Fixes

* [Reporting/New Platform] Use the logger service from core

* Add log tag

* fix jest tests

* ts fixes

* fix mocha test

* convert to jest
@tsullivan tsullivan deleted the reporting/np-server-logger branch January 23, 2020 21:28
tsullivan added a commit that referenced this pull request Jan 24, 2020
…) (#55751)

* [Reporting/New Platform] Use the logger service from core (#55442)

* More Typescript Fixes

* [Reporting/New Platform] Use the logger service from core

* Add log tag

* fix jest tests

* ts fixes

* fix mocha test

* convert to jest

* fix logger usage for legacy shim removed in 8.0
@tsullivan tsullivan mentioned this pull request Jan 29, 2020
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead enhancement New value added to drive a business result Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants