Address review feedback

eclipse-theia · Aug 28, 2023 · 1cd3a61 · 1cd3a61
1 parent 3ca32cf
commit 1cd3a61
Show file tree

Hide file tree

Showing 5 changed files with 40 additions and 62 deletions.
diff --git a/packages/core/src/common/performance/measurement.ts b/packages/core/src/common/performance/measurement.ts
@@ -103,10 +103,10 @@ export interface MeasurementOptions {
     thresholdMillis?: number;
 
     /**
-     * Flag to indicate whether the stopwatch should cache measurement results for later retrieval.
+     * Flag to indicate whether the stopwatch should store measurement results for later retrieval.
      * For example the cache can be used to retrieve measurements which were taken during startup before a listener had a chance to register.
      */
-    cacheResults?: boolean
+    storeResults?: boolean
 }
 
 /**

diff --git a/packages/core/src/common/performance/stopwatch.ts b/packages/core/src/common/performance/stopwatch.ts
@@ -51,19 +51,19 @@ export abstract class Stopwatch {
     @inject(ILogger)
     protected readonly logger: ILogger;
 
-    protected cachedResults: MeasurementResult[] = [];
+    protected _storedMeasurements: MeasurementResult[] = [];
 
-    protected onMeasurementResultEmitter = new Emitter<MeasurementResult>();
-    get onMeasurementResult(): Event<MeasurementResult> {
-        return this.onMeasurementResultEmitter.event;
+    protected onDidAddMeasurementResultEmitter = new Emitter<MeasurementResult>();
+    get onDidAddMeasurementResult(): Event<MeasurementResult> {
+        return this.onDidAddMeasurementResultEmitter.event;
     }
 
     constructor(protected readonly defaultLogOptions: LogOptions) {
         if (!defaultLogOptions.defaultLogLevel) {
             defaultLogOptions.defaultLogLevel = DEFAULT_LOG_LEVEL;
         }
-        if (defaultLogOptions.cacheResults === undefined) {
-            defaultLogOptions.cacheResults = true;
+        if (defaultLogOptions.storeResults === undefined) {
+            defaultLogOptions.storeResults = true;
         }
     }
 
@@ -102,41 +102,36 @@ export abstract class Stopwatch {
         return result;
     }
 
-    protected createMeasurement(name: string, measurement: () => { startTime: number, duration: number }, options?: MeasurementOptions): Measurement {
+    protected createMeasurement(name: string, measure: () => { startTime: number, duration: number }, options?: MeasurementOptions): Measurement {
         const logOptions = this.mergeLogOptions(options);
 
-        const result: Measurement = {
+        const measurement: Measurement = {
             name,
             stop: () => {
-                if (result.elapsed === undefined) {
-                    const { startTime, duration } = measurement();
-                    result.elapsed = duration;
-                    this.handleCompletedMeasurement(name, startTime, duration, logOptions);
-
+                if (measurement.elapsed === undefined) {
+                    const { startTime, duration } = measure();
+                    measurement.elapsed = duration;
+                    const result: MeasurementResult = {
+                        name,
+                        elapsed: duration,
+                        startTime,
+                        owner: logOptions.owner
+                    };
+                    if (logOptions.storeResults) {
+                        this._storedMeasurements.push(result);
+                    }
+                    this.onDidAddMeasurementResultEmitter.fire(result);
                 }
-                return result.elapsed;
+                return measurement.elapsed;
             },
-            log: (activity: string, ...optionalArgs: any[]) => this.log(result, activity, this.atLevel(logOptions, undefined, optionalArgs)),
-            debug: (activity: string, ...optionalArgs: any[]) => this.log(result, activity, this.atLevel(logOptions, LogLevel.DEBUG, optionalArgs)),
-            info: (activity: string, ...optionalArgs: any[]) => this.log(result, activity, this.atLevel(logOptions, LogLevel.INFO, optionalArgs)),
-            warn: (activity: string, ...optionalArgs: any[]) => this.log(result, activity, this.atLevel(logOptions, LogLevel.WARN, optionalArgs)),
-            error: (activity: string, ...optionalArgs: any[]) => this.log(result, activity, this.atLevel(logOptions, LogLevel.ERROR, optionalArgs)),
+            log: (activity: string, ...optionalArgs: any[]) => this.log(measurement, activity, this.atLevel(logOptions, undefined, optionalArgs)),
+            debug: (activity: string, ...optionalArgs: any[]) => this.log(measurement, activity, this.atLevel(logOptions, LogLevel.DEBUG, optionalArgs)),
+            info: (activity: string, ...optionalArgs: any[]) => this.log(measurement, activity, this.atLevel(logOptions, LogLevel.INFO, optionalArgs)),
+            warn: (activity: string, ...optionalArgs: any[]) => this.log(measurement, activity, this.atLevel(logOptions, LogLevel.WARN, optionalArgs)),
+            error: (activity: string, ...optionalArgs: any[]) => this.log(measurement, activity, this.atLevel(logOptions, LogLevel.ERROR, optionalArgs)),
         };
 
-        return result;
-    }
-
-    protected handleCompletedMeasurement(name: string, startTime: number, elapsed: number, options: LogOptions): void {
-        const result: MeasurementResult = {
-            name,
-            elapsed,
-            startTime,
-            owner: options.owner
-        };
-        if (options.cacheResults) {
-            this.cachedResults.push(result);
-        }
-        this.onMeasurementResultEmitter.fire(result);
+        return measurement;
     }
 
     protected mergeLogOptions(logOptions?: Partial<LogOptions>): LogOptions {
@@ -181,8 +176,8 @@ export abstract class Stopwatch {
         this.logger.log(level, `${whatWasMeasured}: ${elapsed.toFixed(1)} ms [${timeFromStart}]`, ...(options.arguments ?? []));
     }
 
-    getCachedResults(): MeasurementResult[] {
-        return [...this.cachedResults];
+    get storedMeasurements(): ReadonlyArray<MeasurementResult> {
+        return this._storedMeasurements;
     }
 
 }
diff --git a/packages/metrics/src/browser/index.ts b/packages/metrics/src/browser/index.ts
diff --git a/packages/metrics/src/browser/metrics-frontend-application-contribution.ts b/packages/metrics/src/browser/metrics-frontend-application-contribution.ts
@@ -41,8 +41,8 @@ export class MetricsFrontendApplicationContribution implements FrontendApplicati
         if (logLevel !== LogLevel.DEBUG) {
             return;
         }
-        this.stopwatch.getCachedResults().forEach(result => this.notify(result));
-        this.stopwatch.onMeasurementResult(result => this.notify(result));
+        this.stopwatch.storedMeasurements.forEach(result => this.notify(result));
+        this.stopwatch.onDidAddMeasurementResult(result => this.notify(result));
     }
 
     protected notify(result: MeasurementResult): void {

diff --git a/packages/metrics/src/node/measurement-metrics-contribution.ts b/packages/metrics/src/node/measurement-metrics-contribution.ts
@@ -31,16 +31,16 @@ export class MeasurementMetricsBackendContribution implements MetricsContributio
     protected logLevelCli: LogLevelCliContribution;
 
     protected metrics = '';
-    protected frontendCounter = new Map<string, string>();
+    protected frontendCounters = new Map<string, string>();
 
     startCollecting(): void {
         if (this.logLevelCli.defaultLogLevel !== LogLevel.DEBUG) {
             return;
         }
         this.metrics += `# HELP ${metricsName} Theia stopwatch measurement results.\n`;
         this.metrics += `# TYPE ${metricsName} gauge\n`;
-        this.backendStopwatch.getCachedResults().forEach(result => this.onBackendMeasurement(result));
-        this.backendStopwatch.onMeasurementResult(result => this.onBackendMeasurement(result));
+        this.backendStopwatch.storedMeasurements.forEach(result => this.onBackendMeasurement(result));
+        this.backendStopwatch.onDidAddMeasurementResult(result => this.onBackendMeasurement(result));
     }
 
     getMetrics(): string {
@@ -59,13 +59,13 @@ export class MeasurementMetricsBackendContribution implements MetricsContributio
     }
 
     protected createFrontendCounterId(frontendId: string): string {
-        const counterId = `frontend-${this.frontendCounter.size + 1}`;
-        this.frontendCounter.set(frontendId, counterId);
+        const counterId = `frontend-${this.frontendCounters.size + 1}`;
+        this.frontendCounters.set(frontendId, counterId);
         return counterId;
     }
 
     protected toCounterId(frontendId: string): string {
-        return this.frontendCounter.get(frontendId) ?? this.createFrontendCounterId(frontendId);
+        return this.frontendCounters.get(frontendId) ?? this.createFrontendCounterId(frontendId);
     }
 
     onFrontendMeasurement(frontendId: string, result: MeasurementResult): void {