fix(dynamodb): Misconfigured metrics causing empty graphs

[iliapolo]

This PR corrects 3 misconfigured metrics we had on the Table construct.

UserErrors

Per the documentation The table.metricUserErrors() does not emit the TableName dimension. It is actually an account (and region) wide metric.

The fix was to just validate that no dimensions are passed to this operation.

SystemErrors

Per the documentation The table.metricSystemErrors() is always emitted with the Operation dimension, and our current implementation does not pass it.

The fix adds an additional operations property to the method, that allows passing an array of operations, the returned metric will be a SUM over those operations. If no operation is passed, we sum all available operations.

Since the current method returns a Metric, returning a math expression won't work since it is an IMetric that doesn't extend Metric. To avoid breaking changes, we introduce a new method, metricSystemErrorsForOperations:

const totalSystemErrors = table.metricSystemErrorsForOperations();

const getPutSystemErrors = table.metricSystemErrorsForOperations({ 
  operations: [dynamo.Operation.PUT_ITEM, dynamo.Operation.GET_ITEM]
}); 

SuccessfulRequestLatency

Per the documentation The table.metricSuccessfulRequestLatency() is always emitted with the Operation dimension, and our current implementation does not pass it.

The fix requires user to pass the Operation dimension.

So the API is:

const getLatency = table.metricSuccessfulRequestLatency({ 
  dimensions: {
    Operation: 'GetItem'
  },
});

Fixes #11261
Fixes #11269

---

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

[gitpod-io]

[njlynch]

Generally, I think we need to do a "deprecate + create new methods" pattern here, over breaking changes to a stable module. There are users for whom the API wasn't broken, who will hit compilation issues when updating to this solution.

That's likely going to lead to some awkward method naming, but I don't see how we get around that otherwise.

[iliapolo]

Just for fun, here is how a combined latency graph will look like:

[njlynch]

Looks good to me. A buncha nit-picky stuff. Needs to be marked as ready for review + merge from master so you can fix the build failures.

[rix0rrr]

Some things to think about for your consideration. Won't block the PR on it, but do be sure you're satisfied with your own choices before merging :).

[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).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
• Commit ID: b24f7d1
• 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).