PR comments

Author: joshdover

src/core/server/metrics/collectors/cgroup.ts

@@ -29,49 +29,43 @@ interface OsCgroupMetricsCollectorOptions {
 }
 
 export class OsCgroupMetricsCollector implements MetricsCollector<OsCgroupMetrics> {
-  // Used to prevent unnecessary file reads on systems not using cgroups
+  /**  Used to prevent unnecessary file reads on systems not using cgroups. */
   private noCgroupPresent = false;
+  private cpuPath?: string;
+  private cpuAcctPath?: string;
 
   constructor(private readonly options: OsCgroupMetricsCollectorOptions) {}
 
   public async collect(): Promise<OsCgroupMetrics> {
-    if (this.noCgroupPresent) {
-      return {};
-    }
-
     try {
-      const cgroups = await readControlGroups();
-      const cpuPath = this.options.cpuPath || cgroups[GROUP_CPU];
-      const cpuAcctPath = this.options.cpuAcctPath || cgroups[GROUP_CPUACCT];
-
-      // prevents undefined cgroup paths
-      if (!cpuPath || !cpuAcctPath) {
-        this.noCgroupPresent = true;
+      await this.initializePaths();
+      if (this.noCgroupPresent || !this.cpuAcctPath || !this.cpuPath) {
         return {};
       }
 
       const [cpuAcctUsage, cpuFsPeriod, cpuFsQuota, cpuStat] = await Promise.all([
-        readCPUAcctUsage(cpuAcctPath),
-        readCPUFsPeriod(cpuPath),
-        readCPUFsQuota(cpuPath),
-        readCPUStat(cpuPath),
+        readCPUAcctUsage(this.cpuAcctPath),
+        readCPUFsPeriod(this.cpuPath),
+        readCPUFsQuota(this.cpuPath),
+        readCPUStat(this.cpuPath),
       ]);
 
       return {
         cpuacct: {
-          control_group: cpuAcctPath,
+          control_group: this.cpuAcctPath,
           usage_nanos: cpuAcctUsage,
         },
 
         cpu: {
-          control_group: cpuPath,
+          control_group: this.cpuPath,
           cfs_period_micros: cpuFsPeriod,
           cfs_quota_micros: cpuFsQuota,
           stat: cpuStat,
         },
       };
     } catch (err) {
       if (err.code === 'ENOENT') {
+        this.noCgroupPresent = true;
         return {};
       } else {
         throw err;
@@ -80,6 +74,29 @@ export class OsCgroupMetricsCollector implements MetricsCollector<OsCgroupMetric
   }
 
   public reset() {}
+
+  private async initializePaths() {
+    // Perform this setup lazily on the first collect call and then memoize the results.
+    // Makes the assumption this data doesn't change while the process is running.
+    if (this.cpuPath && this.cpuAcctPath) {
+      return;
+    }
+
+    // Only read the file if both options are undefined.
+    if (!this.options.cpuPath || !this.options.cpuAcctPath) {
+      const cgroups = await readControlGroups();
+      this.cpuPath = this.options.cpuPath || cgroups[GROUP_CPU];
+      this.cpuAcctPath = this.options.cpuAcctPath || cgroups[GROUP_CPUACCT];
+    } else {
+      this.cpuPath = this.options.cpuPath;
+      this.cpuAcctPath = this.options.cpuAcctPath;
+    }
+
+    // prevents undefined cgroup paths
+    if (!this.cpuPath || !this.cpuAcctPath) {
+      this.noCgroupPresent = true;
+    }
+  }
 }
 
 const CONTROL_GROUP_RE = new RegExp('\\d+:([^:]+):(/.*)');

src/core/server/metrics/collectors/collector.mock.ts

@@ -0,0 +1,33 @@
+/*
+ * 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 { MetricsCollector } from './types';
+
+const createCollector = (collectReturnValue: any = {}): jest.Mocked<MetricsCollector<any>> => {
+  const collector: jest.Mocked<MetricsCollector<any>> = {
+    collect: jest.fn().mockResolvedValue(collectReturnValue),
+    reset: jest.fn(),
+  };
+
+  return collector;
+};
+
+export const metricsCollectorMock = {
+  create: createCollector,
+};

src/core/server/metrics/collectors/os.test.mocks.ts

@@ -0,0 +1,25 @@
+/*
+ * 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 { metricsCollectorMock } from './collector.mock';
+
+export const cgroupCollectorMock = metricsCollectorMock.create();
+jest.doMock('./cgroup', () => ({
+  OsCgroupMetricsCollector: jest.fn(() => cgroupCollectorMock),
+}));

src/core/server/metrics/collectors/os.test.ts

@@ -20,13 +20,16 @@
 jest.mock('getos', () => (cb: Function) => cb(null, { dist: 'distrib', release: 'release' }));
 
 import os from 'os';
+import { cgroupCollectorMock } from './os.test.mocks';
 import { OsMetricsCollector } from './os';
 
 describe('OsMetricsCollector', () => {
   let collector: OsMetricsCollector;
 
   beforeEach(() => {
     collector = new OsMetricsCollector();
+    cgroupCollectorMock.collect.mockReset();
+    cgroupCollectorMock.reset.mockReset();
   });
 
   afterEach(() => {
@@ -96,4 +99,9 @@ describe('OsMetricsCollector', () => {
       '15m': fifteenMinLoad,
     });
   });
+
+  it('calls the cgroup sub-collector', async () => {
+    await collector.collect();
+    expect(cgroupCollectorMock.collect).toHaveBeenCalled();
+  });
 });

src/core/server/metrics/metrics_service.mock.ts

@@ -76,8 +76,8 @@ type MetricsServiceContract = PublicMethodsOf<MetricsService>;
 
 const createMock = () => {
   const mocked: jest.Mocked<MetricsServiceContract> = {
-    setup: jest.fn().mockReturnValue(createSetupContractMock()),
-    start: jest.fn().mockReturnValue(createStartContractMock()),
+    setup: jest.fn().mockReturnValue(createInternalSetupContractMock()),
+    start: jest.fn().mockReturnValue(createInternalStartContractMock()),
     stop: jest.fn(),
   };
   return mocked;