[add_cloud_metadata] Remove logger for AWS/EC2 #36829

constanca-m · 2023-10-12T14:44:33Z

What

Remove the logger for the provider AWS/EC2 in add_cloud_metadata.

Details

add_cloud_metadata is enabled by default. When the provider AWS/EC2 is setup we see this message at all times, even when we are not using AWS/EC2:

{"log.level":"warn","@timestamp":"2023-07-06T18:24:06.085Z","message":"error fetching EC2 Identity Document: operation error ec2imds: GetInstanceIdentityDocument, canceled, context deadline exceeded.","component":{"binary":"filebeat","dataset":"elastic_agent.filebeat","id":"filestream-monitoring","type":"filestream"},"log":{"source":"filestream-monitoring"},"log.logger":"add_cloud_metadata","log.origin":{"file.line":91,"file.name":"add_cloud_metadata/provider_aws_ec2.go"},"service.name":"filebeat","ecs.version":"1.6.0","ecs.version":"1.6.0"}

This change was introduced by this PR: #28285.

It is happening because when creating the provider, we call this function:

beats/libbeat/processors/add_cloud_metadata/add_cloud_metadata.go

Line 73 in e322104

fetchers, err := setupFetchers(initProviders, c)

And then we try to create each provider of the list:

beats/libbeat/processors/add_cloud_metadata/providers.go

Line 115 in e322104

fetcher, err := ff.Create(name, c)

The problem comes when this function is called. We have a logger that will start outputting warnings:

beats/libbeat/processors/add_cloud_metadata/provider_aws_ec2.go

Line 78 in e322104

logger := logp.NewLogger("add_cloud_metadata")

If we check the other providers' files (provider_*.go), we can check that none of them are using loggers inside their Create.

The easiest solution for now is just to delete the loggers.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

The easiest way is to:

Clone this branch.
Build metricbeat.
Start it.
Check the error is still no longer there.

Related issues

Logs

It is no longer present in the logs.

mergify · 2023-10-12T14:45:09Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @constanca-m? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

elasticmachine · 2023-10-12T16:10:41Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-11-01T08:33:46.936+0000
Duration: 71 min 17 sec

Test stats 🧪

Test	Results
Failed	0
Passed	28622
Skipped	2015
Total	30637

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

libbeat/processors/add_cloud_metadata/provider_aws_ec2.go

kaiyan-sheng · 2023-10-19T23:27:46Z

libbeat/processors/add_cloud_metadata/provider_aws_ec2.go

-	if err != nil {
-		logger.Warnf("error fetching cluster name metadata: %s.", err)
-	}
+	clusterName, _ := fetchEC2ClusterNameTag(awsConfig, instanceIdentity.InstanceIdentityDocument.InstanceID)


We probably should not ignore the error here. When we do collect tags from an ec2 instance, this error should be surfaced. For example user might not have DescribeTags permission configured properly.

We should keep this warning here. Also for the original log, since we removed the logger.Warnf("error fetching EC2 Identity Document: %s.", err) line and it has a return there at line 88, this warning should not matter when EC2 is not the proper cloud metadata source.

You're right @kaiyan-sheng , I added the log message again to this one

kaiyan-sheng · 2023-10-19T23:42:00Z

Hey @constanca-m thanks for working on this! I think instead of removing the logger, maybe we should reconsider the logic in add_cloud_metadata and add functions to distinguish different cloud platforms. This way if we are not running on ec2, the code you modified will not even get touched.

libbeat/processors/add_cloud_metadata/provider_aws_ec2.go

constanca-m · 2023-10-27T12:30:59Z

I had a look at this again. The main difference between this provider and the others is the generic fetcher (thanks @tetianakravchenko for pointing it out).

To give context, the way add_cloud_metadata is working is quite simple: we make a request to each provider metadata URL. After some timeout, if there is no reply then the provider will stop. This function is the one to fetch metadata, and AWS uses this one: fetchRawProviderMetadata (the one with the logger).

The responses (timeouts and errors of each provider) are taken care of here:

beats/libbeat/processors/add_cloud_metadata/providers.go

Line 164 in 2970b8a

for i := 0; i < len(p.initData.fetchers); i++ {

If we have no providers (meaning that all of them timed out with the request), we end up doing nothing. Otherwise we take care of each result. Since the logs are going to result.error, I opted for the following logic: if the error beats the timeout, then we should print, otherwise we should not. For that I add:

beats/libbeat/processors/add_cloud_metadata/providers.go

Line 173 in 2970b8a

p.logger.Errorf("add_cloud_metadata: received error %v", result.err)

And this should solve the loggers problem.

What do you think @gizas @tetianakravchenko @kaiyan-sheng ?

constanca-m · 2023-11-01T08:25:28Z

/test

kaiyan-sheng · 2023-11-20T10:53:37Z

libbeat/processors/add_cloud_metadata/provider_aws_ec2.go

@@ -22,13 +22,14 @@ import (
 	"fmt"
 	"net/http"

+	"github.com/elastic/elastic-agent-libs/logp"


nit: I dont think we need to move this import around

We need to move it around because of the errors of one of the tests. They force it to be sorted through the tool goimport

kaiyan-sheng

Thanks for the write up, looks good to me (besides the lib import)!

* Remove logger. * Add error message. * Add log message (cherry picked from commit d66a000) # Conflicts: # libbeat/processors/add_cloud_metadata/provider_aws_ec2.go

* Remove logger. * Add error message. * Add log message (cherry picked from commit d66a000)

* Remove logger. * Add error message. * Add log message (cherry picked from commit d66a000) Co-authored-by: Constança Manteigas <[email protected]>

* Remove logger. * Add error message. * Add log message

Remove logger.

5ee94b1

constanca-m added the Team:Cloud-Monitoring Label for the Cloud Monitoring team label Oct 12, 2023

constanca-m self-assigned this Oct 12, 2023

constanca-m requested a review from a team as a code owner October 12, 2023 14:44

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Oct 12, 2023

constanca-m added the backport-7.17 Automated backport to the 7.17 branch with mergify label Oct 12, 2023

gizas reviewed Oct 19, 2023

View reviewed changes

libbeat/processors/add_cloud_metadata/provider_aws_ec2.go Show resolved Hide resolved

kaiyan-sheng reviewed Oct 19, 2023

View reviewed changes

tetianakravchenko reviewed Oct 24, 2023

View reviewed changes

libbeat/processors/add_cloud_metadata/provider_aws_ec2.go Show resolved Hide resolved

Add error message.

2970b8a

constanca-m and others added 2 commits November 1, 2023 09:14

Add log message

6b5aa7a

Merge branch 'main' into remove-warning-for-ec2-aws

93cbdf4

Add log message

9a00135

aspacca approved these changes Nov 20, 2023

View reviewed changes

kaiyan-sheng reviewed Nov 20, 2023

View reviewed changes

kaiyan-sheng approved these changes Nov 20, 2023

View reviewed changes

constanca-m merged commit d66a000 into elastic:main Nov 20, 2023
7 checks passed

constanca-m deleted the remove-warning-for-ec2-aws branch November 20, 2023 10:56

mergify bot mentioned this pull request Nov 20, 2023

[7.17](backport #36829) [add_cloud_metadata] Remove logger for AWS/EC2 #37150

Closed

constanca-m restored the remove-warning-for-ec2-aws branch November 20, 2023 11:10

constanca-m deleted the remove-warning-for-ec2-aws branch November 20, 2023 11:11

kaiyan-sheng added the backport-v8.11.0 Automated backport with mergify label Nov 20, 2023

mergify bot pushed a commit that referenced this pull request Nov 20, 2023

[add_cloud_metadata] Remove logger for AWS/EC2 (#36829)

9531935

* Remove logger. * Add error message. * Add log message (cherry picked from commit d66a000)

mergify bot mentioned this pull request Nov 20, 2023

[8.11](backport #36829) [add_cloud_metadata] Remove logger for AWS/EC2 #37154

Merged

Scholar-Li pushed a commit to Scholar-Li/beats that referenced this pull request Feb 5, 2024

[add_cloud_metadata] Remove logger for AWS/EC2 (elastic#36829)

e625f67

* Remove logger. * Add error message. * Add log message

aspacca mentioned this pull request Feb 27, 2024

[libbeat] add_cloud_metadata processor logs confusing warnings for AWS even when running on other CSPs #35147

Closed

andrewkroh mentioned this pull request Feb 28, 2024

[libbeat] add_cloud_metadata - startup blocked by AWS IMSDv2 token fetch #33058

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[add_cloud_metadata] Remove logger for AWS/EC2 #36829

[add_cloud_metadata] Remove logger for AWS/EC2 #36829

constanca-m commented Oct 12, 2023

mergify bot commented Oct 12, 2023

elasticmachine commented Oct 12, 2023 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

kaiyan-sheng Oct 19, 2023

kaiyan-sheng Oct 31, 2023

constanca-m Nov 1, 2023

kaiyan-sheng commented Oct 19, 2023

constanca-m commented Oct 27, 2023

constanca-m commented Nov 1, 2023

kaiyan-sheng Nov 20, 2023

constanca-m Nov 20, 2023

kaiyan-sheng left a comment

[add_cloud_metadata] Remove logger for AWS/EC2 #36829

[add_cloud_metadata] Remove logger for AWS/EC2 #36829

Conversation

constanca-m commented Oct 12, 2023

What

Details

Checklist

How to test this PR locally

Related issues

Logs

mergify bot commented Oct 12, 2023

elasticmachine commented Oct 12, 2023 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

kaiyan-sheng Oct 19, 2023

Choose a reason for hiding this comment

kaiyan-sheng Oct 31, 2023

Choose a reason for hiding this comment

constanca-m Nov 1, 2023

Choose a reason for hiding this comment

kaiyan-sheng commented Oct 19, 2023

constanca-m commented Oct 27, 2023

constanca-m commented Nov 1, 2023

kaiyan-sheng Nov 20, 2023

Choose a reason for hiding this comment

constanca-m Nov 20, 2023

Choose a reason for hiding this comment

kaiyan-sheng left a comment

Choose a reason for hiding this comment

elasticmachine commented Oct 12, 2023 •

edited by jenkins-beats-ci bot

Loading