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

CloudWatch: Add AWS/S3 replication metrics (#74416) #74418

Merged

Conversation

jordanefillatre
Copy link
Contributor

  • Add S3 replications metrics to appropriate Namespace in NamespaceMetricsMap

What is this feature?

Allow to use AWS S3 replication metrics using Cloudwatch datasource

Why do we need this feature?

Query "OperationsPendingReplication", "BytesPendingReplication", "ReplicationLatency", "OperationsFailedReplication" metrics

Who is this feature for?

[Add information on what kind of user the feature is for.]

Which issue(s) does this PR fix?:

Fixes #74416

Special notes for your reviewer:

Please check that:

  • [ x] It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@jordanefillatre jordanefillatre requested a review from a team as a code owner September 5, 2023 20:17
@jordanefillatre jordanefillatre requested review from fridgepoet and kevinwcyu and removed request for a team September 5, 2023 20:17
@CLAassistant
Copy link

CLAassistant commented Sep 5, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@iwysiu iwysiu left a comment

Choose a reason for hiding this comment

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

Hi @jordanefillatre ! Thanks for the contribution. Would you mind signing the CLA and reordering the new metrics to be in alphabetical order with the old ones?

Adding a link to the documentation: https://docs.aws.amazon.com/AmazonS3/latest/userguide/metrics-dimensions.html
Have you tried it and confirmed that it works? I noticed that the documentation didn't explicitly list the namespace

  * Add S3 replications metrics to appropriate Namespace in NamespaceMetricsMap
@jordanefillatre jordanefillatre force-pushed the feat/add-cloudwatch-s3-rep-metrics branch from 98059f2 to bce4985 Compare September 6, 2023 07:47
@jordanefillatre
Copy link
Contributor Author

Hi @iwysiu ,
You're right, the documentation isn't very clear about namespace.
I have confirm it by querying api using aws cli:

            "MetricName": "OperationsFailedReplication",
            "Dimensions": [
                {
                    "Name": "SourceBucket",
                    "Value": "xxx"
                },
                {
                    "Name": "DestinationBucket",
                    "Value": "xxx"
                },
                {
                    "Name": "RuleId",
                    "Value": "crr-full-standard"
                }
            ]
        },

It also show that I have omitted to add required dimensions.

Last I've take more time to manually check manually the fix by running a dev environment. I can confirm that it works after last commit update:

Capture d’écran du 2023-09-06 09-56-13

Copy link
Contributor

@iwysiu iwysiu left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for fixing the Dimensions and checking the namespace. Just one little alphabetization fix.

pkg/tsdb/cloudwatch/constants/metrics.go Outdated Show resolved Hide resolved
Co-authored-by: Isabella Siu <sakurablossom@blueblueworld.com>
@jordanefillatre
Copy link
Contributor Author

Oh, so sorry

@iwysiu iwysiu added the no-backport Skip backport of PR label Sep 6, 2023
@iwysiu iwysiu changed the title Plugins: Add CloudWatch AWS/S3 replication metrics (#74416) CloudWatch: Add AWS/S3 replication metrics (#74416) Sep 6, 2023
@iwysiu iwysiu added this to the 10.2.x milestone Sep 6, 2023
Copy link
Contributor

@iwysiu iwysiu left a comment

Choose a reason for hiding this comment

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

No problem, I'll finish up the process stuff and then merge it. Thanks again for contributing!

@iwysiu iwysiu merged commit b8b5d2f into grafana:main Sep 6, 2023
18 of 19 checks passed
chauchausoup pushed a commit to chauchausoup/grafana that referenced this pull request Sep 15, 2023
…4418)

Co-authored-by: Isabella Siu <sakurablossom@blueblueworld.com>
rwwiv pushed a commit that referenced this pull request Oct 2, 2023
Co-authored-by: Isabella Siu <sakurablossom@blueblueworld.com>
@zerok zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plugins: Unable to get CloudWatch AWS/S3 replication metrics
4 participants