Skip to content
This repository was archived by the owner on Oct 3, 2023. It is now read-only.

Adds monitored resource to [core]#173

Closed
isaikevych wants to merge 3 commits intocensus-instrumentation:masterfrom
isaikevych:autopopulate_k8s_container
Closed

Adds monitored resource to [core]#173
isaikevych wants to merge 3 commits intocensus-instrumentation:masterfrom
isaikevych:autopopulate_k8s_container

Conversation

@isaikevych
Copy link
Copy Markdown
Contributor

@isaikevych isaikevych commented Nov 1, 2018

Monitored resource for #130

Issue #38.

@isaikevych isaikevych force-pushed the autopopulate_k8s_container branch from b8c4ef9 to 9935cb5 Compare November 1, 2018 01:47
@isaikevych isaikevych force-pushed the autopopulate_k8s_container branch from 9935cb5 to f3e812c Compare November 1, 2018 01:55
Comment thread packages/opencensus-core/src/common/monitored-resource/gcp-metadata-config.ts Outdated
Comment thread packages/opencensus-core/src/common/monitored-resource/gcp-metadata-config.ts Outdated
@isaikevych isaikevych force-pushed the autopopulate_k8s_container branch from 36ef9cc to eeec932 Compare November 1, 2018 19:10

getLabels() {
const aws = new AwsIdentityDocumentUtils();
return aws.getMetadata();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please only extract the needed information from the AWS metadata (https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/instance-identity-documents.html). The only things needed are account id, instance id and region.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@isaikevych isaikevych force-pushed the autopopulate_k8s_container branch from eeec932 to ce3d154 Compare November 16, 2018 21:35
@isaikevych isaikevych force-pushed the autopopulate_k8s_container branch from 3989d04 to 50e3a25 Compare November 16, 2018 22:16
@@ -0,0 +1,74 @@
export enum MonitoredResources {
GCP_GCE_INSTANCE = 'gce_instance',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type and attribute names should match Specs. See also go constants

MonitoredResources.GCP_GCE_INSTANCE|MonitoredResources.AWS_EC2_INSTANCE;
export type MonitoredResourceMetadata = Record<string, string>|string;

export interface MonitoredResource {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider to change MonitoredResource => Resource. MonitoredResourceUtil = > ResourceUtil, MonitoredResourceType = > ResourceType, AwsMonitoredResource => AwsResource.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new requirements blocked by Resource API

@@ -0,0 +1,105 @@
/**
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change "monitored-resource" => "resource-util" and this shouldn't be part of core package.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new requirements blocked by Resource API

@isaikevych
Copy link
Copy Markdown
Contributor Author

@mayurkale22 Mayur will continue work on it as we decided on our planing
/cc @jparsana

@justindsmith
Copy link
Copy Markdown
Contributor

@mayurkale22 Mayur will continue work on it as we decided on our planing
/cc @jparsana

I'm confused. Is this PR ready for review / merge, or is @mayurkale22 going to continue with some work and so should hold off on review?

@mayurkale22
Copy link
Copy Markdown
Member

Obsolete, counterpart is already implemented in #312 (under review).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants