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

[browser logging] allow to configure root level #176397

Merged
merged 8 commits into from
Feb 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions config/kibana.yml
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,10 @@
# - name: metrics.ops
# level: debug

# Enables debug logging on the browser (dev console)
#logging.browser.root:
# level: debug
Comment on lines +136 to +138
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure it was really needed given right now it's mostly for internal/development usage, but I still added a snippet in the config file.


# =================== System: Other ===================
# The path where Kibana stores persistent data not saved in Elasticsearch. Defaults to data
#path.data: data
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,12 +133,9 @@ export class AnalyticsClient implements IAnalyticsClient {
properties: eventData as unknown as Record<string, unknown>,
};

// debug-logging before checking the opt-in status to help during development
if (this.initContext.isDev) {
this.initContext.logger.debug<EventDebugLogMeta>(`Report event "${eventType}"`, {
ebt_event: event,
});
}
this.initContext.logger.debug<EventDebugLogMeta>(`Report event "${eventType}"`, {
ebt_event: event,
});
Comment on lines +136 to +138
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer needs this isDev check now that the default level is info and is configurable.

Copy link
Contributor

Choose a reason for hiding this comment

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


const optInConfig = this.optInConfig$.value;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import type { PluginName, DiscoveredPlugin } from '@kbn/core-base-common';
import type { ThemeVersion } from '@kbn/ui-shared-deps-npm';
import type { EnvironmentMode, PackageInfo } from '@kbn/config';
import type { CustomBranding } from '@kbn/core-custom-branding-common';
import type { BrowserLoggingConfig } from '@kbn/core-logging-common-internal';

/** @internal */
export interface InjectedMetadataClusterInfo {
Expand Down Expand Up @@ -45,6 +46,7 @@ export interface InjectedMetadata {
publicBaseUrl?: string;
assetsHrefBase: string;
clusterInfo: InjectedMetadataClusterInfo;
logging: BrowserLoggingConfig;
env: {
mode: EnvironmentMode;
packageInfo: PackageInfo;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
"@kbn/config",
"@kbn/ui-shared-deps-npm",
"@kbn/core-base-common",
"@kbn/core-custom-branding-common"
"@kbn/core-custom-branding-common",
"@kbn/core-logging-common-internal"
],
"exclude": [
"target/**/*",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,29 @@
* Side Public License, v 1.
*/

import type { BrowserLoggingConfig } from '@kbn/core-logging-common-internal';
import { unsafeConsole } from '@kbn/security-hardening';
import { BrowserLoggingSystem } from './logging_system';

describe('', () => {
describe('BrowserLoggingSystem', () => {
const timestamp = new Date(Date.UTC(2012, 1, 1, 14, 33, 22, 11));

let mockConsoleLog: jest.SpyInstance;
let loggingSystem: BrowserLoggingSystem;

const createLoggingConfig = (parts: Partial<BrowserLoggingConfig> = {}): BrowserLoggingConfig => {
return {
root: {
level: 'warn',
},
...parts,
};
};

beforeEach(() => {
mockConsoleLog = jest.spyOn(unsafeConsole, 'log').mockReturnValue(undefined);
jest.spyOn<any, any>(global, 'Date').mockImplementation(() => timestamp);
loggingSystem = new BrowserLoggingSystem({ logLevel: 'warn' });
loggingSystem = new BrowserLoggingSystem(createLoggingConfig());
});

afterEach(() => {
Expand Down Expand Up @@ -74,5 +84,37 @@ describe('', () => {
]
`);
});

it('allows to override the root logger level', () => {
loggingSystem = new BrowserLoggingSystem(createLoggingConfig({ root: { level: 'debug' } }));

const logger = loggingSystem.get('foo.bar');
logger.trace('some trace message');
logger.debug('some debug message');
logger.info('some info message');
logger.warn('some warn message');
logger.error('some error message');
logger.fatal('some fatal message');

expect(mockConsoleLog.mock.calls).toMatchInlineSnapshot(`
Array [
Array [
"[2012-01-31T13:33:22.011-05:00][DEBUG][foo.bar] some debug message",
],
Array [
"[2012-01-31T08:33:22.011-05:00][INFO ][foo.bar] some info message",
],
Array [
"[2012-01-31T03:33:22.011-05:00][WARN ][foo.bar] some warn message",
],
Array [
"[2012-01-30T22:33:22.011-05:00][ERROR][foo.bar] some error message",
],
Array [
"[2012-01-30T17:33:22.011-05:00][FATAL][foo.bar] some fatal message",
],
]
`);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,13 @@
* Side Public License, v 1.
*/

import { LogLevel, Logger, LoggerFactory, LogLevelId, DisposableAppender } from '@kbn/logging';
import { getLoggerContext } from '@kbn/core-logging-common-internal';
import { LogLevel, Logger, LoggerFactory, DisposableAppender } from '@kbn/logging';
import { getLoggerContext, BrowserLoggingConfig } from '@kbn/core-logging-common-internal';
import type { LoggerConfigType } from './types';
import { BaseLogger } from './logger';
import { PatternLayout } from './layouts';
import { ConsoleAppender } from './appenders';

export interface BrowserLoggingConfig {
logLevel: LogLevelId;
}

const CONSOLE_APPENDER_ID = 'console';

/**
Expand Down Expand Up @@ -54,7 +50,7 @@ export class BrowserLoggingSystem implements IBrowserLoggingSystem {

private getLoggerConfigByContext(context: string): LoggerConfigType {
return {
level: this.loggingConfig.logLevel,
level: this.loggingConfig.root.level,
appenders: [CONSOLE_APPENDER_ID],
name: context,
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,4 @@ export {
ROOT_CONTEXT_NAME,
DEFAULT_APPENDER_NAME,
} from './src';
export type { BrowserLoggingConfig, BrowserRootLoggerConfig } from './src/browser_config';
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/*
* 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 type { LogLevelId } from '@kbn/logging';

/**
* @internal
*/
export interface BrowserLoggingConfig {
root: BrowserRootLoggerConfig;
}

/**
* @internal
*/
export interface BrowserRootLoggerConfig {
level: LogLevelId;
}
Comment on lines +11 to +23
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Base types for the browser-side logging config. It's simplistic right now, but more fields should appear when we go further on #144276

Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
export { config } from './src/logging_config';
export type {
LoggingConfigType,
LoggingConfigWithBrowserType,
loggerContextConfigSchema,
loggerSchema,
} from './src/logging_config';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ test('`schema` creates correct schema with defaults.', () => {
expect(config.schema.validate({})).toMatchInlineSnapshot(`
Object {
"appenders": Map {},
"browser": Object {
"root": Object {
"level": "info",
},
},
"loggers": Array [],
"root": Object {
"appenders": Array [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ export const loggerSchema = schema.object({
level: levelSchema,
});

const browserConfig = schema.object({
root: schema.object({
level: levelSchema,
}),
});

export const config = {
path: 'logging',
schema: schema.object({
Expand All @@ -63,6 +69,7 @@ export const config = {
}),
level: levelSchema,
}),
browser: browserConfig,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm exposing the browser logging configuration via the logging.browser config prefix. So the browser-side config is "included" in the server-side logging prefix that is logging.

I wasn't sure, but in the end I still think it makes sense to have a single prefix (and that it isn't worth introducing heavy changes to move the server-side logging under the logging.server prefix either), but please tell if you think otherwise or have another idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to have them symmetrical, but I think logging.browser.* is fine

}),
};

Expand All @@ -71,6 +78,10 @@ export type LoggingConfigType = Pick<TypeOf<typeof config.schema>, 'loggers' | '
appenders: Map<string, AppenderConfigType>;
};

/** @internal */
export type LoggingConfigWithBrowserType = LoggingConfigType &
Pick<TypeOf<typeof config.schema>, 'browser'>;

/**
* Config schema for validating the inputs to the {@link LoggingServiceStart.configure} API.
* See {@link LoggerContextConfigType}.
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,16 @@
* Side Public License, v 1.
*/

import { firstValueFrom } from 'rxjs';
import UiSharedDepsNpm from '@kbn/ui-shared-deps-npm';
import * as UiSharedDepsSrc from '@kbn/ui-shared-deps-src';
import type { IConfigService } from '@kbn/config';
import type { BrowserLoggingConfig } from '@kbn/core-logging-common-internal';
import type { UiSettingsParams, UserProvidedValues } from '@kbn/core-ui-settings-common';
import {
config as loggingConfigDef,
type LoggingConfigWithBrowserType,
} from '@kbn/core-logging-server-internal';

export const getSettingValue = <T>(
settingName: string,
Expand Down Expand Up @@ -54,3 +61,12 @@ export const getStylesheetPaths = ({
]),
];
};

export const getBrowserLoggingConfig = async (
configService: IConfigService
): Promise<BrowserLoggingConfig> => {
const loggingConfig = await firstValueFrom(
configService.atPath<LoggingConfigWithBrowserType>(loggingConfigDef.path)
);
return loggingConfig.browser;
};
Loading