feat(apigateway): add metrics for Stage and Method constructs by cecheta · Pull Request #20617 · aws/aws-cdk

cecheta · 2022-06-03T21:59:45Z

This PR adds metrics for API Gateway stages and methods. The metrics have been largely copied from the existing RestApi metrics methods.

Closes #16433

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

gitpod-io · 2022-06-03T21:59:52Z

comcalvi

Thanks for the contribution! Please add an integ test using the new metric factories and update the README for this module.

Pull request has been modified.

cecheta · 2022-06-18T19:56:56Z

@comcalvi README updated and integration test added

comcalvi · 2022-06-28T05:47:42Z

packages/@aws-cdk/aws-apigateway/lib/restapi.ts

+    class ImportWithApiName extends Import {
+      public readonly restApiName = restApiName;
+    }


Why not add restApiName to Import itself, so we don't need this extra class?

I was struggling with the fact that the attribute was optional, but I guess we can fall back to id

Removed the unnecessary class

comcalvi · 2022-06-28T05:50:01Z

packages/@aws-cdk/aws-apigateway/lib/restapi.ts

   * Import an existing RestApi that can be configured with additional Methods and Resources.
   */
  public static fromRestApiAttributes(scope: Construct, id: string, attrs: RestApiAttributes): IRestApi {
+    const restApiName = attrs.restApiName ?? '';


why do we need to assign it ''? Line 684 of this file states that the default is ID of the RestApi construct. This is confusing since we assign it to the empty string here instead.

comcalvi · 2022-06-28T05:51:29Z

packages/@aws-cdk/aws-apigateway/lib/restapi.ts

+      public readonly restApiName = restApiName;
+    }
+
+    if (attrs.restApiName !== undefined) return new ImportWithApiName(scope, id);


we shouldn't be referencing attrs.restApiName here, we already have restApiName above. This usage is confusing; I would delete the restApiName local variable and use attrs.restApiName instead.

comcalvi · 2022-06-28T05:52:04Z

packages/@aws-cdk/aws-apigateway/lib/stage.ts

+  /**
+   * Metric for the number of client-side errors captured in a given period.
+   *
+   * Default: sum over 5 minutes


throughout this PR, default comments should read analogously to:

Suggested change

* Default: sum over 5 minutes

* @default - sum over 5 minutes

Updated throughout

comcalvi · 2022-06-28T05:53:50Z

packages/@aws-cdk/aws-apigateway/test/integ.restapi-metrics.ts

+const app = new cdk.App();
+
+const stack = new cdk.Stack(app, 'restapi-metrics');
+
+const restApi = new apigw.RestApi(stack, 'Api');
+
+const stage = restApi.deploymentStage;
+
+const method = restApi.root.addMethod('GET');


Suggested change

const app = new cdk.App();

const stack = new cdk.Stack(app, 'restapi-metrics');

const restApi = new apigw.RestApi(stack, 'Api');

const stage = restApi.deploymentStage;

const method = restApi.root.addMethod('GET');

const app = new cdk.App();

const stack = new cdk.Stack(app, 'restapi-metrics');

const restApi = new apigw.RestApi(stack, 'Api');

const stage = restApi.deploymentStage;

const method = restApi.root.addMethod('GET');

Pull request has been modified.

comcalvi · 2022-07-14T18:10:43Z

packages/@aws-cdk/aws-apigateway/README.md

-The APIs with the `metric` prefix can be used to get reference to specific metrics for this API. For example,
-the method below refers to the client side errors metric for this API.
+These metrics can be referred to using the metric APIs available on the `RestApi`, `Stage` and `Method` constructs.
+Note that detailed metrics must be enabled for a stage to use the `Method` metrics.


Please include a link to documentation that explains how to enable detailed metrics.

comcalvi · 2022-07-14T18:19:05Z

packages/@aws-cdk/aws-apigateway/lib/method.ts

+   * Returns the given named metric for this API method
+   */
+  public metric(metricName: string, stage: IStage, props?: cloudwatch.MetricOptions): cloudwatch.Metric {


I find this confusing; the doc string says that it returns "the given named metric", as if the metric already exists, but then the method invokes a constructor.

can we rename this to or attachMetric and change the docstring to something like "adds the named metric for this API method. It has metric name metricName and dimensions:

{please list them here in some form}

to this method."

I've mainly copied the existing code from restapi.ts, all the metric methods invoke a constructor and return a metric after calling attachTo().

The docstring seems to match metrics in other modules such as SQS and Lambda

I would think that renaming the docstring to something like "Returns a metric for this API method with the given name" would be sufficient to make it clearer

comcalvi · 2022-07-14T18:20:10Z

packages/@aws-cdk/aws-apigateway/lib/method.ts

+   *
+   * @default - sum over 5 minutes
+   */
+  public metricCacheHitCount(stage: IStage, props?: cloudwatch.MetricOptions): cloudwatch.Metric {


I like this abstraction on top of the canned metrics, nice work 👍

comcalvi · 2022-07-14T18:22:46Z

packages/@aws-cdk/aws-apigateway/lib/stage.ts

+  /**
+   * Returns the given named metric for this stage
+   */
+  public metric(metricName: string, props?: cloudwatch.MetricOptions): cloudwatch.Metric {


similar comment as above.

Additionally, having metric() next to metricClientError() is confusing; can we add a more descriptive name to metric(), to better distinguish it from the other metric*() methods in both classes?

Again, the naming metric() here seems to match the naming in other modules such as SQS and Lambda, maybe it is better to leave them consistent?

Pull request has been modified.

mergify · 2022-08-03T02:27:15Z

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

aws-cdk-automation · 2022-08-03T03:04:04Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: adf5ea4
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2022-08-03T03:04:24Z

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

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Jun 3, 2022

aws-cdk-automation requested a review from a team June 3, 2022 21:59

comcalvi self-assigned this Jun 15, 2022

comcalvi previously requested changes Jun 15, 2022

View reviewed changes

comcalvi previously requested changes Jun 28, 2022

View reviewed changes

comcalvi previously requested changes Jul 14, 2022

View reviewed changes

Chinedum Echeta and others added 6 commits July 18, 2022 21:22

feat(apigateway): add metrics for Stage and Method constructs

53999b4

feat(apigateway): Add integration tests, update README

c27233c

feat(apigateway): remove ImportWithApiName

183597c

feat(apigateway): update spacing

7c35e84

feat(apigateway): update comments

5e22d8d

feat(apigateway): update README

39bfd9c

TheRealAmazonKendra approved these changes Aug 3, 2022

View reviewed changes

Merge branch 'main' into metrics

adf5ea4

mergify bot merged commit 3bf1361 into aws:main Aug 3, 2022

	* Default: sum over 5 minutes
	* @default - sum over 5 minutes

Conversation

cecheta commented Jun 3, 2022 • edited by comcalvi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Adding new Unconventional Dependencies:

New Features

Uh oh!

gitpod-io bot commented Jun 3, 2022

Uh oh!

comcalvi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cecheta commented Jun 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Aug 3, 2022

Uh oh!

aws-cdk-automation commented Aug 3, 2022

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Aug 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cecheta commented Jun 3, 2022 •

edited by comcalvi

Loading

comcalvi left a comment •

edited

Loading