Stats/Stackdriver: export data using metrics API #275
Conversation
Codecov Report
@@ Coverage Diff @@
## master #275 +/- ##
==========================================
- Coverage 94.81% 94.58% -0.24%
==========================================
Files 110 110
Lines 7771 7421 -350
Branches 713 679 -34
==========================================
- Hits 7368 7019 -349
+ Misses 403 402 -1
Continue to review full report at Codecov.
|
mayurkale22
force-pushed
the
rewrite_stackdriver_exporter
branch
from
8c51192
to
37c367a
Compare
mayurkale22
requested review from
draffensperger,
justindsmith and
OsvaldoRosado
| const resourceLabels = {project_id: this.projectId}; | ||
| const monitoredResource = {type: 'global', labels: resourceLabels}; | ||
|
|
||
| metricsList.map(metric => { |
There was a problem hiding this comment.
If the value is not being used here, could you do a for (const .. of ..) loop instead of a .map call?
| metricsList.map(metric => { | ||
| const timeSeriesList = | ||
| createTimeSeriesList(metric, monitoredResource, this.metricPrefix); | ||
| timeSeriesList.forEach(ts => { |
There was a problem hiding this comment.
Rather than using forEach and push, could you use map here? And would that allow you to make the initial assignment of timeSeries above be the result of the map?
| {[key: string]: {view: View; tags: {[key: string]: Tags;}}}; | ||
| private displayNamePrefix: string; | ||
| // tslint:disable:no-any | ||
| private onMetricUploadError: any; |
There was a problem hiding this comment.
It seems like a possible value of this is a function. Could you make the type something like (err: Error) => void|{}, which would allow it to either a function that takes an error, or just any arbitrary object? That way there is some amount of documentation about what role it plays.
mayurkale22
force-pushed
the
rewrite_stackdriver_exporter
branch
from
37c367a
to
459df54
Compare
| for (const metricProducer of metricProducerManager.getAllMetricProducer()) { | ||
| for (const metric of metricProducer.getMetrics()) { | ||
| const isRegistered = | ||
| await this.registerMetricDescriptor(metric.descriptor); |
There was a problem hiding this comment.
Would it be worth running these registerMetricDescriptor calls in parallel? Not sure how useful it would be but they don't seem dependent on each other in this loop.
It seems like if you ran them all in parallel with something like Promise.all then you could aggregate their results together into the metricsList list once they all complete.
There was a problem hiding this comment.
The idea is to register each metricDescriptor once (and ignore metricDescriptor that are already registered). If we run registerMetricDescriptor in parallel, there are chances will make more external calls than usual. WDYT?
There was a problem hiding this comment.
OK. I suppose registering metric descriptors may not happen that often, so it's probably OK to leave as is. Maybe just something we can think of if anybody has a performance issue with it that we could try to compress it to a Set and run in parallel, but I like leaving it alone for now.
There was a problem hiding this comment.
This is certainly a good point, I have added TODO comment for now to track this in future.
|
I will merge this PR later today, if no other comments. |
This PR is part of #257
The majority of work has been handled/covered in
StackdriverStatsExporterUtils#266, this PR is based on that work to export data to backend.