Stats/Stackdriver: Use resource util to generate MonitoredResource#314
Conversation
| async function getInstanceId() { | ||
| try { | ||
| return await gcpMetadata.instance('id'); | ||
| const id = await gcpMetadata.instance('id'); |
There was a problem hiding this comment.
This is numerical ID, need to convert to string before assigning to label value.
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
=========================================
- Coverage 94.9% 94.9% -0.01%
=========================================
Files 118 118
Lines 8030 8101 +71
Branches 717 723 +6
=========================================
+ Hits 7621 7688 +67
- Misses 409 413 +4
Continue to review full report at Codecov.
|
songy23
left a comment
songy23
left a comment
There was a problem hiding this comment.
LGTM overall, defer to @draffensperger for Node readability approval.
| const labels: Labels = {project_id: projectId}; | ||
| let mappings: Labels = {}; | ||
| const autoDetectedResource = await resource.detectResource(); | ||
| switch (autoDetectedResource.type) { |
There was a problem hiding this comment.
Optional: if you make a helper function that implements the logic of this switch then you can make an assignment like const [type, mappings] = getTypeAndMappings(autoDetectedResource.type). This would avoid the let initializations.
| return `${pad}${str}`; | ||
| } | ||
|
|
||
| function getGcpResourceLabelsMappings() { |
There was a problem hiding this comment.
What's the motivation for having these be functions and not just assign the values to the constants above?
IMO, having the constant right next to its value would make it clearer and more compact, but up to you on this.
There was a problem hiding this comment.
taking a suggestion. 👍
| it('should return a k8s MonitoredResource', async () => { | ||
| process.env.OC_RESOURCE_TYPE = 'k8s.io/container'; | ||
| process.env.OC_RESOURCE_LABELS = | ||
| 'k8s.io/pod/name=pod-xyz-123,k8s.io/container/name=c1,k8s.io/namespace/name=default,cloud.google.com/gce/zone=zone1'; |
There was a problem hiding this comment.
Optional nit: for long strings like this, I often like to break them up with the + operator to keep the line under 80 chars.
|
@draffensperger thanks for the review, PTAL again once you get time. |
draffensperger
left a comment
draffensperger
left a comment
There was a problem hiding this comment.
Looks good, thanks!
5 participants
Fixes #130