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

feat: expose metrics for all AWS API calls #1862

Merged

Conversation

threadproc
Copy link
Contributor

Description of your changes

This change will add a new Prometheus metric, aws_api_calls_total, exposed by controller-runtime that counts all requests made to the AWS APIs. This is useful when using expensive APIs like the Service Catalog API ($0.0007/request) for accounting purposes on accounts with multiple clusters.

I have:

  • Read and followed Crossplane's [contribution process].
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

This has been built and deployed to a test cluster where it was successfully observed to cause no errors when calling either v1 or v2 SDK methods, and successfully included the new metric on the /metrics endpoint.

Copy link
Collaborator

@MisterMX MisterMX left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution, @threadproc! I think including AWS metric would a very meaningful enhancement.

I have made some small remarks that would need to be solved before merging. Can you take a look at them?

pkg/controller/aws.go Outdated Show resolved Hide resolved
pkg/controller/aws.go Outdated Show resolved Hide resolved
@threadproc threadproc force-pushed the feature/aws-api-call-metrics branch from d281921 to 9acbf87 Compare September 5, 2023 17:34
@threadproc
Copy link
Contributor Author

@MisterMX I've made the requested changes and resolved some linting errors as well, thanks for the review!

@threadproc threadproc requested a review from MisterMX September 5, 2023 17:36
@chlunde
Copy link
Collaborator

chlunde commented Sep 5, 2023

Great, very nice that you found a way to two this via hooks in the SDK!

cmd/provider/main.go Outdated Show resolved Hide resolved
pkg/utils/metrics/setup.go Outdated Show resolved Hide resolved
@threadproc threadproc force-pushed the feature/aws-api-call-metrics branch from 9acbf87 to 27686ee Compare September 6, 2023 10:02
@threadproc
Copy link
Contributor Author

@MisterMX I've implemented the requested changes and squashed them down again, thank you!

@threadproc threadproc requested a review from MisterMX September 6, 2023 10:03
Copy link
Collaborator

@MisterMX MisterMX left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you very much for your contribution @threadproc!

@MisterMX MisterMX merged commit 29651ec into crossplane-contrib:master Sep 6, 2023
8 checks passed
@threadproc threadproc deleted the feature/aws-api-call-metrics branch September 6, 2023 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants