Make Stats singleton (globalStats), separate registerView from createView#291
Conversation
draffensperger
left a comment
draffensperger
left a comment
There was a problem hiding this comment.
What's the general motivation for the breaking change?
| // Stats singleton instance | ||
| import {BaseStats} from './stats/stats'; | ||
| import {Stats} from './stats/types'; | ||
| const globalStats: Stats = BaseStats.instance; |
There was a problem hiding this comment.
Is the : Stats type specifier needed here?
Those are useful for enforcing the type assignment to a structural type like const x: Y = {...}, but since BaseStats.instance already has a type I would think it isn't needed.
There was a problem hiding this comment.
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.
@draffensperger Are you ok with my comment or you have better approach to handle this?
There was a problem hiding this comment.
Yes, that makes sense, thanks for clarifying.
| // MetricProducerManager when Stats is initialized. | ||
| // const metricProducer: MetricProducer = new MetricProducerForStats(this); | ||
| // Metrics.getMetricProducerManager().add(metricProducer); | ||
| const metricProducer: MetricProducer = new MetricProducerForStats(this); |
There was a problem hiding this comment.
Similar here on not needing the : MetricProducer type annotation since it's implied.
| * @param view The view to be registered | ||
| */ | ||
| registerView(view: View) { | ||
| registerView(view: View): void { |
There was a problem hiding this comment.
Nit: do we need to include : void here? (Same comment applies to the other methods below)
There was a problem hiding this comment.
This not really required, but we are already using the same notion in other classes. I was trying to make things consistent.
There was a problem hiding this comment.
OK, I think that's fine.
|
|
||
| /** | ||
| * Registers a view to listen to new measurements in its measure. Prefer using | ||
| * the method createView() that creates an already registered view. |
There was a problem hiding this comment.
Is this comment about creating an already registered view still true given the change above to remove the registerView call in the createView implementation above?
There was a problem hiding this comment.
Good catch, removed obsolete comment.
| * the method createView() that creates an already registered view. | ||
| * @param view The view to be registered | ||
| */ | ||
| registerView(view: View): void; |
There was a problem hiding this comment.
Do we need : void here? (Same comment applies to registerExporter below).
|
|
Thanks for the explanation. Is the plan to go directly to GA or will there be an intermediate Beta release? |
I am not 100% sure about this. All the work related to GA Release / Feature Parity work effort available here : https://github.com/census-instrumentation/opencensus-node/issues?q=is%3Aopen+is%3Aissue+label%3AGA-Effort |
mayurkale22
force-pushed
the
globalStats
branch
from
0ce384e to
5e8ac8c
Compare
3 participants
This PR is based on #276.
This PR contains two breaking changes:
new Stats()has been deprecated on Stats class. The global singletonglobalStatsobject should be used instead.registerView()hook has been removed fromcreateView()API. This is related to No need tostats.registerView(view)#287.Fixes #290 #276