feat: add prometheus stats exporter to opencensus nodejs #96
Conversation
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
eduardoemery
force-pushed
the
exporter_prometheus
branch
from
8c4165d
to
992f470
Compare
|
CLAs look good, thanks! |
djonathascardoso
force-pushed
the
exporter_prometheus
branch
2 times, most recently
from
859cf6e
to
c15055c
Compare
| private registerMetric(view: View, tags?: Tags): Metric { | ||
| const metricName = this.getPrometheusMetricName(view); | ||
| /** Get metric if already registered */ | ||
| const metric = register.getSingleMetric(metricName); |
There was a problem hiding this comment.
Would it be possible to set up a registry belonging to this Exporter instance, instead of registering to a global registry?
There was a problem hiding this comment.
Yes, it's possible. I'll change.
| const metric = register.getSingleMetric(metricName); | ||
|
|
||
| // Create a new metric if there is no one | ||
| if (!metric) { |
There was a problem hiding this comment.
I would suggest swapping this and write if (metric) { return metric; }, and everything else afterwards.
There was a problem hiding this comment.
Ok, I'll change.
| // Creating the metric based on aggregation type | ||
| switch (view.aggregation) { | ||
| case AggregationType.COUNT: | ||
| return new Counter(metricObj); |
There was a problem hiding this comment.
Will whatever gets returned here be returned the next time register.getSingleMetric is called with the same metric name?
There was a problem hiding this comment.
Yes, because this new instance run the registerMetric in background. Anyway, I'm going to change this to fit with the proposed changes in your first comment.
| * Get a string unit type based on measure unit | ||
| * @param view View used to get the unit | ||
| */ | ||
| private getUnit(view: View): string { |
There was a problem hiding this comment.
In this case I believe the correct is getUnit because it returns only one unit
There was a problem hiding this comment.
In colloquial English we typically say "units", even for one unit. But I'm not sure if this is grammatically correct, so we can keep it this way.
| } | ||
|
|
||
| /** | ||
| * Not used in this context |
There was a problem hiding this comment.
Can you elaborate here that metrics can't happen because it depends on information that is only present in Measurement objects?
There was a problem hiding this comment.
Sure, I'll put this.
|
|
||
| /** Format and sends Stats to Prometheus */ | ||
| export class PrometheusStatsExporter implements StatsEventListener { | ||
| static readonly defaultOptions = { |
There was a problem hiding this comment.
I would suggest making this in ALL_CAPS
There was a problem hiding this comment.
Ok, looks better.
djonathascardoso
force-pushed
the
exporter_prometheus
branch
from
c15055c
to
92a4cb4
Compare
| } | ||
|
|
||
| /** | ||
| * Not used because to register metrics it's necessary some information only |
There was a problem hiding this comment.
Nit: to register metrics it's necessary some information -> registering metrics requires information that is
djonathascardoso
force-pushed
the
exporter_prometheus
branch
from
92a4cb4
to
cb6d3e6
Compare
This PR adds prometheus stats exporter to opencensus nodejs. Unit tests are included.