feat(chatbot): log retention support and metrics utility methods

[humanzz]

Closes #10135

---

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

[humanzz]

cc @skinny85 @nija-at @luckily

[nija-at]

Looks super nice and clean to me. Good job!

Some minor comments around improving docs and testing.

[nija-at]

Would be better assertions if you could create a CF resource from this metric and assert these values using stack inspection APIs, like toHaveResourceLike().

How about creating an alarm from this metric and asserting these very values but on the AWS::CloudWatch::Alarm resource?

[humanzz]

@nija-at Actually, similar to my Other question on #10135 about the inability to expose a LogGroup, I don't think this will work with AWS::CloudWatch::Alarm as it doesn't have the concept of a region.

I think there's a general problem here when it comes to global AWS services like Chatbot that write CloudWatch resources to us-east-1 and the lack of classes/constructs that represent those cloudwatch resources in CDK.

[humanzz]

I refrained from exposing a logGroup() method or even a LogGroupArn because of that.
I think the metrics are potentially usable in dashboards but am not sure if we'd want to test using that instead.

Cross-Region Functionality

Cross-Region functionality is now built-in automatically. You do not need to take any extra steps to be able to display metrics from different Regions in a single account on the same graph or the same dashboard.

[humanzz]

What do you think?

[humanzz]

I tried with the dashboard and the test would be something like

  test('getting configuration metric', () => {
    const slackChannel = new chatbot.SlackChannelConfiguration(stack, 'MySlackChannel', {
      slackWorkspaceId: 'ABC123',
      slackChannelId: 'DEF456',
      slackChannelConfigurationName: 'ConfigurationName',
      logRetention: logs.RetentionDays.ONE_MONTH,
    });
    const metric = slackChannel.metric('MetricName');
    new cloudwatch.Dashboard(stack, 'Dashboard', { dashboardName: 'DashboardName' })
      .addWidgets(new cloudwatch.GraphWidget({ left: [metric] }));

    expect(metric).toEqual(new cloudwatch.Metric({
      namespace: 'AWS/Chatbot',
      region: 'us-east-1',
      dimensions: {
        ConfigurationName: 'ConfigurationName',
      },
      metricName: 'MetricName',
    }));
    expect(stack).toHaveResourceLike('AWS::CloudWatch::Dashboard', {
      DashboardBody: {
        'Fn::Join': [
          '',
          [
            '{"widgets":[{"type":"metric","width":6,"height":6,"x":0,"y":0,"properties":{"view":"timeSeries","region":"',
            {
              Ref: 'AWS::Region',
            },
            '","metrics":[["AWS/Chatbot","MetricName","ConfigurationName","ConfigurationName",{"region":"us-east-1"}]],"yAxis":{}}}]}',
          ],
        ],
      },
      DashboardName: 'DashboardName',
    });
  });

But my feeling is that this will be a fragile test heavily dependent on how GraphWidget is rendered... if any property changes position, the test will likely break.

[nija-at]

I wanted to go back to your first response.

@nija-at Actually, similar to my Other question on #10135 about the inability to expose a LogGroup, I don't think this will work with AWS::CloudWatch::Alarm as it doesn't have the concept of a region.

While there might be a problem when deployed, it should still be good enough for a unit test, no? We're only looking to synthesize and assert.
Is there any reason why CDK will reject when new cloudwatch.Alarm() is used?

[humanzz]

It would definitely work as a unit test for assertion purposes.
My only qualm with it is that unit tests can in a way show how code should be used and we know that this not how this method should be used.

If you think it's fine and that it won't be read for more than assertion purposes, I am happy to update the PR

[nija-at]

I think this is a decent compromise. We'll need to look into the issue of deployability you've raised separately.

[humanzz]

Cool... I'll have an updated PR soon

[humanzz]

Updated

Pull request has been modified.

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[humanzz]

@nija-at thnx!
Re cross-region metrics/log groups, will you - or would u like me to - create an issue to track that?

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
• Commit ID: d4160cc
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).