-
Notifications
You must be signed in to change notification settings - Fork 132
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
Count external API calls #1241
Count external API calls #1241
Conversation
a1f9f69
to
ddc9008
Compare
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.
Thank you very much @mergenci for working on this important observability topic. Left some comments for you to consider. Also would you like to briefly discuss the wrapped http.RoundTripper
approach in the PR description so that we record the challenges associated with that alternative?
Signed-off-by: Cem Mergenci <cmergenci@gmail.com>
ddc9008
to
28c4ff2
Compare
github.com/aws/aws-sdk-go v1.49.2 | ||
github.com/aws/aws-sdk-go-v2 v1.24.1 |
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.
These bumps are coming from the native provider's upbound fork dependency versions.
@@ -6,12 +6,14 @@ package clients | |||
|
|||
import ( | |||
"context" | |||
"reflect" | |||
"unsafe" |
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.
Cool :)
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.
😎🎉
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.
Thank you @mergenci, lgtm.
/test-examples="examples/iam/v1beta1/role.yaml" |
Description of your changes
This PR introduces three AWS API call counters:
API calls that couldn't be completed because of a connection error are not counted. API calls that return API errors (or no errors) are counted. There are no comprehensive AWS documentation on request rate limits, but here are two resources on the topic, for reference:
This PR also removes unsafe pointer operations, as described in upbound/terraform-provider-aws#196.
Alternatives considered
During the design phase, we investigated whether implementing an http.RoundTripper would be a good solution. Ideally, we would have a common implementation for AWS SDK v1 and v2, since both methods use an http.Client under the hood. RoundTripper implementation proved to be infeasible, because of the following reasons:
Checklist
I have:
make reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR if necessary.I couldn't run
make reviewable
, because my local terraform setup is broken.How has this code been tested
I've tested the code manually using the following resource configuration below, which contains resources that use AWS SDK v1 and v2, as of this writing. Because Upjet comes with Prometheus client, Upjet-based providers serve their metrics at
:8080/metrics
, by default. Here's a sample excerpt after applying the resource configuration:I manually cross-checked reported counts with the calls reported by CloudTrail Event History. Note that CloudTrail Event History may take up to a few minutes to show latest calls.
To test connection errors, I put breakpoints in the code, shut down my Internet connection upon hitting the breakpoint, and then resumed execution. To test API errors, I tried to delete a VPC that has a Security Group configured.
Resource Configuration