-
Notifications
You must be signed in to change notification settings - Fork 94
Make Stats singleton (globalStats), separate registerView from createView #291
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,17 +18,21 @@ import * as defaultLogger from '../common/console-logger'; | |
| import * as loggerTypes from '../common/types'; | ||
| import {StatsEventListener} from '../exporters/types'; | ||
| import {Metric} from '../metrics/export/types'; | ||
| import {Metrics} from '../metrics/metrics'; | ||
|
|
||
| import {AggregationType, Measure, Measurement, MeasureType, MeasureUnit, View} from './types'; | ||
| import {MetricProducerForStats} from './metric-producer'; | ||
| import {AggregationType, Measure, Measurement, MeasureType, MeasureUnit, Stats, View} from './types'; | ||
| import {BaseView} from './view'; | ||
|
|
||
| export class Stats { | ||
| export class BaseStats implements Stats { | ||
| /** A list of Stats exporters */ | ||
| private statsEventListeners: StatsEventListener[] = []; | ||
| /** A map of Measures (name) to their corresponding Views */ | ||
| private registeredViews: {[key: string]: View[]} = {}; | ||
| /** An object to log information to */ | ||
| private logger: loggerTypes.Logger; | ||
| /** Singleton instance */ | ||
| private static singletonInstance: BaseStats; | ||
|
|
||
| /** | ||
| * Creates stats | ||
|
|
@@ -37,21 +41,22 @@ export class Stats { | |
| constructor(logger = defaultLogger) { | ||
| this.logger = logger.logger(); | ||
|
|
||
| // TODO (mayurkale): Decide how to inject MetricProducerForStats. | ||
| // It should be something like below, but looks like not the right place. | ||
|
|
||
| // Create a new MetricProducerForStats and register it to | ||
| // MetricProducerManager when Stats is initialized. | ||
| // const metricProducer: MetricProducer = new MetricProducerForStats(this); | ||
| // Metrics.getMetricProducerManager().add(metricProducer); | ||
| const metricProducer = new MetricProducerForStats(this); | ||
| Metrics.getMetricProducerManager().add(metricProducer); | ||
| } | ||
|
|
||
| /** Gets the stats instance. */ | ||
| static get instance(): Stats { | ||
| return this.singletonInstance || (this.singletonInstance = new this()); | ||
| } | ||
|
|
||
| /** | ||
| * Registers a view to listen to new measurements in its measure. Prefer using | ||
| * the method createView() that creates an already registered view. | ||
| * Registers a view to listen to new measurements in its measure. | ||
| * @param view The view to be registered | ||
| */ | ||
| registerView(view: View) { | ||
| registerView(view: View): void { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: do we need to include
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This not really required, but we are already using the same notion in other classes. I was trying to make things consistent.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I think that's fine. |
||
| if (this.registeredViews[view.measure.name]) { | ||
| this.registeredViews[view.measure.name].push(view); | ||
| } else { | ||
|
|
@@ -67,7 +72,7 @@ export class Stats { | |
| } | ||
|
|
||
| /** | ||
| * Creates and registers a view. | ||
| * Creates a view. | ||
| * @param name The view name | ||
| * @param measure The view measure | ||
| * @param aggregation The view aggregation type | ||
|
|
@@ -82,15 +87,14 @@ export class Stats { | |
| bucketBoundaries?: number[]): View { | ||
| const view = new BaseView( | ||
| name, measure, aggregation, tagKeys, description, bucketBoundaries); | ||
| this.registerView(view); | ||
| return view; | ||
| } | ||
|
|
||
| /** | ||
| * Registers an exporter to send stats data to a service. | ||
| * @param exporter An stats exporter | ||
| */ | ||
| registerExporter(exporter: StatsEventListener) { | ||
| registerExporter(exporter: StatsEventListener): void { | ||
| this.statsEventListeners.push(exporter); | ||
|
|
||
| for (const measureName of Object.keys(this.registeredViews)) { | ||
|
|
@@ -153,7 +157,7 @@ export class Stats { | |
| * Updates all views with the new measurements. | ||
| * @param measurements A list of measurements to record | ||
| */ | ||
| record(...measurements: Measurement[]) { | ||
| record(...measurements: Measurement[]): void { | ||
| if (this.hasNegativeValue(measurements)) { | ||
| this.logger.warn(`Dropping measurments ${measurements}, value to record | ||
| must be non-negative.`); | ||
|
|
@@ -176,4 +180,12 @@ export class Stats { | |
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Remove all registered Views and exporters from the stats. | ||
| */ | ||
| clear(): void { | ||
| this.registeredViews = {}; | ||
| this.statsEventListeners = []; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,8 +14,76 @@ | |
| * limitations under the License. | ||
| */ | ||
|
|
||
| import {StatsEventListener} from '../exporters/types'; | ||
| import {Metric} from '../metrics/export/types'; | ||
|
|
||
| /** Main interface for stats. */ | ||
| export interface Stats { | ||
| /** | ||
| * Creates a view. | ||
| * @param name The view name | ||
| * @param measure The view measure | ||
| * @param aggregation The view aggregation type | ||
| * @param tagKeys The view columns (tag keys) | ||
| * @param description The view description | ||
| * @param bucketBoundaries The view bucket boundaries for a distribution | ||
| * aggregation type | ||
| */ | ||
| createView( | ||
| name: string, measure: Measure, aggregation: AggregationType, | ||
| tagKeys: string[], description: string, | ||
| bucketBoundaries?: number[]): View; | ||
|
|
||
| /** | ||
| * Registers a view to listen to new measurements in its measure. | ||
| * @param view The view to be registered | ||
| */ | ||
| registerView(view: View): void; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need |
||
|
|
||
| /** | ||
| * Creates a measure of type Double. | ||
| * @param name The measure name | ||
| * @param unit The measure unit | ||
| * @param description The measure description | ||
| */ | ||
| createMeasureDouble(name: string, unit: MeasureUnit, description?: string): | ||
| Measure; | ||
|
|
||
| /** | ||
| * Creates a measure of type Int64. Values must be integers up to | ||
| * Number.MAX_SAFE_INTERGER. | ||
| * @param name The measure name | ||
| * @param unit The measure unit | ||
| * @param description The measure description | ||
| */ | ||
| createMeasureInt64(name: string, unit: MeasureUnit, description?: string): | ||
| Measure; | ||
|
|
||
| /** | ||
| * Updates all views with the new measurements. | ||
| * @param measurements A list of measurements to record | ||
| */ | ||
| record(...measurements: Measurement[]): void; | ||
|
|
||
| /** | ||
| * Remove all registered Views and exporters from the stats. | ||
| */ | ||
| clear(): void; | ||
|
|
||
| /** | ||
| * Gets a collection of produced Metric`s to be exported. | ||
| * @returns {Metric[]} List of metrics | ||
| */ | ||
| getMetrics(): Metric[]; | ||
|
|
||
| /** | ||
| * Registers an exporter to send stats data to a service. | ||
| * @param exporter An stats exporter | ||
| */ | ||
| registerExporter(exporter: StatsEventListener): void; | ||
| } | ||
|
|
||
|
|
||
| /** Tags are maps of names -> values */ | ||
| export interface Tags { | ||
| [key: string]: string; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the
: Statstype specifier needed here?Those are useful for enforcing the type assignment to a structural type like
const x: Y = {...}, but sinceBaseStats.instancealready has a type I would think it isn't needed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is added to avoid compiler error ->
Exported variable 'globalStats' has or is using name 'Stats' from external module "/opencensus-core/src/stats/types" but cannot be named.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@draffensperger Are you ok with my comment or you have better approach to handle this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that makes sense, thanks for clarifying.