Implement base browser-side logging system #144107

pgayvallet · 2022-10-27T13:42:47Z

Summary

Part of #33796

Initial browser-side logging system implementation, as described in #33796 (comment)

Create the following packages:

@kbn/core-logging-common-internal : extracted common parts of the logging implementation that are now reused by both the server and browser logging systems.
@kbn/core-logging-browser-internal : the browser-side logging system implementation
@kbn/core-logging-browser-mocks : the associated mocks

…ide-logging

pgayvallet

Self review

pgayvallet · 2022-10-28T11:57:34Z

packages/core/logging/core-logging-common-internal/src/layouts/conversions/index.ts

+export { LoggerConversion } from './logger';
+export { LevelConversion } from './level';
+export { MessageConversion } from './message';
+export { MetaConversion } from './meta';
+export { DateConversion } from './date';


I moved the pattern conversions to the common package. A few notes though:

the pid conversion was kept on the server-side, given the concept cannot be used on browser side

the level and logger conversion were supporting highlighting using chalk. This only works with for terminal output (and was leaking server-only libraries into core's browser bundle), so I duplicated them and removed the highlight from the common version.

pgayvallet · 2022-10-28T11:58:35Z

packages/core/logging/core-logging-common-internal/src/layouts/pattern_layout.ts

+ * color highlighting (eg. to make log messages easier to read in the terminal).
+ * @internal
+ */
+export class PatternLayout implements Layout {


This base PatternLayout is inherited by the browser and server implementations.

pgayvallet · 2022-10-28T11:59:24Z

packages/core/logging/core-logging-common-internal/src/logger.ts

+ * A basic, abstract logger implementation that delegates the create of log records to the child's createLogRecord function.
+ * @internal
+ */
+export abstract class AbstractLogger implements Logger {


Same logic here, I created an abstract logger class that is used for the concrete browser/server implementations. The only abstract method to implement on the children is createLogRecord

pgayvallet · 2022-10-28T12:01:31Z

packages/core/logging/core-logging-browser-internal/src/layouts/pattern_layout.ts

+ * color highlighting (eg. to make log messages easier to read in the terminal).
+ * @internal
+ */
+export class PatternLayout extends BasePatternLayout {


Inheriting the base PatternLayout. Only the constructor is overridden to manually pass some parameters to it:

highlight: false given it's not supported on the browser

the browser-side list of supported conversions (excluding the pid one)

Is there a reason for renaming the base pattern layout here rather than in the common code?

pgayvallet · 2022-10-28T12:03:09Z

packages/core/logging/core-logging-browser-internal/src/logging_system.ts

+  private getLoggerConfigByContext(context: string): LoggerConfigType {
+    return {
+      level: this.loggingConfig.logLevel,
+      appenders: [CONSOLE_APPENDER_ID],
+      name: context,
+    };
+  }


As planned for the initial version, the browser-side logging configuration is very naive, and preconfigure all loggers to use the global level and a single console appender.

As an MVP, this is fine.

pgayvallet · 2022-10-28T12:05:05Z

packages/core/logging/core-logging-browser-internal/src/types.ts

+export interface LoggerConfigType {
+  appenders: string[];
+  name: string;
+  level: LogLevelId;
+}


This is a duplication from the type in the @kbn/core-logging-server package (packages/core/logging/core-logging-server/src/logger.ts).

When we'll eventually want to support full browser-side logging configuration, we will need to move all the logging config types from the @kbn/core-logging-server to a common one, but given this wasn't necessary for the first implementation, I simply copied the single type I'm using.

I created #144276 as a way to track enhancements and priority for further work in this area.

pgayvallet · 2022-10-28T12:36:14Z

packages/core/plugins/core-plugins-browser-internal/src/plugin_context.ts

+    logger: {
+      get(...contextParts) {
+        return coreContext.logger.get('plugins', pluginManifest.id, ...contextParts);
+      },
+    },


pluginInitConfig.logger.get('...') now available on the browser-side.

pgayvallet · 2022-10-28T12:45:02Z

packages/core/root/core-root-browser-internal/src/core_system.ts

+    const logLevel: LogLevelId = injectedMetadata.env.mode.dev ? 'all' : 'warn';
+    this.loggingSystem = new BrowserLoggingSystem({ logLevel });


Using the all level for development and warn level for production.

elasticmachine · 2022-10-28T13:54:19Z

Pinging @elastic/kibana-core (Team:Core)

…ide-logging

…-fix'

TinaHeiligers · 2022-10-31T22:05:31Z

packages/core/logging/core-logging-browser-internal/src/appenders/console_appender.ts

+   */
+  public append(record: LogRecord) {
+    // eslint-disable-next-line no-console
+    console.log(this.layout.format(record));


The MVP implements .log by default, which is fine to start with.

In #144276 we'd have a bit more flexibility in how we approach a fuller feature set. Assuming we'd expand on wrapping the console and, since the specifics of how the console object depends on the browser, we should at least implement a subset of the de facto methods that log to std out:
.error, .debug, .warn,.info

Yea. I didn't want to go into browser implementation details in the first version. I'll add a bullet point in #144276 to look at how it would be possible to have a proper browser console delegation using more methods

TinaHeiligers · 2022-10-31T22:48:15Z

packages/core/logging/core-logging-common-internal/src/logger.ts

+    protected readonly factory: LoggerFactory
+  ) {}
+
+  protected abstract createLogRecord<Meta extends LogMeta>(


TIL (again): protected vs private. I had to read up on protected vs private class methods. The difference is subtle.

Yea. I almost never use protected in typescript, but for abstract class, the abstract method definitions must be declared as protected, else typescript complains.

TinaHeiligers · 2022-10-31T22:49:33Z

packages/core/logging/core-logging-server-internal/src/appenders/console/console_appender.ts

@@ -7,7 +7,7 @@
 */

 import { schema } from '@kbn/config-schema';
-import { Layout, LogRecord, DisposableAppender } from '@kbn/logging';
+import type { Layout, LogRecord, DisposableAppender } from '@kbn/logging';


Nicely spotted!

TinaHeiligers · 2022-10-31T22:52:50Z

packages/core/logging/core-logging-server-internal/BUILD.bazel

@@ -50,10 +52,12 @@ TYPES_DEPS = [
  "@npm//rxjs",
  "@npm//@types/moment-timezone",
  "@npm//elastic-apm-node",
+  "@npm//chalk",


How did we migrate the logging service to packages without specifying @npm//chalk in the BUILD.bazel config? Even on main right now, there aren't any errors. 😕 Are you sure we're not inheriting the dependency from elsewhere?

Yea, I asked myself the same question... No idea to be honest.

TinaHeiligers

I left a few comments and one about an apparent 'missing' dependency regarding chalk that's a bit puzzling.

As an MVP for browser-side logging for dev, we don't need anything more than listed in #33796 (comment).

We can handle further enhancements at a later stage.

LGTM!

pgayvallet · 2022-11-01T07:17:45Z

@elasticmachine merge upstream

kibana-ci · 2022-11-01T08:16:37Z

💚 Build Succeeded

Buildkite Build
Commit: a76fdd5

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`core`	391	413	+22

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/core-logging-browser-mocks`	-	4	+4
`@kbn/core-logging-common-internal`	-	31	+31
`@kbn/core-plugins-browser`	10	11	+1
`core`	1201	1202	+1
total			+37

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`@kbn/core-logging-browser-internal`	-	1	+1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`core`	369.4KB	376.8KB	+7.4KB

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-logging-browser-mocks`	-	4	+4
`@kbn/core-logging-common-internal`	-	38	+38
`@kbn/core-plugins-browser`	14	15	+1
`core`	2703	2704	+1
total			+44

ESLint disabled in files

id	before	after	diff
`osquery`	1	2	+1

ESLint disabled line counts

id	before	after	diff
`@kbn/core-logging-browser-internal`	-	3	+3
`enterpriseSearch`	19	21	+2
`fleet`	57	63	+6
`osquery`	103	108	+5
`securitySolution`	439	443	+4
total			+20

Total ESLint disabled count

id	before	after	diff
`@kbn/core-logging-browser-internal`	-	3	+3
`enterpriseSearch`	20	22	+2
`fleet`	65	71	+6
`osquery`	104	110	+6
`securitySolution`	516	520	+4
total			+21

History

💚 Build #84060 succeeded 6fdd727
💔 Build #83812 failed d898143
💔 Build #83804 failed e7ff0b0
💚 Build #83512 succeeded 2fc7ff3
💚 Build #83475 succeeded 75b796a
💚 Build #83434 succeeded 5c480e0

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

* main: (43 commits) [Synthetics] Step details page screenshot (elastic#143452) [Lens] Datatable expression types improvement. (elastic#144173) [packages/kbn-journeys] start apm after browser start and stop after browser is closed (elastic#144267) [Files] Make files namespace agnostic (elastic#144019) Implement base browser-side logging system (elastic#144107) Correct wrong multiplier for byte conversion (elastic#143751) [Monaco] Add JSON syntax support to the Monaco editor (elastic#143739) CCS Smoke Test for Remote Clusters and Index Management (elastic#142423) [api-docs] Daily api_docs build (elastic#144294) chore(NA): include progress on Bazel tasks (elastic#144275) [RAM] Allow users to see event logs from all spaces they have access to (elastic#140449) [APM] Show recommended minimum size when going below 5 minutes (elastic#144170) [typecheck] delete temporary target_types dirs in packages (elastic#144271) [Security Solution][Endpoint] adds new alert loading utility and un-skip FTR test for endpoint (elastic#144133) [performance/journeys] revert data_stress_test_lens.ts journey step (elastic#144261) [TIP] Use search strategies in Threat Intelligence (elastic#143267) Optimize react-query dependencies (elastic#144206) [babel/node] invalidate cache when synth pkg map is updated (elastic#144258) [APM] AWS lambda estimated cost (elastic#143986) [Maps] layer group wizard (elastic#144129) ...

pgayvallet and others added 7 commits October 27, 2022 11:10

start refactoring and moving stuff around

1d6486f

add abstraction for BaseLogger

a26d1d5

plug logging into core system

63968e8

add logger factory to plugin init context

931c9e5

add unit tests for browser package

000da5b

add more unit tests

37f676b

[CI] Auto-commit changed files from 'node scripts/generate codeowners'

730ea22

pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Logging v8.6.0 labels Oct 27, 2022

pgayvallet and others added 10 commits October 28, 2022 08:42

fix mock

80fb9a2

Merge remote-tracking branch 'upstream/main' into kbn-33796-browser-s…

5c480e0

…ide-logging

add mocks, update system tests

c16eeb5

Merge remote-tracking branch 'upstream/main' into kbn-33796-browser-s…

1ee8dc1

…ide-logging

update files due to merge

066b87e

[CI] Auto-commit changed files from 'node scripts/generate codeowners'

94818ac

update readme and package

f59e2d0

remove chalk usages from client-side

75b796a

add plugin context tests

1564d05

Merge remote-tracking branch 'upstream/main' into kbn-33796-browser-s…

2fc7ff3

…ide-logging

pgayvallet commented Oct 28, 2022

View reviewed changes

pgayvallet marked this pull request as ready for review October 28, 2022 13:54

pgayvallet requested a review from a team as a code owner October 28, 2022 13:54

pgayvallet added the release_note:skip Skip the PR/issue when compiling release notes label Oct 28, 2022

pgayvallet and others added 3 commits October 31, 2022 08:38

Merge remote-tracking branch 'upstream/main' into kbn-33796-browser-s…

e7ff0b0

…ide-logging

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

d898143

…-fix'

fix new packages

6fdd727

TinaHeiligers mentioned this pull request Oct 31, 2022

Implement a full-feature browser-side logger #144276

Open

TinaHeiligers reviewed Oct 31, 2022

View reviewed changes

TinaHeiligers approved these changes Oct 31, 2022

View reviewed changes

pgayvallet mentioned this pull request Nov 1, 2022

[browser] Logging Service #33796

Closed

Merge branch 'main' into kbn-33796-browser-side-logging

a76fdd5

pgayvallet enabled auto-merge (squash) November 1, 2022 07:18

pgayvallet merged commit 2fb12fb into elastic:main Nov 1, 2022

kibanamachine added the backport:skip This commit does not require backporting label Nov 1, 2022

TinaHeiligers mentioned this pull request Dec 12, 2022

[On Week][Global Experience] Generate Large Dataset #146816

Closed

9 tasks

pgayvallet mentioned this pull request Oct 31, 2023

[EBT] Collect console errors #140933

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement base browser-side logging system #144107

Implement base browser-side logging system #144107

pgayvallet commented Oct 27, 2022 •

edited by TinaHeiligers

Loading

pgayvallet left a comment

pgayvallet Oct 28, 2022

pgayvallet Oct 28, 2022

pgayvallet Oct 28, 2022

pgayvallet Oct 28, 2022

TinaHeiligers Oct 31, 2022

pgayvallet Oct 28, 2022

TinaHeiligers Oct 31, 2022

pgayvallet Oct 28, 2022

TinaHeiligers Oct 31, 2022

pgayvallet Oct 28, 2022

pgayvallet Oct 28, 2022

elasticmachine commented Oct 28, 2022

TinaHeiligers Oct 31, 2022

pgayvallet Nov 1, 2022

TinaHeiligers Oct 31, 2022

pgayvallet Nov 1, 2022

TinaHeiligers Oct 31, 2022

TinaHeiligers Oct 31, 2022 •

edited

Loading

pgayvallet Nov 1, 2022

TinaHeiligers left a comment

pgayvallet commented Nov 1, 2022

kibana-ci commented Nov 1, 2022

API count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

		const logLevel: LogLevelId = injectedMetadata.env.mode.dev ? 'all' : 'warn';
		this.loggingSystem = new BrowserLoggingSystem({ logLevel });

Implement base browser-side logging system #144107

Implement base browser-side logging system #144107

Conversation

pgayvallet commented Oct 27, 2022 • edited by TinaHeiligers Loading

Summary

pgayvallet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Oct 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers Oct 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers left a comment

Choose a reason for hiding this comment

pgayvallet commented Nov 1, 2022

kibana-ci commented Nov 1, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Public APIs missing exports

Page load bundle

API count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

pgayvallet commented Oct 27, 2022 •

edited by TinaHeiligers

Loading

TinaHeiligers Oct 31, 2022 •

edited

Loading