-
Notifications
You must be signed in to change notification settings - Fork 237
Add support for EC2 metadata as dimensions for host metrics #1876
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
Conversation
|
||
// FilterReservedKeys out reserved tag keys | ||
// FilterReservedKeys out reserved tag keys and resolves AWS metadata variables at translation time | ||
func FilterReservedKeys(input any) any { |
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.
I don't think this is the right place for this. Have we considered putting it in translator/translate/util/placeholderUtil
and using the ec2tagger.SupportedAppendDimensions
that the ec2taggerprocessor
translator uses?
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.
Instead of doing this as part of the FilterReservedKeys
, can we just call ResolveAWSMetadataPlaceholders
after in commonconfigutil.go
?
} | ||
|
||
for { | ||
result, err := ec2Client.DescribeTags(input) |
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.
I tried to ask q if there's any other place in the code that uses DescribeTags that I could borrow this logic from...didn't find anything. Let me know if I can re-use something here
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.
Looks like we use DescribeTags for retrieving the EKS cluster name: https://github.com/aws/amazon-cloudwatch-agent/blob/main/translator/translate/logs/util/get_eks_cluster_name.go#L80. And we have some specialized retry logic with backoff logic, so it might be important to re-use that. That code is specific for retrieving the EKS cluster name though, so some refactoring would be needed to make these two uses mesh.
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.
Yeah, I could try to refactor but not sure if I should tackle it in this PR. Can be a follow up
|
||
func getEC2TagValue(tagKey string) string { | ||
ec2Util := ec2util.GetEC2UtilSingleton() | ||
if ec2Util.InstanceID == "" || ec2Util.Region == "" { |
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.
need instance id and region to call describe tags for the current instance
} | ||
|
||
for { | ||
result, err := ec2Client.DescribeTags(input) |
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.
Looks like we use DescribeTags for retrieving the EKS cluster name: https://github.com/aws/amazon-cloudwatch-agent/blob/main/translator/translate/logs/util/get_eks_cluster_name.go#L80. And we have some specialized retry logic with backoff logic, so it might be important to re-use that. That code is specific for retrieving the EKS cluster name though, so some refactoring would be needed to make these two uses mesh.
|
||
// FilterReservedKeys out reserved tag keys | ||
// FilterReservedKeys out reserved tag keys and resolves AWS metadata variables at translation time | ||
func FilterReservedKeys(input any) any { |
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.
Instead of doing this as part of the FilterReservedKeys
, can we just call ResolveAWSMetadataPlaceholders
after in commonconfigutil.go
?
…nsions. Move placeholder resolving logic to ProcessAppendDimensions
for { | ||
result, err := ec2Client.DescribeTags(input) | ||
if err != nil { | ||
log.Printf("E! Failed to describe EC2 tag '%s': %v", tagKey, err) |
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: Won't this start to show up in every translator log since we don't include DescribeTags
in the default policy? Are we sure we want to log this?
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.
We do this already when trying to get the EKS cluster name: https://github.com/aws/amazon-cloudwatch-agent/blob/main/translator/translate/logs/util/get_eks_cluster_name.go#L80
But this is only invoked if a customer sets their append_dimensions with an aws placeholder. So I think it makes sense to log it because they are trying to use a tag for their dimension, but it will fail.
// Cache AWS metadata on first use | ||
if awsMetadata == nil { | ||
awsMetadata = GetAWSMetadataPlaceholderInfo() | ||
} |
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: This will cache the metadata for one append_dimensions section, but I think it's going to re-generate it for each append_dimensions section in the agent config, incurring another AWS call to DescribeTags. Hard to know if that's an issue or not. The special retry logic in get_eks_cluster_name.go tells me we've had some issues in the past with the DescribeTags
call. GetAWSMetadataPlaceholderInfo
could do the caching instead so that it's shared across the entire invocation of the translator.
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.
yeah, for ec2 metadata at least we end up calling the ec2util singleton, so we are kind of double caching here: ec2 := ec2util.GetEC2UtilSingleton()
. For tags though we want to make sure we aren't making double the calls. Can have a separate singleton for that
Description of the issue
Currently if you tried to use
append_dimensions
within a host metrics plugin, it will append the raw string. If you tried to use the${aws:
formatting to substitute the dimension with EC2 metadata information or tags, it will not work. This is only supported in the globalappend_dimensions
section.The above will create the following dimensions today:
where
${aws:InstanceType}
is the literal string value. It does not get substitutedDescription of changes
The host plugins are telegraf-based and use TOML. We can query EC2 metadata service at translation time to populate the correct value into the TOML file.
From the above example:
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
Manual testing:
For the following cwagent config
Toml:
EC2 Tagger Yaml Config:
Requirements
Before commiting your code, please do the following steps.
make fmt
andmake fmt-sh
make lint
Integration Tests
To run integration tests against this PR, add the
ready for testing
label.