Skip to content
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

Awscloudwatchtags #41388

Merged
merged 61 commits into from
Oct 29, 2024
Merged

Awscloudwatchtags #41388

merged 61 commits into from
Oct 29, 2024

Conversation

gizas
Copy link
Contributor

@gizas gizas commented Oct 23, 2024

  • Enhancement

Proposed commit message

Reopening the PR in order to merge #40755
Due to building errors (#33913 (comment)) the initial PR had been reverted

Related issues

gizas and others added 30 commits September 6, 2024 12:51
fixing chagelog

deleting unneeded manifest

 Changes to be committed:
	deleted:    metricbeat/module/kubernetes/_meta/test/docs/01_playground/metricbeat2.yaml
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Oct 23, 2024
@mergify mergify bot assigned gizas Oct 23, 2024
Copy link
Contributor

mergify bot commented Oct 23, 2024

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b awscloudwatchtags upstream/awscloudwatchtags
git merge upstream/main
git push upstream awscloudwatchtags

Copy link
Contributor

mergify bot commented Oct 23, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @gizas? 🙏.
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-8./d is the label to automatically backport to the 8./d branch. /d is the digit

Copy link
Contributor

mergify bot commented Oct 23, 2024

backport-8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

@mergify mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Oct 23, 2024
@mergify mergify bot mentioned this pull request Oct 23, 2024
6 tasks
@gizas
Copy link
Contributor Author

gizas commented Oct 24, 2024

@elastic/beats-tech-leads can I have an approval please?

go.mod Outdated
@@ -177,8 +177,11 @@ require (
github.com/Azure/azure-storage-blob-go v0.15.0
github.com/Azure/go-autorest/autorest/adal v0.9.24
github.com/apache/arrow/go/v14 v14.0.2
github.com/aws/aws-sdk-go v1.54.19
Copy link
Member

@cmacknz cmacknz Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is deprecated and goes EOL in July 2025, when you'll be forced to remove it because it won't get security updates (or I guess you could wait until there is a CVE reported in it and then urgently remove it): https://github.com/aws/aws-sdk-go

This PR also makes agentbeat 5% larger. Do we really need both versions of the AWS SDK?

~/go/src/github.com/elastic/beats/x-pack/agentbeat main
❯ du -h agentbeat
216M    agentbeat

~/go/src/github.com/elastic/beats/x-pack/agentbeat awscloudwatchtags
❯ du -h agentbeat
227M    agentbeat

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole idea of this fix was that we needed to query both Rest APIs (that were included in old sdk) and WebSocket and HTTP APIs (that are included in the v2 sdk) in case user needed the metadata
(See code for check)

Let me doublecheck again and let you know. I am not still merging this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmacknz I was not accurate, the https://github.com/aws/aws-sdk-go was not needed. Now it is removed!

Long explanation:

I was wrongly using aws.StringValue (See old commit) from old sdk, in order to be safe with nill pointer exception

I found a replacer with aws.Tostring from new sdk (see new code)

Just to clarify for the following code, both functions use aws-sdk-go-v2 but:

  • GetAPIGatewayRestAPIOutput : Uses package apigateway ("github.com/aws/aws-sdk-go-v2/service/apigateway")
  • GetAPIGatewayAPIOutput: Uses package apigatewayv2 ("github.com/aws/aws-sdk-go-v2/service/apigatewayv2")

@gizas gizas merged commit a547473 into main Oct 29, 2024
139 of 142 checks passed
@gizas gizas deleted the awscloudwatchtags branch October 29, 2024 07:36
mergify bot pushed a commit that referenced this pull request Oct 29, 2024
* adding fix for aws tags of cloudwatch

* updating docs

* Update x-pack/metricbeat/module/aws/cloudwatch/cloudwatch_test.go

Co-authored-by: kaiyan-sheng <[email protected]>

* Update x-pack/metricbeat/module/aws/cloudwatch/cloudwatch.go

Co-authored-by: kaiyan-sheng <[email protected]>

* adding a validation for the LimitRestAPI

* setting to max limit to 500

* setting to max limit to 500

* removing uneeded sdk

---------

Co-authored-by: kaiyan-sheng <[email protected]>
(cherry picked from commit a547473)
gizas added a commit that referenced this pull request Oct 29, 2024
* adding fix for aws tags of cloudwatch

* updating docs

* Update x-pack/metricbeat/module/aws/cloudwatch/cloudwatch_test.go

Co-authored-by: kaiyan-sheng <[email protected]>

* Update x-pack/metricbeat/module/aws/cloudwatch/cloudwatch.go

Co-authored-by: kaiyan-sheng <[email protected]>

* adding a validation for the LimitRestAPI

* setting to max limit to 500

* setting to max limit to 500

* removing uneeded sdk

---------

Co-authored-by: kaiyan-sheng <[email protected]>
(cherry picked from commit a547473)

Co-authored-by: Andrew Gizas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-8.x Automated backport to the 8.x branch with mergify Team:obs-ds-hosted-services Label for the Observability Hosted Services team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants