From 2634c764a7f224197ddcf97a82843f3585be4750 Mon Sep 17 00:00:00 2001
From: pgayvallet <pierre.gayvallet@elastic.co>
Date: Thu, 3 Dec 2020 19:37:13 +0100
Subject: [PATCH] some self review

---
 .../time_interval/time_interval_policy.test.ts | 17 +++++++++++++++++
 .../time_interval/time_interval_policy.ts      |  7 ++++---
 .../rolling_file/rolling_file_appender.ts      | 18 +++++++-----------
 3 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/src/core/server/logging/appenders/rolling_file/policies/time_interval/time_interval_policy.test.ts b/src/core/server/logging/appenders/rolling_file/policies/time_interval/time_interval_policy.test.ts
index 79561173087bc..3f06883da8884 100644
--- a/src/core/server/logging/appenders/rolling_file/policies/time_interval/time_interval_policy.test.ts
+++ b/src/core/server/logging/appenders/rolling_file/policies/time_interval/time_interval_policy.test.ts
@@ -32,6 +32,7 @@ const format = 'YYYY-MM-DD HH:mm:ss';
 describe('TimeIntervalTriggeringPolicy', () => {
   afterEach(() => {
     getNextRollingTimeMock.mockReset();
+    jest.restoreAllMocks();
   });
 
   const createLogRecord = (timestamp: Date): LogRecord => ({
@@ -72,6 +73,22 @@ describe('TimeIntervalTriggeringPolicy', () => {
     );
   });
 
+  it('calls `getNextRollingTime` with the current time if `context.currentFileTime` is not set', () => {
+    const currentTime = moment('2018-06-15 04:27:12', format).toDate().getTime();
+    jest.spyOn(Date, 'now').mockReturnValue(currentTime);
+    const context = createContext(0);
+    const config = createConfig('15m', true);
+
+    new TimeIntervalTriggeringPolicy(config, context);
+
+    expect(getNextRollingTimeMock).toHaveBeenCalledTimes(1);
+    expect(getNextRollingTimeMock).toHaveBeenCalledWith(
+      currentTime,
+      config.interval,
+      config.modulate
+    );
+  });
+
   describe('#isTriggeringEvent', () => {
     it('returns true if the event time is after the nextRolloverTime', () => {
       const eventDate = moment('2010-10-20 04:43:12', format).toDate();
diff --git a/src/core/server/logging/appenders/rolling_file/policies/time_interval/time_interval_policy.ts b/src/core/server/logging/appenders/rolling_file/policies/time_interval/time_interval_policy.ts
index e500c6f50ceed..352e124875845 100644
--- a/src/core/server/logging/appenders/rolling_file/policies/time_interval/time_interval_policy.ts
+++ b/src/core/server/logging/appenders/rolling_file/policies/time_interval/time_interval_policy.ts
@@ -32,11 +32,12 @@ export interface TimeIntervalTriggeringPolicyConfig {
    * How often a rollover should occur.
    *
    * @remarks
-   * Due to of modulate rolling works, it is required to have an integer value for the highest time unit
+   * Due to how modulate rolling works, it is required to have an integer value for the highest time unit
    * of the duration (you can't overflow to a higher unit).
-   * For example, `15m` and `4h` are valid values , but `90m` is not (as it is `1.5h`),.
+   * For example, `15m` and `4h` are valid values , but `90m` is not (as it is `1.5h`).
    */
   interval: Duration;
+
   /**
    * Indicates whether the interval should be adjusted to cause the next rollover to occur on the interval boundary.
    *
@@ -65,7 +66,7 @@ export const timeIntervalTriggeringPolicyConfigSchema = schema.object({
  */
 export class TimeIntervalTriggeringPolicy implements TriggeringPolicy {
   /**
-   * millisecond timestamp of when the next rollover should occur.
+   * milliseconds timestamp of when the next rollover should occur.
    */
   private nextRolloverTime: number;
 
diff --git a/src/core/server/logging/appenders/rolling_file/rolling_file_appender.ts b/src/core/server/logging/appenders/rolling_file/rolling_file_appender.ts
index 1dee21941264c..194be9612bd3e 100644
--- a/src/core/server/logging/appenders/rolling_file/rolling_file_appender.ts
+++ b/src/core/server/logging/appenders/rolling_file/rolling_file_appender.ts
@@ -44,7 +44,6 @@ export interface RollingFileAppenderConfig {
   layout: LayoutConfigType;
   /**
    * The absolute path of the file to write to.
-   * If the file, or any of its parent directories, do not exist, they will be created.
    */
   path: string;
   /**
@@ -81,9 +80,6 @@ export class RollingFileAppender implements DisposableAppender {
   private readonly strategy: RollingStrategy;
   private readonly buffer: BufferAppender;
 
-  /**
-   * Creates FileAppender instance with specified layout and file path.
-   */
   constructor(config: RollingFileAppenderConfig) {
     this.context = new RollingFileContext(config.path);
     this.context.refreshFileInfo();
@@ -95,8 +91,8 @@ export class RollingFileAppender implements DisposableAppender {
   }
 
   /**
-   * Formats specified `record` and writes them to the specified file.
-   * @param record `LogRecord` instance to be logged.
+   * Formats specified `record` and writes it to the specified file. If the record
+   * would trigger a rollover, then it will be performed before writing to the file.
    */
   public append(record: LogRecord) {
     // if we are currently rolling the files, push the log record
@@ -119,7 +115,8 @@ export class RollingFileAppender implements DisposableAppender {
   }
 
   /**
-   * Disposes `FileAppender`. Waits for the underlying file stream to be completely flushed and closed.
+   * Disposes the appender.
+   * If a rollout is currently in progress, it will ve awaited.
    */
   public async dispose() {
     if (this.disposed) {
@@ -141,14 +138,13 @@ export class RollingFileAppender implements DisposableAppender {
     try {
       await this.strategy.rollout();
       await this.fileManager.closeStream();
-      this.rollingPromise = undefined;
-      this.isRolling = false;
     } catch (e) {
       // eslint-disable-next-line no-console
       console.log('Error while rolling file: ', e);
-    } finally {
-      this.flushBuffer();
     }
+    this.rollingPromise = undefined;
+    this.isRolling = false;
+    this.flushBuffer();
   }
 
   private flushBuffer() {