Add utils to automatically detect monitored resources.#292
Conversation
| # GAE common attributes | ||
| # See: https://cloud.google.com/appengine/docs/flexible/python/runtime# | ||
| # environment_variables | ||
| GAE_ATTRIBUTES = { |
There was a problem hiding this comment.
GAE attributes should work as before.
| _GCE_ATTRIBUTES = { | ||
| # ProjectID is the identifier of the GCP project associated with this | ||
| # resource, such as "my-project". | ||
| 'project_id': 'project/project-id', |
There was a problem hiding this comment.
project_id, instance_id and zone also apply to GKE attributes.
| """ | ||
| raise NotImplementedError | ||
|
|
||
| def _get_resource_label_format(self): |
There was a problem hiding this comment.
This should be specific to Stackdriver Trace.
|
|
||
| if is_gke_environment(): | ||
| return monitored_resource.GcpGkeMonitoredResource() | ||
| elif is_gae_environment(): |
There was a problem hiding this comment.
This should be gce instead of gae. Same for the functions below.
| return '/g.co/r/{res_type}/'.format(res_type=self.get_resource_type()) | ||
|
|
||
|
|
||
| class GcpGceMonitoredResource(MonitoredResource): |
There was a problem hiding this comment.
There should also be a GcpGaeMonitoredResource for backwards-compatibility in Python
| if r.status_code != 404: | ||
| _aws_metadata_cache[_aws_identity_document] = r.json() | ||
|
|
||
| except Exception as e: |
There was a problem hiding this comment.
Could you be specific in the exception type here?
| _aws_identity_document = 'aws_identity_document' | ||
|
|
||
|
|
||
| def initialize_aws_identity_document(): |
There was a problem hiding this comment.
Add a doc string explaining what this function is dong?
|
|
||
| def initialize_aws_identity_document(): | ||
| try: | ||
| r = requests.get(_AWS_INSTANCE_IDENTITY_DOCUMENT_URI, |
There was a problem hiding this comment.
Instead of adding the requests dependency, we could use Python builtin module urllib, and be sure to add some sort of retry logic in this function in case the request would fail accidentally.
The idea is that we should avoid adding new dependencies if we don't have to.
| pass | ||
|
|
||
|
|
||
| class AwsIdentityDocumentUtils(object): |
There was a problem hiding this comment.
Why having a class with every methods in it are staticmethod? We can just have independent functions.
| """ | ||
| labels_list = [] | ||
| if _aws_identity_document in _aws_metadata_cache: | ||
| from opencensus.trace.attributes import Attributes |
There was a problem hiding this comment.
Move the import to the top of the file is better.
| """MonitoredResource returns the resource type and resource labels. | ||
| """ | ||
|
|
||
| def get_resource_type(self): |
There was a problem hiding this comment.
We can make resource_type a property of the class by adding @property decorator to the method. And the method name could just be resource_type.
| """ | ||
| raise NotImplementedError | ||
|
|
||
| def get_resource_labels(self): |
| https://cloud.google.com/monitoring/api/resources#tag_gce_instance | ||
| """ | ||
|
|
||
| def get_resource_type(self): |
| """ | ||
|
|
||
| def get_resource_type(self): | ||
| return "gce_instance" |
There was a problem hiding this comment.
Avoid using magic strings in functions. We could define it at the top of the file like GCE_INSTANCE = "gce_instance".
|
|
||
| install_requires = [ | ||
| 'google-api-core >= 0.1.1, < 2.0.0', | ||
| 'google-api-core >= 0.1.1, < 2.0.0', 'requests' |
There was a problem hiding this comment.
After using urllib, we won't need this dependency.
|
Be sure to add more unit tests, we will need to have 100% test coverage in order to make the CI happy. To skip test coverage check for some specific lines, adding |
…-python into monitoredresource_utils
| response_content = None | ||
| except URLError: | ||
| response_content = None | ||
| except socket.timeout: |
There was a problem hiding this comment.
except (HTTPError, URLError, socket.timeout):
response_content = None
| """Initialize metadata service once and load gcp metadata into map | ||
| This method should only be called once. | ||
| """ | ||
| global inited |
There was a problem hiding this comment.
It's a little weird to declare global variable in a class as we aren't using them anywhere outside. Can we declare the variables inside the class?
class GcpMetadataConfig(object):
inited = False
is_running_on_gcp = False
| """A Google Container Engine (GKE) container instance. | ||
| KUBERNETES_SERVICE_HOST environment variable must be set. | ||
| """ | ||
| if _KUBERNETES_SERVICE_HOST in os.environ: |
There was a problem hiding this comment.
return _KUBERNETES_SERVICE_HOST in os.environ
4 participants
This is according to OpenCensus MonitoredResource specs
issues #204 #205
/cc @songy23