Default config metrics exporter #233
Conversation
| return "aggregation_dimensions", config.Dimensions | ||
| } | ||
|
|
||
| func (config *AggregationDimensions) SetDefaultDimensions() { |
There was a problem hiding this comment.
It seems you only call SetDefault... in this file? Maybe just inline the func manually. Also we might have a constant string for InstanceId defined somewhere.
There was a problem hiding this comment.
It is only used in a single place, so inlining does make sense. I was following appendDimensions.go, so it'd probably make sense to change it in both places.
There's a constant string for InstanceId defined in ruleInstanceId.go, but it doesn't seem like it's used anywhere else except for within that file.
There was a problem hiding this comment.
I think hard code the string here is fine, you can add a constant within this file/package if you want to.
| @@ -29,6 +31,17 @@ func (config *Metrics) ToMap(ctx *runtime.Context) (string, map[string]interface | |||
| } | |||
| } | |||
|
|
|||
| if config.AggregationDimensions == nil && ctx.WantAggregateDimensions { | |||
There was a problem hiding this comment.
So this assume that the new added option should not coexist with other option like WantEC2TagDimensions? Do we ensure that in the wizard?
There was a problem hiding this comment.
This doesn't prevent WantEC2TagDimensions from coexisting. It just adds a separate prompt to add the AggregationDimensions. I believe that based on the documentation for setting up the config for metric explorer (https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch-Metrics-Explorer.html), both the WantEC2TagDimensions and WantAggregateDimensions options are required during setup, but I felt that keeping the two options separate made sense if they weren't configuring with that purpose in mind. The default would include both.
|
As @pingleig suggested, once you confirm that this works from end to end, we can merge the PR. |
|
@jefchien any update on this? |
|
@pingleig Sorry for the delay. I've validated the configuration and behavior. Feel free to merge it when ready. |
|
@jefchien btw: remember to add milestone for this PR |
|
@jefchien it seems the test are failing, though we do have flaky tests ... https://github.com/aws/amazon-cloudwatch-agent/actions/runs/984367255 |
Description of the issue
In order to support EC2 metrics published by the CW Agent on Metrics Explorer, the dimensions of the metrics should be set as "Instance Id". This is something we achieve for the existing customers using some guideline docs. But for new customers who will onboard for the first time, we would like to offer this support by default by arranging these settings in the CW Agent config file.
Description of changes
Added a yes/no prompt in
defaultConfig.goas part of the config wizard to add theaggregation_dimensionsfield to themetricsconfig. By default, theaggregation_dimensionswill have a single dimension of "Instance Id". Moved pricing disclaimer from prompt to welcome message.License
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Tests
Updated
defaultConfig_test.goto account for the additional prompt and added theaggregation_dimensionsfield to the expected.Tested the validity of the generated config and performed end-to-end test.