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

Add rolling-file appender to core logging #84735

Merged
merged 28 commits into from
Dec 10, 2020
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
1ae0289
You need to start somewhere
pgayvallet Dec 2, 2020
864c587
revert comment
pgayvallet Dec 2, 2020
004436e
rename default strategy to numeric
pgayvallet Dec 2, 2020
f59bca6
add some tests
pgayvallet Dec 2, 2020
b5f098f
fix some tests
pgayvallet Dec 2, 2020
d5d3fd1
update documentation
pgayvallet Dec 2, 2020
a56c61e
update generated doc
pgayvallet Dec 2, 2020
6b6fc49
change applyBaseConfig to be async
pgayvallet Dec 2, 2020
a04d0c7
fix integ tests
pgayvallet Dec 2, 2020
91f1420
add integration tests
pgayvallet Dec 2, 2020
8165f35
some renames
pgayvallet Dec 3, 2020
30ba0d4
more tests
pgayvallet Dec 3, 2020
1077f30
Merge remote-tracking branch 'upstream/master' into kbn-56291-rolling…
pgayvallet Dec 3, 2020
8f4581b
more tests
pgayvallet Dec 3, 2020
b226a63
nits on README
pgayvallet Dec 3, 2020
2634c76
some self review
pgayvallet Dec 3, 2020
a9e21d5
doc nits
pgayvallet Dec 3, 2020
3e740c6
self review
pgayvallet Dec 3, 2020
02adb81
Merge remote-tracking branch 'upstream/master' into kbn-56291-rolling…
pgayvallet Dec 7, 2020
6694e24
use `escapeRegExp` from lodash
pgayvallet Dec 7, 2020
c72c20f
Merge remote-tracking branch 'upstream/master' into kbn-56291-rolling…
pgayvallet Dec 8, 2020
3aa0bc7
address some review comments
pgayvallet Dec 8, 2020
2cf122f
a few more nits
pgayvallet Dec 9, 2020
a8d26ef
extract `isDevCliParent` check outside of LoggingSystem.upgrade
pgayvallet Dec 9, 2020
e76a089
log errors from context
pgayvallet Dec 9, 2020
bf0e390
Merge remote-tracking branch 'upstream/master' into kbn-56291-rolling…
pgayvallet Dec 9, 2020
fc190b9
Merge remote-tracking branch 'upstream/master' into kbn-56291-rolling…
pgayvallet Dec 10, 2020
bbb96f4
add defaults for policy/strategy
pgayvallet Dec 10, 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
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,5 @@
<b>Signature:</b>

```typescript
export declare type AppenderConfigType = ConsoleAppenderConfig | FileAppenderConfig | LegacyAppenderConfig;
export declare type AppenderConfigType = ConsoleAppenderConfig | FileAppenderConfig | LegacyAppenderConfig | RollingFileAppenderConfig;
```
2 changes: 1 addition & 1 deletion packages/kbn-logging/src/appenders.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,5 @@ export interface Appender {
* @internal
*/
export interface DisposableAppender extends Appender {
dispose: () => void;
dispose: () => void | Promise<void>;
}
136 changes: 136 additions & 0 deletions src/core/server/logging/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
- [Layouts](#layouts)
- [Pattern layout](#pattern-layout)
- [JSON layout](#json-layout)
- [Appenders](#appenders)
- [Rolling File Appender](#rolling-file-appender)
- [Triggering Policies](#triggering-policies)
- [Rolling strategies](#rolling-strategies)
- [Configuration](#configuration)
- [Usage](#usage)
- [Logging config migration](#logging-config-migration)
Expand Down Expand Up @@ -127,6 +131,138 @@ Outputs the process ID.
With `json` layout log messages will be formatted as JSON strings that include timestamp, log level, context, message
text and any other metadata that may be associated with the log message itself.

## Appenders

### Rolling File Appender

Similar to Log4j's `RollingFileAppender`, this appender will log into a file, and rotate it following a rolling
strategy when the configured policy triggers.

#### Triggering Policies

The triggering policy determines when a rolling should occur.

There are currently two policies supported: `size-limit` and `time-interval`.

##### SizeLimitTriggeringPolicy

This policy will rotate the file when it reaches a predetermined size.

```yaml
logging:
appenders:
rolling-file:
kind: rolling-file
path: /var/logs/kibana.log
policy:
kind: size-limit
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 align our naming with one Elaticsearch uses?
size-limit --> SizeBasedTriggeringPolicy

We have another issue to unify config keys #57551, but probably you can change the newly added keys to comply with ES schema from the very beginning (policy --> policies)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, IDK about doing it directly in this PR instead of globalizing all the required changes in #57551 tbh. We would be introducing inconsistencies in our own logging API, which is imho probably worse.

In example:

size-limit --> SizeBasedTriggeringPolicy

All our other 'types' are lowercase, dash separated. Having a mix of both seems wrong

policy --> policies

In log4j configuration , policies is a map (they accept multiple policies). we don't ATM (and TIL by looking at log4j's code, it would requires significant work, their code is a patchwork of hacks and side effects to support that...) I feel like using policies could be misleading as the user may try to use an array instead of an object in the configuration?

Also, note that if we want to align the naming, there are other things to change:

kind: size-limit, in log4j, it's type, not kind. However all our other logging configuration structures are using the kind naming, so this would introduce yet more divergence in our own logging config API.

Not opposed to do it right now if you think it's still better though, just tell me.

Copy link
Contributor

Choose a reason for hiding this comment

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

in log4j, it's type, not kind

yeah, #57551 should fix it.

All our other 'types' are lowercase, dash separated.

Just curious, what types do you mean? I'm fine if we postpone this change till #57551 also it means that we should do that before removing the legacy log rotation @joshdover any objections?

size: 50mb
strategy:
//...
layout:
kind: pattern
```

The options are:

- `size`

the maximum size the log file should reach before a rollover should be performed.

The default value is `100mb`

##### TimeIntervalTriggeringPolicy

This policy will rotate the file every given interval of time.

```yaml
logging:
appenders:
rolling-file:
kind: rolling-file
path: /var/logs/kibana.log
policy:
kind: time-interval
interval: 10s
modulate: true
strategy:
//...
layout:
kind: pattern
```

The options are:

- `interval`

How often a rollover should occur.

The default value is `24h`

- `modulate`

Whether the interval should be adjusted to cause the next rollover to occur on the interval boundary.

For example, when true, if the interval is `4h` and the current hour is 3 am then the first rollover will occur at 4 am
and then next ones will occur at 8 am, noon, 4pm, etc.

The default value is `true`.

#### Rolling strategies

The rolling strategy determines how the rollover should occur: both the naming of the rolled files,
and their retention policy.

There is currently one strategy supported: `numeric`.

##### NumericRollingStrategy

This strategy will suffix the file with a given pattern when rolling,
and will retains a fixed amount of rolled files.

```yaml
logging:
appenders:
rolling-file:
kind: rolling-file
path: /var/logs/kibana.log
policy:
// ...
strategy:
kind: numeric
pattern: '-%i'
max: 2
layout:
kind: pattern
```

For example, with this configuration:

- During the first rollover kibana.log is renamed to kibana-1.log. A new kibana.log file is created and starts
being written to.
- During the second rollover kibana-1.log is renamed to kibana-2.log and kibana.log is renamed to kibana-1.log.
A new kibana.log file is created and starts being written to.
- During the third and subsequent rollovers, kibana-2.log is deleted, kibana-1.log is renamed to kibana-2.log and
kibana.log is renamed to kibana-1.log. A new kibana.log file is created and starts being written to.

The options are:

- `pattern`

The suffix to append to the file path when rolling. Must include `%i`, as this is the value
that will be converted to the file index.

for example, with `path: /var/logs/kibana.log` and `pattern: '-%i'`, the created rolling files
will be `/var/logs/kibana-1.log`, `/var/logs/kibana-2.log`, and so on.

The default value is `-%i`

- `max`

The maximum number of files to keep. Once this number is reached, oldest files will be deleted.

The default value is `7`

## Configuration

As any configuration in the platform, logging configuration is validated against the predefined schema and if there are
Expand Down
11 changes: 11 additions & 0 deletions src/core/server/logging/appenders/appenders.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@

import { mockCreateLayout } from './appenders.test.mocks';

import { ByteSizeValue } from '@kbn/config-schema';
import { LegacyAppender } from '../../legacy/logging/appenders/legacy_appender';
import { Appenders } from './appenders';
import { ConsoleAppender } from './console/console_appender';
import { FileAppender } from './file/file_appender';
import { RollingFileAppender } from './rolling_file/rolling_file_appender';

beforeEach(() => {
mockCreateLayout.mockReset();
Expand Down Expand Up @@ -83,4 +85,13 @@ test('`create()` creates correct appender.', () => {
});

expect(legacyAppender).toBeInstanceOf(LegacyAppender);

const rollingFileAppender = Appenders.create({
kind: 'rolling-file',
path: 'path',
layout: { highlight: true, kind: 'pattern', pattern: '' },
strategy: { kind: 'numeric', max: 5, pattern: '%i' },
policy: { kind: 'size-limit', size: ByteSizeValue.parse('15b') },
});
expect(rollingFileAppender).toBeInstanceOf(RollingFileAppender);
});
15 changes: 12 additions & 3 deletions src/core/server/logging/appenders/appenders.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ import {
import { Layouts } from '../layouts/layouts';
import { ConsoleAppender, ConsoleAppenderConfig } from './console/console_appender';
import { FileAppender, FileAppenderConfig } from './file/file_appender';
import {
RollingFileAppender,
RollingFileAppenderConfig,
} from './rolling_file/rolling_file_appender';

/**
* Config schema for validting the shape of the `appenders` key in in {@link LoggerContextConfigType} or
Expand All @@ -39,10 +43,15 @@ export const appendersSchema = schema.oneOf([
ConsoleAppender.configSchema,
FileAppender.configSchema,
LegacyAppender.configSchema,
RollingFileAppender.configSchema,
]);

/** @public */
export type AppenderConfigType = ConsoleAppenderConfig | FileAppenderConfig | LegacyAppenderConfig;
export type AppenderConfigType =
| ConsoleAppenderConfig
| FileAppenderConfig
| LegacyAppenderConfig
| RollingFileAppenderConfig;

/** @internal */
export class Appenders {
Expand All @@ -57,10 +66,10 @@ export class Appenders {
switch (config.kind) {
case 'console':
return new ConsoleAppender(Layouts.create(config.layout));

case 'file':
return new FileAppender(Layouts.create(config.layout), config.path);

case 'rolling-file':
return new RollingFileAppender(config);
case 'legacy-appender':
return new LegacyAppender(config.legacyLoggingConfig);

Expand Down
72 changes: 72 additions & 0 deletions src/core/server/logging/appenders/rolling_file/mocks.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { PublicMethodsOf } from '@kbn/utility-types';
import type { Layout } from '@kbn/logging';
import type { RollingFileContext } from './rolling_file_context';
import type { RollingFileManager } from './rolling_file_manager';
import type { TriggeringPolicy } from './policies/policy';
import type { RollingStrategy } from './strategies/strategy';

const createContextMock = (filePath: string) => {
const mock: jest.Mocked<RollingFileContext> = {
currentFileSize: 0,
currentFileTime: 0,
filePath,
refreshFileInfo: jest.fn(),
};
return mock;
};

const createStrategyMock = () => {
const mock: jest.Mocked<RollingStrategy> = {
rollout: jest.fn(),
};
return mock;
};

const createPolicyMock = () => {
const mock: jest.Mocked<TriggeringPolicy> = {
isTriggeringEvent: jest.fn(),
};
return mock;
};

const createLayoutMock = () => {
const mock: jest.Mocked<Layout> = {
format: jest.fn(),
};
return mock;
};

const createFileManagerMock = () => {
const mock: jest.Mocked<PublicMethodsOf<RollingFileManager>> = {
write: jest.fn(),
closeStream: jest.fn(),
};
return mock;
};

export const rollingFileAppenderMocks = {
createContext: createContextMock,
createStrategy: createStrategyMock,
createPolicy: createPolicyMock,
createLayout: createLayoutMock,
createFileManager: createFileManagerMock,
};
63 changes: 63 additions & 0 deletions src/core/server/logging/appenders/rolling_file/policies/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { schema } from '@kbn/config-schema';
import { assertNever } from '@kbn/std';
import { TriggeringPolicy } from './policy';
import { RollingFileContext } from '../rolling_file_context';
import {
sizeLimitTriggeringPolicyConfigSchema,
SizeLimitTriggeringPolicyConfig,
SizeLimitTriggeringPolicy,
} from './size_limit';
import {
TimeIntervalTriggeringPolicyConfig,
TimeIntervalTriggeringPolicy,
timeIntervalTriggeringPolicyConfigSchema,
} from './time_interval';

export { TriggeringPolicy } from './policy';

/**
* Any of the existing policy's configuration
*
* See {@link SizeLimitTriggeringPolicyConfig} and {@link TimeIntervalTriggeringPolicyConfig}
*/
export type TriggeringPolicyConfig =
| SizeLimitTriggeringPolicyConfig
| TimeIntervalTriggeringPolicyConfig;

export const triggeringPolicyConfigSchema = schema.oneOf([
sizeLimitTriggeringPolicyConfigSchema,
timeIntervalTriggeringPolicyConfigSchema,
]);

export const createTriggeringPolicy = (
config: TriggeringPolicyConfig,
context: RollingFileContext
): TriggeringPolicy => {
switch (config.kind) {
case 'size-limit':
return new SizeLimitTriggeringPolicy(config, context);
case 'time-interval':
return new TimeIntervalTriggeringPolicy(config, context);
default:
return assertNever(config);
}
};
30 changes: 30 additions & 0 deletions src/core/server/logging/appenders/rolling_file/policies/policy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { LogRecord } from '@kbn/logging';

/**
* A policy used to determinate when a rollout should be performed.
*/
export interface TriggeringPolicy {
/**
* Determines whether a rollover should occur before logging given record.
**/
isTriggeringEvent(record: LogRecord): boolean;
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved
}
Loading