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

Introduce Appenders and Loggers. #12852

Merged

Conversation

azasypkin
Copy link
Member

@azasypkin azasypkin commented Jul 14, 2017

Fixes #13098.
Fixes #13105.

The underlying idea of separate appenders and loggers is inspired by log4j, log4rs and log4js logging frameworks to be similar to Elasticserach logging.

@@ -102,3 +102,36 @@
# The default locale. This locale can be used in certain circumstances to substitute any missing
# translations.
#i18n.defaultLocale: "en"

Copy link
Member Author

@azasypkin azasypkin Jul 14, 2017

Choose a reason for hiding this comment

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

Just a sample of config I use for testing.

@@ -0,0 +1,55 @@
import { assertNever } from '../lib/utils';
Copy link
Member Author

Choose a reason for hiding this comment

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

logging folder currently co-exists with logger folder, I keep them both for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 We can remove it when we're done reviewing this PR

import { assertNever } from '../lib/utils';

export type LogLevelId =
| 'all'
Copy link
Member Author

@azasypkin azasypkin Jul 14, 2017

Choose a reason for hiding this comment

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

all and off logging levels are quite handy when you want to disable specific logger entirely (off) or allow everything to be logged (all).


export class LogLevel {
static Off = new LogLevel('off', 1);
static Fatal = new LogLevel('fatal', 2, 'red');
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'm planning to get rid of color from the LogLevel since it seems to be only related to ConsoleAppender.


export class MutableLoggerFactory implements LoggerFactory {
private config?: LoggingConfig;
private readonly appenders: Map<string, BaseAppender> = new Map([
Copy link
Member Author

Choose a reason for hiding this comment

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

That the initial idea on how to handle log before config is loaded, works well for now.

constructor(private readonly loggingFactory: MutableLoggerFactory) {}

upgrade(config$: Observable<LoggingConfig>) {
config$.takeUntil(this.stop$).subscribe({
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 don't close LoggingFactory in complete handler anymore because of the fact that it's called when RawLoggingService is closed that happens before we stop other subsystems (eg. plugins).

import { FileAppenderConfig } from './appenders/file/FileAppenderConfig';
import { RollingFileAppenderConfig } from './appenders/rolling-file/RollingFileAppenderConfig';

const KNOWN_APPENDERS = new Map<string, new (schema: BaseAppenderConfigSchemaType) => BaseAppenderConfig>([
Copy link
Member Author

Choose a reason for hiding this comment

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

Still not sure how to register known appenders.....

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean register other appenders or the ones we have in core (listed below)?

Copy link
Member Author

@azasypkin azasypkin Jul 14, 2017

Choose a reason for hiding this comment

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

I was thinking about both... but not sure if we ever want to allow plugins to add new loggers.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the ES approach here? If ES doesn't, we likely shouldn't either. Most things can be solved using beats etc, where there is already a great system for handling these things

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point (and question), I'll explore what ES does here.

@@ -0,0 +1,24 @@
import { LogRecord } from '../../LogRecord';
Copy link
Member Author

@azasypkin azasypkin Jul 14, 2017

Choose a reason for hiding this comment

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

There is a chance that I get rid of additional folders for appenders and their config. But it seems that rolling-file-appender will require a lot of additional code (roll over policy handling) and having everything in one file can be a mess...

};
}

const schemaType = typeOfSchema(() => schema.object(getRawSchema()));
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'm not sure if I like this piece of code (and similar one for sub-classes), but I don't see better way right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's zoom and pair on improving it

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I need your help here :)

@kimjoar kimjoar self-requested a review July 14, 2017 08:19
Copy link
Contributor

@kimjoar kimjoar left a comment

Choose a reason for hiding this comment

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

This is an awesome start. Let's zoom early next week and go through it

@@ -0,0 +1,55 @@
import { assertNever } from '../lib/utils';
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 We can remove it when we're done reviewing this PR

import { FileAppenderConfig } from './appenders/file/FileAppenderConfig';
import { RollingFileAppenderConfig } from './appenders/rolling-file/RollingFileAppenderConfig';

const KNOWN_APPENDERS = new Map<string, new (schema: BaseAppenderConfigSchemaType) => BaseAppenderConfig>([
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean register other appenders or the ones we have in core (listed below)?

};
}

const schemaType = typeOfSchema(() => schema.object(getRawSchema()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's zoom and pair on improving it

private getLoggerConfigByContext(config: LoggingConfig, context: string) {
// If we don't have dedicated configuration for specified context, let's use config from `root` context,
// that is always should be defined (enforced by configuration schema).
return config.loggers.get(config.loggers.has(context) ? context : 'root')!;
Copy link
Contributor

Choose a reason for hiding this comment

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

If context is server.foo and we have a config.loggers registered for server, I think that's the expected config, not the root config.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we also need the ability to override the log level itself? e.g. for ES: https://www.elastic.co/guide/en/elasticsearch/reference/5.1/misc-cluster.html#cluster-logger.

That would likely have to be a different config, though

Copy link
Member Author

Choose a reason for hiding this comment

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

If context is server.foo and we have a config.loggers registered for server, I think that's the expected config, not the root config.

Heh, exactly, it's just not implemented yet ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we also need the ability to override the log level itself? e.g. for ES: https://www.elastic.co/guide/en/elasticsearch/reference/5.1/misc-cluster.html#cluster-logger.

That would likely have to be a different config, though

I think once I handle your first comment above it will be possible, like this (appenders should become optional though, and if none of the logger defines appenders, the ones from root should be used, I'll think how to make that easier):


logging:
  appenders:
    console:
      kind: console
      pattern: "[{timestamp}][{level}][{context}] {message}"
  loggers:
    server:
      appenders: [server-console]
      level: all
    server.foo:
      level: debug

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, something like that could work

Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh, if we use . as context separator it seems we can't really define a schema for it (since we expand these properties into nested objects and in the example above server schema should allow level, appenders and foo properties), unless I'm missing something. If that's true, maybe we can use another separator (eg. server:foo or server::foo)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is like yaml will look in this case: https://docs.rs/log4rs/0.7.0/log4rs/#examples

this.path = schema.path;
}

createAppender(): FileAppender {
Copy link
Contributor

Choose a reason for hiding this comment

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

This "feels" wrong to me (that the config "owns" creating the appender). Let's chat about it on zoom, I can def be convinced the other way

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 have mixed feelings too, let's discuss in zoom. The idea was that plugin just registers new config schema with our logging subsystem and that's it, the new appenders that plugin provides are created based on it. But I'm sure there is a better way to do this :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, good point. I like that idea. Let's explore on zoom on Monday

for (const [loggerKey, loggerAdapter] of this.loggers.entries()) {
const loggerConfig = this.getLoggerConfigByContext(config, loggerKey);
loggerAdapter.logger = new BaseLogger(
// TODO: Do we need to store namespace as an array at all?
Copy link
Contributor

Choose a reason for hiding this comment

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

I initially thought there was value in it, but I'm not sure anymore. E.g. for my comment below on getLoggerConfigByContext it might even be easier to use strings instead. Personally I feel the array is "cleaner" (by some definition of "clean" 🙈), but if the apis feel better with strings instead (which it looks like right now), I'm +1 on moving to strings

Copy link
Member Author

Choose a reason for hiding this comment

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

Personally I feel the array is "cleaner" (by some definition of "clean" 🙈)

Generally I agree with you on that, let's see how much back and forth (split and join) we'll have in the final version of the PR, if not that much we can keep an array form.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we have to go back and forth something is "wrong", aka it should be a string all the time. I'd prefer there to only be a join at the "last possible moment", otherwise we should probably string it at all times.

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 agree, but there will be at least one more place where we'll have to additionally either split/sliceor join (depending on the type we decide to keep): it's when we'll be traversing up server.foo.bar namespace.

@azasypkin azasypkin force-pushed the issue-12469-new-logging-backup branch from 9adf3a6 to 8e1bf0d Compare July 15, 2017 09:26
// If we don't have configuration for the specified context and it's the "nested" one (eg. `foo.bar.baz`),
// let's move up to the parent context (eg. `foo.bar`) and check if it has config we can rely on. Otherwise
// we fallback to the `root` context that should always be defined (enforced by configuration schema).
return this.getLoggerConfigByContext(
Copy link
Member Author

Choose a reason for hiding this comment

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

Here is support for "hierarchical" logger config sketched on the ✈️ :)

import { BaseAppender } from '../base/BaseAppender';
import { FileAppenderConfig } from './FileAppenderConfig';

export class FileAppender extends BaseAppender {
Copy link
Member Author

Choose a reason for hiding this comment

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

Probably neither of file-based appenders will go into the first PR to make it simpler for review, but I'm keeping them right now just to have a broader picture.


append(record: LogRecord) {
super.append(record);
console.log(this.logRecordToFormattedString(record));
Copy link
Contributor

Choose a reason for hiding this comment

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

How would you handle colorizing here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we have a plan what and how we want to colorize? It seems ES doesn't do this, but let me think...

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking something along the lines of current Kibana:

screen shot 2017-07-17 at 11 51 32

It makes it way easier to see error and warn, for example, plus the context.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, would be nice, will look into it

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is how it can be done, every appender can specifically post/pre-format any log message part. ConsoleLogger applies color using this extension hook. Though not sure about different colors for different context like it's done currently, do you think we need to support it?

Copy link
Member Author

@azasypkin azasypkin Jul 17, 2017

Choose a reason for hiding this comment

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

Some logging frameworks introduce separate configurable entity dedicated for formatting (encoders, formatters etc). But it seems we don't need such level of complexity right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of logRecordToFormattedString(logRecord) we could do:

logRecordToFormattedString({
  timestamp: ...,
  level: ...,
  context: ...
  message: ...
})

And then you have full control in the appender. And we wouldn't have to add so many protected methods

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure :) The only reason I chose protected is to have default base behavior (eg. format date and changing the case for the log level), so that it's not smeared across all of the appenders. Do you want all appenders to do argument formatting independently?

Copy link
Contributor

Choose a reason for hiding this comment

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

I want control and that looks like it'll give us control in the appender. I'd rather extract a "formatDate" util helper than using lots of protected methods (I prefer abstractions where I keep control over those that only DRY up the code)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, np, let's not overgeneralize at this point then.


constructor(config: LoggingConfigType) {
this.fillAppendersConfig(config);
this.fillLoggersConfig(config);
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like this is doing to much in the constructor? Maybe have a static "createLoggingConfig" helper?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it should eventually (in this PR) go to the static createFromSchema or something.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh Config should implement ConfigWithSchema that defines the signature of constructor, so I can't pass appenders and loggers as constructor arguments from static createLoggingConfig :/

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, of course, didn't think about that. Right now the config service "owns" instantiating the LoggingConfig. Hm. Let's just keep the methods in there instead of moving them to a static helper for now. (Unless you have other ideas you want to explore, of course)

Copy link
Member Author

@azasypkin azasypkin Jul 18, 2017

Choose a reason for hiding this comment

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

Nope, don't have a good idea right now. Will keep it as is for now.

import { typeOfSchema } from "../../../types";
import { BaseAppender } from './BaseAppender';

export function getRawSchema() {
Copy link
Contributor

Choose a reason for hiding this comment

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

These actually don't need to be functions. The schema can just be specified in a const instead.

(It makes sense to have it as a function if we inject schema, but we don't here, and I'm not sure we'll ever expose this stuff to plugins either)

Then you can use schema.TypeOf to get the type (just search for TypeOf to find examples)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the tip, will check it out.

kind: time
limit: 30s
rolling-file-by-time:
kind: rolling-file
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need file and rolling-file? Could maybe have an optional policy on file instead?

Copy link
Member Author

@azasypkin azasypkin Jul 17, 2017

Choose a reason for hiding this comment

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

No, it's not that advanced right now and we don't really need to separate them. It makes sense to start from a single file-based appender and eventually split them if we need to.

]);

const createLoggerSchema = () => {
return schema.object({
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: to increase readability on all of these maybe:

import { object, mapOf, string } from '../lib/schema'

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, will change, I was mostly concerned that without schema namespace object and string look like reserved TS words.

@ppf2 ppf2 mentioned this pull request Jul 20, 2017
@azasypkin azasypkin force-pushed the issue-12469-new-logging-backup branch from 2410570 to cede288 Compare July 24, 2017 09:51
@azasypkin azasypkin changed the title [WIP] Introduce Appenders and Loggers. Introduce Appenders and Loggers. Jul 24, 2017

/**
* Possible log level string values.
* @internal
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've made @internal as much stuff as possible, it's easier to expose something later than remove it.

this.config = config;

// Re-log all buffered log records with newly configured appenders.
for (const logRecord of this.bufferAppender.flush()) {
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've moved bufferAppender to a separate property since it should be used during all config updates in the future to not miss anything (eg. file appender is being asynchronously disposed, but module would like to log something at the same time, so before closing file we replace file appender with buffer one and we should not miss anything).

/**
* Separator string that used within nested context name (eg. plugins::pid).
*/
const CONTEXT_SEPARATOR = '::';
Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I'm using now, still not sure what will be the best. As you remember it's difficult to use . as a separator because of the fact that config treats properties with . in the name as nested properties and tries to expand them. But from another hand is may be not that bad idea to separate . and :: logically.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, wonder if we could use . as separator if we reworked the logic here https://github.com/elastic/kibana/blob/new-platform/platform/config/__tests__/ensureDeepObject.test.ts#L51-L69 to something like:

test("don't expand nested properties", () => {
  const obj = {
    'foo.bar': {
      'quux.a': 1,
      b: 2
    }
  };

  expect(ensureDeepObject(obj)).toEqual({
    foo: {
      bar: {
        'quux.a': 1,
        b: 2
      }
    }
  });
});

Really not sure about the trade-offs here, just that . looks much better than ::.

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, we can extract ^ into a separate issue too

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, I'll add comment in the PR and file separate issue that I'll be handling next as it's the major pain point.

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 #13105


return object({
appenders: mapOf(string(), Appenders.createConfigSchema(schema), {
defaultValue: new Map([
Copy link
Member Author

@azasypkin azasypkin Jul 24, 2017

Choose a reason for hiding this comment

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

These defaults are just "something" to have empty config and working logging at the same time, we should probably discuss (here or in the follow-up) what defaults we really want.

Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: maybe extract a const defaultLoggingAppenders or something like that, just to improve readability of this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, why not, will do

['root', { appenders: ['console'], level: 'info' as LogLevelId }]
]),
validate(value) {
const rootLogger = value.get(ROOT_CONTEXT_NAME);
Copy link
Member Author

Choose a reason for hiding this comment

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

Having root in the common loggers config and its appender in common appenders config makes code much easier, but config becomes harder to tune: e.g. in case you want to add a new logger you'll have to define root logger explicitly, same for new appender - you'll have to define appender that root logger uses explicitly as well. I'd definitely improve this somehow, but maybe in the next PR. What do you think? Do you have any ideas?

/**
* Map of the appender unique arbitrary key and its corresponding config.
*/
readonly appenders: Map<string, AppenderConfigType> = new Map();
Copy link
Member Author

@azasypkin azasypkin Jul 24, 2017

Choose a reason for hiding this comment

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

I actually decided to get rid of dedicated classes for appender configs, validated config/schema seems to be good enough.

@@ -0,0 +1,117 @@
import { LogLevel } from '../LogLevel';
Copy link
Member Author

Choose a reason for hiding this comment

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

Tests may seem to verbose and redundant, but I thought maybe that's ok for such a core thing :) Let me know if you'd like to simplify them.

@kimjoar
Copy link
Contributor

kimjoar commented Jul 25, 2017

Nitpick suggestion:

diff --git a/platform/logging/LoggerFactory.ts b/platform/logging/LoggerFactory.ts
index 8e498695d..fbdda38c1 100644
--- a/platform/logging/LoggerFactory.ts
+++ b/platform/logging/LoggerFactory.ts
@@ -28,29 +28,30 @@ export class MutableLoggerFactory implements LoggerFactory {
       return this.loggers.get(context)!;
     }
 
-    let loggerLevel, loggerAppenders;
-    if (this.config) {
-      const { level, appenders } = this.getLoggerConfigByContext(
-        this.config,
-        context
-      )!;
-      loggerLevel = LogLevel.fromId(level);
-      loggerAppenders = appenders.map(
-        appenderKey => this.appenders.get(appenderKey)!
+    this.loggers.set(
+      context,
+      new LoggerAdapter(this.createBaseLogger(context, this.config))
     );
-    } else {
+
+    return this.loggers.get(context)!;
+  }
+
+  private createBaseLogger(context: string, config: LoggingConfig | undefined) {
+    if (config === undefined) {
       // If we don't have config yet, use `buffered` appender that will store all logged messages in the memory
       // until the config is ready.
-      loggerLevel = LogLevel.All;
-      loggerAppenders = [this.bufferAppender];
+      return new BaseLogger(context, LogLevel.All, [this.bufferAppender])
     }
 
-    this.loggers.set(
-      context,
-      new LoggerAdapter(new BaseLogger(context, loggerLevel, loggerAppenders))
+    const { level, appenders } = this.getLoggerConfigByContext(
+      config,
+      context
+    )!;
+    const loggerLevel = LogLevel.fromId(level);
+    const loggerAppenders = appenders.map(
+      appenderKey => this.appenders.get(appenderKey)!
     );
-
-    return this.loggers.get(context)!;
+    return new BaseLogger(context, loggerLevel, loggerAppenders);
   }
 
   /**
@@ -74,14 +75,7 @@ export class MutableLoggerFactory implements LoggerFactory {
     }
 
     for (const [loggerKey, loggerAdapter] of this.loggers.entries()) {
-      const loggerConfig = this.getLoggerConfigByContext(config, loggerKey);
-      loggerAdapter.logger = new BaseLogger(
-        loggerKey,
-        LogLevel.fromId(loggerConfig.level),
-        loggerConfig.appenders.map(
-          appenderKey => this.appenders.get(appenderKey)!
-        )
-      );
+      loggerAdapter.logger = this.createBaseLogger(loggerKey, config);
     }
 
     this.config = config;

const rootLogger = value.get(ROOT_CONTEXT_NAME);
if (!rootLogger) {
throw new Error(
`"${ROOT_CONTEXT_NAME}" logger should be configured!`
Copy link
Contributor

@kimjoar kimjoar Jul 25, 2017

Choose a reason for hiding this comment

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

Been playing around with this and I think we need to improve the UX. Let's say you're relying on the logging defaults and to debug something you just want to up the log level on your plugin. What feels right to me is being able to just:

logging:
  loggers:
    myPlugin:
      level: debug

Now that fails with "root" logger should be configured!.

Another use case could be wanting to log everything from a plugin to a file while debugging, e.g. something like:

logging:
  appenders:
    myPluginAppender:
      kind: 'file'
      path: './myPlugin.log'
      layout:
        kind: 'pattern'
  loggers:
    myPlugin:
      appenders:
        - myPluginAppender
      level: all

Here we could also just use the defaults for the rest.

Maybe instead of applying the defaults like we do now, we add the default logger and appender automatically, unless you override them. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is what I was complaining about here #12852 (comment).

It definitely has to be improved, but I'd rather experiment with it a dedicated follow-up. Does it work for you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha, too many comments on this PR now. I agree with you. Can you create an issue for it? Maybe include some examples like I did above

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 #13098

import { LogLevel } from './LogLevel';

/**
* Interface describing essential parts of every log message.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: I think we should be more concise with our comments by removing redundant introductory text like "Interface describing". As an example, this comment could simply be Essential parts of every log message, and I don't think we lose any information there since the language itself makes it clear that this is an interface.

It doesn't really matter on any one instance, but when applied to comments throughout a code base, this can help lead to shorter and more to-the-point comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, I agree, will review all comments before merging.

@azasypkin azasypkin force-pushed the issue-12469-new-logging-backup branch from e743466 to 9ae0528 Compare July 28, 2017 08:00
@azasypkin
Copy link
Member Author

As we discussed with @kjbekkelund I'm merging this PR now to unblock other PRs that are starting to pile up.

I will still really appreciate your feedback/review/comments/objections/whatever @epixa @tylersmalley. You can take a look at the latest README in the next PR (#12941). I'm especially interested in knowing whether our new logging system covers/doesn't break use cases you have in mind.

@azasypkin azasypkin merged commit 052982d into elastic:new-platform Jul 31, 2017
@azasypkin azasypkin deleted the issue-12469-new-logging-backup branch July 31, 2017 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants