New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[AWS] Collect metrics from linked accounts #35540
[AWS] Collect metrics from linked accounts #35540
Conversation
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good kaiyan - thanks for the quick turnaround on this. couple of small questions. no blockers.
@@ -195,11 +199,16 @@ func (m *MetricSet) getCloudWatchBillingMetrics( | |||
} | |||
for valI, metricDataResultValue := range output.Values { | |||
labels := strings.Split(*output.Label, labelSeparator) | |||
event := mb.Event{} | |||
if labels[accountIdIdx] != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
under what circumstances is this empty when we pass includeLinkedAccounts
?
|
||
label := metricName + labelSeparator | ||
label := metric.AccountID + labelSeparator + "${PROP('AccountLabel')}" + labelSeparator + metricName + labelSeparator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit not related to this PR - it would be better if we could avoid the manual work of keeping this label in sync with the index constants used to parse it. one way would just be to wrap the label creation and parsing in a type; at least that way the indexing concerns don't bleed out into the code. let's not change it now, just a thought though.
@@ -338,7 +347,7 @@ func (m *MetricSet) addCostMetrics(metrics map[string]costexplorertypes.MetricVa | |||
return event | |||
} | |||
|
|||
func constructMetricQueries(listMetricsOutput []types.Metric, dataGranularity time.Duration) []types.MetricDataQuery { | |||
func constructMetricQueries(listMetricsOutput []aws.MetricWithID, dataGranularity time.Duration) []types.MetricDataQuery { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, not required for this PR
We should rename constructMetricQueries
and createMetricDataQuery
with something clearer (e.g createQueries vs createSingleQuery)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR Kaiyan! I put some comments but none of the are deal breakers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding codeowner approval for the go.mod changes. Did not look closely at the other parts of the PR.
Thank you all for the review! I made another commit for some of the comments, and I will address the rest in a separate PR. |
(cherry picked from commit 217e658)
(cherry picked from commit 217e658)
(cherry picked from commit 217e658) # Conflicts: # NOTICE.txt # go.mod # go.sum # metricbeat/docs/modules/aws.asciidoc # x-pack/metricbeat/module/aws/_meta/docs.asciidoc # x-pack/metricbeat/module/aws/billing/billing.go # x-pack/metricbeat/module/aws/cloudwatch/cloudwatch.go # x-pack/metricbeat/module/aws/cloudwatch/cloudwatch_test.go # x-pack/metricbeat/module/aws/utils.go # x-pack/metricbeat/module/aws/utils_test.go
(cherry picked from commit 217e658)
What does this PR do?
This PR adds support for collecting metrics from linked accounts in AWS.
IncludeLinkedAccounts
is a new parameter offered inListMetrics
CloudWatch API for users to include metrics from source accounts when this API is called in a monitoring account. With this change, we also update thecloud.account.id
to the account that the instance belongs to, either the monitoring account or one of the linked accounts.Also I added dynamic label collection in this PR as well to collect the actual
cloud.account.name
for each metric. Dynamic label is supported as a part of theget-metric-data
API call for CloudWatch. With adding${PROP('AccountLabel')}
as a part of the label for collecting cloudwatch metrics, we can get the label specified for the source account that owns the metric.Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Related issues
Use cases
Config 1: Collect metrics from a given namespace.
Expected behavior: We should collect metrics from all instances both from the monitoring account and linked accounts.
Config 2: Collect a specific metric from a given instance.
Expected behavior: We should find the instance from one of the linked accounts and collect the metric.
Config3: collect a specific metric from all instances both from the linked accounts and the monitoring account.
Expected behavior: We should see
CPUUtilization
metric for all instances from both the linked accounts and the monitoring account.
Sample Output