Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Processor: add_cloud_metadata] Use AWS client to get instance metadata and EKS cluster name #35182
[Processor: add_cloud_metadata] Use AWS client to get instance metadata and EKS cluster name #35182
Changes from 12 commits
8d26505
3da87ca
39a690b
481f65c
4886d96
76a4da4
d659712
737f3d3
9255c71
db67e1b
bb5e429
98b5cb5
fb0293e
5b5e54d
1769926
64439f3
bbd928c
b193f5c
add4956
1b865c9
b6c28bd
da6ba8b
0241f22
1b23f61
79070e6
1679fed
87a3dd7
f4ae2cf
979778b
19ab154
b9ed1e8
29c4e2e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
Maybe just import fmt
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.
done - 0241f22
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.
This is an error message, is there a reason the log level is debug? Should be
Errorf
.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.
add_cloud_metadata
tries to fetch the metadata from all the available cloud providers, so if running on GCP -fetchMetadata
for AWS will still run and in this case it might will be confusing to get errors on GCP regarding the AWS configuration. I've changed it toWarnf
as it was done before - 19ab154There 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.
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.
Same as above with fmt.errorf
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.
This can be Warnf level and not debug?
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.
@tetianakravchenko only minor if you think that we need to expose this to warnings or to errors , as it is quite important to print this by default, or repeat what you do in line
beats/libbeat/processors/add_cloud_metadata/provider_aws_ec2.go
Line 93 in 98b5cb5
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.
@gizas thank you for pointing it out! From my understanding it should be fine to use it this way, http fetcher uses this definition as well -
beats/libbeat/processors/add_cloud_metadata/http_fetcher.go
Line 94 in 98b5cb5
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.
My comment has to do more with the level error vs debug vs warn. I would say not to use debug as it would hide any messages unless you raise the log level
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.
this code is outdated - was replaced to
Warn
in 0241f22There 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.
fyi: this field is already defined in ecs orchestrator.cluster.id
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.
quick question here: since we are only returning
Tags[0]
, does that mean we only have one set of tag key/value returned from the DescribeTags API call? If so, should we check len(tagsResult.Tags) == 1 instead?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.
yes, there only 1 element is expected, it should be the same as to run:
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.
@kaiyan-sheng I've changed it in 79070e6
I've found a mistake I make in
types.TagDescription
struct, now it return exactly 1 element