-
Notifications
You must be signed in to change notification settings - Fork 97
Add MetricProducerManager to keep track of all MetricProducers #253
Add MetricProducerManager to keep track of all MetricProducers #253
Conversation
Codecov Report
@@ Coverage Diff @@
## master #253 +/- ##
=========================================
+ Coverage 94.54% 94.6% +0.05%
=========================================
Files 101 105 +4
Lines 7317 7397 +80
Branches 690 691 +1
=========================================
+ Hits 6918 6998 +80
Misses 399 399
Continue to review full report at Codecov.
|
* @return {MetricProducerManager}. | ||
*/ | ||
getMetricProducerManager(): MetricProducerManager { | ||
return ExportComponent.METRIC_PRODUCER_MANAGER; |
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 ExportComponent
just to keep the MetricProducerManager as a singleton? I'm not sure this needs to be a class at all if that's the case (vs simply exporting a const).
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.
SGTM, Done. PTAL
c7e6c86
to
b0e2b76
Compare
* | ||
* @return {MetricProducerManager}. | ||
*/ | ||
static getMetricProducerManager(): MetricProducerManager { |
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.
Depending on what getMetricRegistry
does, you might be able to make these properties instead of getter methods (or even potentially replacing this class for a plain module with consts - just like the previous PR feedback).
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.
The only reason to use getter here is to make apis consistent across other language libraries. WDYT? I am happy to change as per your suggestion.
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.
Personally I'm not a fan of classes used only for namespacing, since Node modules already provide a namespace and classes that are the only export in a file end up causing double-namespacing for anyone using require
instead of ES6 import syntax. That being said, I can understand the consistency argument.
One thing to note, if this were a plain module end users get some extra flexibility with importing, which might be beneficial. Eg:
Importing like this which provides similar syntax to the class implementation
import * as Metrics from '....metrics';
Metrics.metricProducerManager.add(…)
Or importing only what's needed to simplify code
import {metricProducerManager} from '....metrics';
metricProducerManager.add(…)
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.
Just noticed your question might have been specifically just about .getMetricProducerManager()
vs .metricProducerManager
- rather than also the replacement of the class with consts 😄
I don't have a strong opinion either way, since both are incredibly similar to the end user. All things equal I'd lean to not having to do a function call vs having to do one.
* Keeps a set of MetricProducer that is used by exporters to determine the | ||
* metrics that need to be exported. | ||
*/ | ||
export class MetricProducerManager { |
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.
Should users be able to construct their own, or should only the singleton be used? If users don't need their own, we might not want to export 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.
Should users be able to construct their own, or should only the singleton be used?
should only the singleton be used.
If users don't need their own, we might not want to export this.
Agreed, removed export and added MetricProducerManager
interface type, which is implemented by BaseMetricProducerManager
.
852f84b
to
ccec80d
Compare
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.
LGTM with a few more optional nits
* This method should be used by any metrics exporter that automatically | ||
* exports data for MetricProducer registered with the MetricProducerManager. | ||
* | ||
* @return {MetricProducer[]} List of MetricProducers. |
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.
Should this be Set<MetricProducer>
? More generally, what's the value of specifying types in JSDoc comments if they are already there in TypeScript? Does that help documentation generators?
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.
Changed to Set<MetricProducer>
. Mostly I follow https://google.github.io/styleguide/jsguide.html#jsdoc for JSDoc comments.
import {MetricRegistry} from './metric-registry'; | ||
|
||
|
||
// Class for accessing the default MetricsComponent. |
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.
Should this be JSDoc?
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.
Done.
private static readonly METRIC_REGISTRY = Metrics.newMetricRegistry(); | ||
private static readonly METRIC_COMPONENT = new MetricsComponent(); | ||
|
||
/** |
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.
Optional nit: could this comment be simplified to /** @return {MetricProducerManager} The global MetricProducerManager. */
?
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.
Done.
*/ | ||
static getMetricProducerManager(): MetricProducerManager { | ||
return metricProducerManagerInstance; | ||
} | ||
|
||
/** |
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.
Similar comment here on optionally combining the English Returns ..
and JSDoc @return
and making it a single line.
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.
Done.
c5c9d2f
to
96c5bba
Compare
Thanks for the reviews @draffensperger and @justindsmith |
Basically, the
MetricProducerManager
keeps a set of MetricProducers (MetricProducerForRegistry
andMetricProducerForStats
) that is used by exporters to determine the metrics that need to be exported. Next step is to rewrite all the stats exporters (Stackdriver and prometheus) to use data from MetricProducerManager instead of viewdata.The
MetricProducerManager
,MetricsComponent
andExportComponent
classes are same as Java's implementation of metrics package. https://github.com/census-instrumentation/opencensus-java/tree/master/api/src/main/java/io/opencensus/metrics/export