Flexible Dimension #46
Conversation
Add "Unit" in metric map report.
Add "Unit" in metric map report.
Create a framework to add any dimension if needed
Add Hostname dimension
… dimension as a local variable to return awslab Issue awslabs#43
|
Docs need to be updated to make people aware of how to use this feature |
| if self.config.DIMENSION_CONFIG_PATH != None: | ||
| d = Dimensions(self.config, self.vl) | ||
| dimensions = d.get_dimensions() | ||
| else: |
There was a problem hiding this comment.
There is a ASG feature pushed, it added a fixed dimension, maybe it will have some conflict with this pull request. Could you merge it?
There was a problem hiding this comment.
Yes, I am going to merge it in my new merge request.
| "Host" : self._get_host_dimension(), | ||
| "PluginInstance" : self._get_plugin_instance_dimension() | ||
| } | ||
| if self.config.DIMENSION_CONFIG_PATH != None: |
There was a problem hiding this comment.
use "if self.config.DIMENSION_CONFIG_PATH:"
If the configuration file is empty. how to handle it?
There was a problem hiding this comment.
If the configuration file is empty, it will fall into the else case, which will use the original implementation to use the fixed dimension of "Host" and "PluginInstance".
| def register_plugin(self): | ||
| self.func = dimension_get_instance_id | ||
| self.args = { | ||
| 'name': "InstanceId", |
There was a problem hiding this comment.
looks like the name of this dimension was "Host", now it is changed to "InstanceId", do not know if it will bring confusion.
There was a problem hiding this comment.
This flexible dimension is configurable. We definitely can implement another Dimension_Host class with the same code. It is up to us and the user to expand the scenarios. Here, I am just giving two examples. I will write more document to explain how to use this feature.
| @@ -67,3 +69,6 @@ def _add_values(self, metric, metric_map, metric_prefix): | |||
| metric_map[metric_prefix + self._STAT_MIN] = metric.statistics.min | |||
| metric_map[metric_prefix + self._STAT_SUM] = metric.statistics.sum | |||
| metric_map[metric_prefix + self._STAT_SAMPLE] = metric.statistics.sample_count | |||
|
|
|||
| def _add_unit(self, metric, metric_map, metric_prefix): | |||
| metric_map[metric_prefix + self._UNIT_KEY] = metric.unit | |||
There was a problem hiding this comment.
Is it possible that collectd give different unit every time? for example: sometimes, it is seconds, some times, it is miscroseconds.
There was a problem hiding this comment.
Yes, it is possible according to collectd protocol. We implemented that in our collectd read plugin.
| self.func = dimension_get_hostname | ||
| self.args = { | ||
| 'name': "Hostname", | ||
| 'value': os.uname()[1] |
There was a problem hiding this comment.
Do not know if following issue will happen.
Return a 5-tuple containing information identifying the current operating system. The tuple contains 5 strings: (sysname, nodename, release, version, machine). Some systems truncate the nodename to 8 characters or to
the leading component; a better way to get the hostname is socket.gethostname() or even socket.gethostbyaddr(socket.gethostname()).
|
This pull request has conflict with master branch, could you merge the latest codes? |
|
I have added the explanation how to use this feature in README.md. Thanks. |
|
Please confirm this contribution is under the terms of the MIT license. |
|
Yes, I confirm that this contribution is under the MIT license. |
|
+1 for this feature, anything this PR is waiting on? |
|
Colleague in Open Source IP department is checking the license. |
|
Is this contribution being made on behalf of the individual or on behalf of the company? |
|
It is on behalf of the company, Palo Alto Networks. |
|
What are we waiting for to merge this PR? |
|
Sorry. It is a little complicated. The latest reply from our security team: Liang: I replied: I expect to receive repeated contributions from this person. Anyway, I need to wait for the reply from security team and then reserve some time to test it. |
|
Thanks for the update. Yes, I might contribute more on this project if they are useful. I can get our IP attorney to answer some further questions if needed. Thanks. |
|
@yimuniao any update on the issue here? We are also waiting for this feature. :) |
|
This contribution was approved by Security team. I will test it and if everything is ok, I will merge it soon. |
|
I met following error, some metric has "none" for dimension "PluginInstance" :
< HTTP/1.1 400 Bad Request <
|
| self.func = dimension_get_plugin_instance | ||
| self.args = { | ||
| 'name': "PluginInstance", | ||
| 'value': self.vl.plugin_instance |
There was a problem hiding this comment.
if self.vl.plugin_instance is none, you can use "NONE". Refer to https://github.com/awslabs/collectd-cloudwatch/blob/master/src/cloudwatch/modules/metricdata.py#L139
|
After change some codes like: Following error occurred, Could you remove the changes related to "unit". We can use another pull request to address "unit" issue.
< HTTP/1.1 400 Bad Request <
|
Add "Unit" in metric map report.
… dimension as a local variable to return awslab Issue awslabs#43
2, Added tests for metricdata build with flex dimensions
…loudwatch into flex_dimension
|
@yimuniao I have fixed all the issues. Please review it. Thanks. |
|
I guess that this issue is pretty old now, but I'd love to have this feature. |
This is a framework implementing a way to easily add any dimension. (Issue #43)
The dimension coming in as a plugin. You can add it in dimensionplugin directory. As long as you initialize the dimension in Dimensions class init function, you can enable the dimension in the runtime by adding its name in dimensions.conf.
I have include a fix for adding Unit (Issue #42) in this request since that original request comes from a branch we can't open access to the public.