Auto detect GCE, GKE and AWS EC2 resources#312
Auto detect GCE, GKE and AWS EC2 resources#312mayurkale22 merged 2 commits intocensus-instrumentation:masterfrom
Conversation
mayurkale22
force-pushed
the
gke_gcp_aws_resources
branch
from
76361f0 to
152be9f
Compare
Codecov Report
@@ Coverage Diff @@
## master #312 +/- ##
=========================================
+ Coverage 94.79% 94.9% +0.11%
=========================================
Files 114 118 +4
Lines 7721 8030 +309
Branches 703 717 +14
=========================================
+ Hits 7319 7621 +302
- Misses 402 409 +7
Continue to review full report at Codecov.
|
draffensperger
left a comment
draffensperger
left a comment
There was a problem hiding this comment.
LGTM, though left a number of nit picky comments.
| export async function detectResource(): Promise<Resource> { | ||
| const resources: Resource[] = []; | ||
| const resourceFromEnv = CoreResource.createFromEnvironmentVariables(); | ||
| resources.push(resourceFromEnv); |
There was a problem hiding this comment.
Could you just do const resources = [CoreResource.createFromEnvironmentVariables()]?
| } else if (resourceType === ResourceType.GCP_GCE_INSTANCE) { | ||
| resources.push(await getComputerEngineResource()); | ||
| } | ||
| if (resourceType === ResourceType.AWS_EC2_INSTANCE) { |
There was a problem hiding this comment.
For consistency, should this have an else before the if similar to the resourceType === ResourceType.GCP_GCE_INSTANCE above?
| } | ||
| if (resourceType === ResourceType.AWS_EC2_INSTANCE) { | ||
| resources.push(await getAwsEC2Resource()); | ||
| } |
There was a problem hiding this comment.
What would you think about adding an exhuaustiveness check here? See https://basarat.gitbooks.io/typescript/docs/types/discriminated-unions.html
Would it make sense for this to be a switch?
There was a problem hiding this comment.
Initial if...else block is to pick GKE or GCE, since one instance cannot be both a GCE instance and a GKE instance
|
|
||
| /** Determine the compute environment in which the code is running. */ | ||
| export async function getResourceType(): Promise<ResourceType> { | ||
| if (!resourceType) { |
There was a problem hiding this comment.
Optional nit: what would you think about making this first line to be if (resourceType) return resourceType;, which would allow if statements below to be less nested.
| } catch { | ||
| /* ignore */ | ||
| } | ||
| return ''; |
There was a problem hiding this comment.
What would you think about moving this return '' up into the catch block? Then you could potentially remove the /* ignore */ comment since the error handling just causes an empty string to return.
There was a problem hiding this comment.
Good call. I was thinking about it, but missed somehow. done.
| } catch { | ||
| /* ignore */ | ||
| } | ||
| return ''; |
There was a problem hiding this comment.
Similar comment here on moving the return '' up into the catch
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
There was a problem hiding this comment.
Optional (may just be my opinion): I tend to like to have index.ts just export symbols and then have more clearly named files that do the heavy lifting. E.g. calling this one detect-resource.ts or similar and same for its test.
mayurkale22
force-pushed
the
gke_gcp_aws_resources
branch
from
6abfdd8 to
7406d7b
Compare
5 participants
This PR is to automatically detect monitored resources based the environment that the application is running in, according to OpenCensus MonitoredResource specs. This util should be independent of specific exporters, since it's supposed to be shared between the Stackdriver Stats exporter and Stackdriver Trace exporter.
I will send another PR to integrate this detected resources (
detectResource) to the Stackdriver exporter.Fixes #38.