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.

Sign up for GitHub

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

Jump to bottom

feat(ce): Add L2 constructs for cost categories, anomaly monitor and subscriptions #16564

Closed
wants to merge 1 commit into from
Closed

feat(ce): Add L2 constructs for cost categories, anomaly monitor and subscriptions #16564

wants to merge 1 commit into from

Conversation

kadrach
Copy link
Member

@kadrach kadrach commented Sep 21, 2021

An attempt of an implementation of L2 constructs for cost anomaly monitors, subscriptions and cost categories. Before I continue shaving this yak, looking for any and all feedback!

The long and short of it:

const costCategory = new ce.CostCategory(this, 'cost-category', {
  rules: JSON.stringify([{  // See below.
    Type: 'REGULAR',
    Value: 'ServiceIsAmazonEC2',
    // DefaultValue: 'ThisIsNotSupportedViaCloudFormation',
    Rule: {
      Dimensions: {
        Key: 'SERVICE_CODE',
        Values: ['AmazonEC2'],
        MatchOptions: ['EQUALS'],
      },
    },
  }]),
});

const customMonitorWithCostCategory = new ce.AnomalyMonitor(this, 'custom-anomaly-monitor-with-cost-category', {
  specification: JSON.stringify({
    CostCategories: {
      Key: costCategory.costCategoryName,
      Values: ['ServiceIsAmazonEC2'],
    },
  }),
});

new ce.AnomalySubscription(this, 'subscription', {
  frequency: ce.Frequency.IMMEDIATE,
  monitors: [customMonitorWithCostCategory],
  subscribers: [
    new ce.SnsTopic(new sns.Topic(this, 'Topic')),
  ],
  threshold: 5,
});

The cost explorer APIs implement a (potentially recursive) Expression that is currently not implemented in this set of constructs. I am unsure what the preferred approach is to model this in the CDK. Importing the type from the SDK does not play nicely with jsii, copying seems a little odd. Perhaps something similar to how step function definitions are implemented, i.e. like the following?

category.addRule({
  type: "REGULAR",
  value: "foo",
  expression: Expression.And([
    Expression.CostCategory(...),
    Expression.Dimension(...),
    Expression.Or([...]),
  ]),
});

See #8635 (there's a whole two of us 👍's!).

This is my first contribution, please let me know if there's a better place to do this (e.g. if this should be an RFC instead).

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Sep 21, 2021

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: d8ae4c9
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@peterwoodworth peterwoodworth added effort/large Large work item – several weeks of effort @aws-cdk/aws-certificatemanager Related to Amazon Certificate Manager @aws-cdk/aws-ce Related to AWS Cost Explorer and removed @aws-cdk/aws-certificatemanager Related to Amazon Certificate Manager labels Sep 21, 2021
@njlynch njlynch self-assigned this Sep 22, 2021
@njlynch njlynch added the p2 label Sep 22, 2021
@njlynch
Copy link
Contributor

njlynch commented Sep 22, 2021

Thanks for the contribution!

At first glance, my initial impression is that we definitely want stronger modeling here. Any time we are passing in an arbitrary struct via JSON.parse as the primary input to a construct, it's a flag. This is one of those cases where the CDK can provide a much clearer, more type-safe interface than what is natively supported by CloudFormation.

I am unsure what the preferred approach is to model this in the CDK. Importing the type from the SDK does not play nicely with jsii, copying seems a little odd. Perhaps something similar to how step function definitions are implemented, i.e. like the following?

Yes, this modeling looks much closer to what I'd expect. I don't have the cycles right at the moment to do a deeper dive into it, but this type of more structured approach looks much more in line with what I would expect.

As a first set of iterations, we could definitely move this to an RFC, or iterate here on the README until we reach a model that makes sense. The bulk of the RFC is writing up the README anyway, so it's six of one or a half-dozen of either. The advantage of the RFC is that you might get a few more eyes on it. Once we have a high-level agreement on the README/API, then you can proceed on the implementation.

@njlynch njlynch added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Sep 22, 2021
@kadrach
Copy link
Member Author

kadrach commented Sep 22, 2021 •
edited
Loading

Thank you @njlynch, happy to proceed in this direction. Let's move this to an RFC if you feel this is the better approach.

@kadrach kadrach marked this pull request as draft September 22, 2021 10:06
@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Sep 23, 2021
@njlynch
Copy link
Contributor

njlynch commented Oct 25, 2021

Let's move this to an RFC if you feel this is the better approach.

Yes, I think that makes the most sense.

Please create an RFC tracking issue in our RFC repo, and then create your pull request against that issue with the contents of the README to start. We can iterate from there.

Closing this out in favor of the RFC.

@njlynch njlynch closed this Oct 25, 2021
@seanWLawrence seanWLawrence mentioned this pull request Jan 27, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@njlynch njlynch

Labels
@aws-cdk/aws-ce Related to AWS Cost Explorer effort/large Large work item – several weeks of effort p2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kadrach @aws-cdk-automation @njlynch @peterwoodworth