-
Notifications
You must be signed in to change notification settings - Fork 22
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: Support AWS usage marketplace #1770
Conversation
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.
Looks good, added a questions and comments.
How can I test this?
premium/usage.go
Outdated
@@ -200,6 +206,20 @@ func NewUsageClient(meta plugin.Meta, ops ...UsageClientOptions) (UsageClient, e | |||
TeamNameValue: u.teamName, | |||
}, nil | |||
} | |||
// If user wants to use the AWS Marketplace for billing, don't even try to communicate with CQ API | |||
if isAWSMarketplace() { | |||
cfg, err := awsConfig.LoadDefaultConfig(context.TODO()) |
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.
What's the reason for using context.TODO()
in favor of context.Background()
? Maybe we should add timeout as well for loading the config?
Also, can we put this logic in a setupAWSMarketplace
function as NewUsageClient
is getting quite long
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.
context.TODO()
is supposed to be used when the surrounding function doesn't have a context available to use, so that is why I used it here. I am not sure the value of a timeout would add here.
I have moved the logic to setupAWSMarketplace in 566bbc0
premium/usage.go
Outdated
u.awsMarketPlaceClient = marketplacemetering.NewFromConfig(cfg) | ||
u.teamName = "AWS_MARKETPLACE" | ||
// This needs to be larger than normal, because we can only send a single usage record per second (from each compute node) | ||
u.batchLimit = 1000000000 |
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.
I'm not sure this is necessary, but would need to verify. I think how it should work is that there is a minimum flush time that should come into effect (by default this is 10s)
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.
I think there is a race condition, if a batch is flushed and within the same second the sync ends then the timestamp will have the same value as timestamps are truncated to the second
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.
Ah, good catch... ok this should work for now but we should probably try and think of a better solution in the long term, otherwise we won't count any records if the sync is abruptly stopped
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.
Code looks great, how can I test this?
} | ||
|
||
func isAWSMarketplace() bool { | ||
return os.Getenv("CQ_AWS_MARKETPLACE_CONTAINER") == "true" |
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.
Where these envs are being set? If it's something user settable (and not a published image of some sort) strconv.ParseBool would've been better, but I'm guessing it's an image?
//go:generate mockgen -package=mocks -destination=../premium/mocks/marketplacemetering.go -source=usage.go AWSMarketplaceClientInterface | ||
type AWSMarketplaceClientInterface interface { | ||
MeterUsage(ctx context.Context, params *marketplacemetering.MeterUsageInput, optFns ...func(*marketplacemetering.Options)) (*marketplacemetering.MeterUsageOutput, error) | ||
} |
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.
This was added to ensure that we could mock the AWS API calls like we do the API Server calls
Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>
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.
🚀
🤖 I have created a release *beep* *boop* --- ## [4.58.0](v4.57.1...v4.58.0) (2024-08-02) ### Features * Support AWS usage marketplace ([#1770](#1770)) ([1eb6d1a](1eb6d1a)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This adds support for using CloudQuery plugins as part of the AWS Marketplace by changing the metering implementation to use the Marketplace instead, if specified via environment variables.