-
Notifications
You must be signed in to change notification settings - Fork 330
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
Migrate to AWS SDK v2 #307
Conversation
Signed-off-by: Anas <[email protected]>
Signed-off-by: Anas <[email protected]>
Signed-off-by: Anas <[email protected]>
Signed-off-by: Anas <[email protected]>
Signed-off-by: Anas <[email protected]>
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.
From a cursory review this looks okay, however I expect the devil is in the details. What sort of manual testing have you done on this?
src/main/java/io/prometheus/cloudwatch/CloudWatchCollector.java
Outdated
Show resolved
Hide resolved
src/main/java/io/prometheus/cloudwatch/CloudWatchCollector.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Anas <[email protected]>
Signed-off-by: Anas <[email protected]>
Signed-off-by: Anas <[email protected]>
@brian-brazil I have addressed your reviews. Can you please have a 2nd look ?
I run the latest released version and this PR locally, fetch the metrics (wget) and compare the output (I did it multiple times). The result is always like this :
I am also running a local Prometheus and it's scrapping both version of the exporter : Not perfect, but should give a rough idea about the returned results by both versions of the exporter. PS: I am importing SQS metrics only. If you have a better idea for manual testing I can give it a try (if it doesn't require too much time). |
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.
Try all the config options basically, the tag ones in particular as that's a separate code path. Something is likely to break with a change like this, but we should try to minimise the chances of it.
src/main/java/io/prometheus/cloudwatch/CloudWatchCollector.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Anas <[email protected]>
Signed-off-by: Anas <[email protected]>
@brian-brazil I have tried all the config options and all seems fine. It was time consuming but I think it was worth it since I found something that I missed before ecd822f Regarding the tagging feature, here is the results of the tests I did: Test 1: Without tag filteringUsing the config below: region: eu-central-1
metrics:
- aws_namespace: AWS/ApplicationELB
aws_metric_name: RequestCount
aws_dimensions: [LoadBalancer]
aws_statistics: [Sum] I got the following results
Test 2: With tag filteringUsing the config below: region: eu-central-1
metrics:
- aws_namespace: AWS/ApplicationELB
aws_metric_name: RequestCount
aws_dimensions: [LoadBalancer]
aws_statistics: [Sum]
aws_tag_select:
tag_selections:
kubernetes.io/ingress-name: ["REDACTED"]
resource_type_selection: "elasticloadbalancing:loadbalancer"
resource_id_dimension: LoadBalancer I got the following results
|
@brian-brazil is there anything else needed for this PR ? |
I haven't had a chance to test this out myself yet. |
Thanks! |
Signed-off-by: Anas [email protected]
Fixes #172
@brian-brazil can I have your review please ?