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.

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

(@aws-cdk/aws-lambda): Version.metric() function uses wrong metric options when the version is associated with an alias #14248

Open
huyphan opened this issue Apr 19, 2021 · 5 comments
Labels
@aws-cdk/aws-lambda Related to AWS Lambda bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@huyphan
Copy link
Contributor

huyphan commented Apr 19, 2021

The Issue

When the Lambda function version is associated with an alias, its metric() function (or other metric helper functions such as metricErrors() or metricInvocations()) uses the wrong values of dimensions property for the metrics. Consider the snippet below where we have a Lambda function and an alias:

const lambdaFunction = new Function(this, "MyFunction", { 
  ...
  functionName: "MyFunctionName"
  ...
});
const alias = new Alias(this, "MyFunctionAlias", {
  aliasName: "live",
  version: lambdaFunction.currentVersion
});
return lambdaFunction.version.metricErrors();

That code will return a metric with a dimensions property like this:

{        
  FunctionName: "MyFunctionName",
  Resource: `arn:aws:lambda:us-west-2:123456789012:function:MyFunctionName:42`,
}

(42 is the function version)

In reality, the correct dimensions for function version's metrics should be like this:

{
  "FunctionName": "MyFunctionName",
  "Resource': "MyFunctionName:live"
  "ExecutedVersion": "42"
}

(live is the name of the function alias)

You can probably verify that by running using ListMetrics API of CloudWatch against AWS/Lambda namespace:

 aws cloudwatch list-metrics --namespace AWS/Lambda --metric-name Errors --region=us-west-2
{
    "Metrics": [
    <output redacted>
       {
            "Namespace": "AWS/Lambda",
            "Dimensions": [
                {
                    "Name": "FunctionName",
                    "Value": "MyFunctionName"
                },
                {
                    "Name": "ExecutedVersion",
                    "Value": "5"
                },
                {
                    "Name": "Resource",
                    "Value": "MyFunctionName:live"
                }
            ],
            "MetricName": "Errors"
        }
    ]
} 

References:

  public metric(metricName: string, props: cloudwatch.MetricOptions = {}): cloudwatch.Metric {
    // Metrics on Aliases need the "bare" function name, and the alias' ARN, this differs from the base behavior.
    return super.metric(metricName, {
      dimensions: {
        FunctionName: this.lambda.functionName,
        // construct the ARN from the underlying lambda so that alarms on an alias
        // don't cause a circular dependency with CodeDeploy
        // see: https://github.com/aws/aws-cdk/issues/2231
        Resource: `${this.lambda.functionArn}:${this.version}`,
      },
      ...props,
    });
  }

The proposal

Users should be able to:

  1. Pass an alias to Version.metric*() functions
  2. Pass a function version to Alias.metric*() functions

Either case, the functions would return metrics with the same dimensions described above.

Environment

  • CDK CLI Version: 1.98.0 (build 79f4512)
  • Module Version: 1.98
  • Node.js Version: v10.0.0
  • OS: all
  • Language (Version): all

Other information

@huyphan huyphan added guidance Question that needs advice or information. needs-triage This issue or PR still needs to be triaged. labels Apr 19, 2021
@github-actions github-actions bot added the @aws-cdk/aws-lambda Related to AWS Lambda label Apr 19, 2021
@peterwoodworth
Copy link
Contributor

Hey @huyphan, sorry for the long response time.

As you can see in the implementation, the comment says that Metrics on Aliases need the "bare" function name, and the alias' ARN, this differs from the base behavior. This is presently intended behavior, so I'll mark this as a feature request as you're requesting for additional functionality to the metric() functions.

@nija-at what are your thoughts on this?

@peterwoodworth peterwoodworth added feature-request A feature should be added or improved. feature/enhancement A new API to make things easier or more intuitive. A catch-all for general feature requests. and removed guidance Question that needs advice or information. labels May 18, 2021
@nija-at
Copy link
Contributor

nija-at commented May 19, 2021

I'm not super familiar with function metrics, so let me know if I say anything incorrect here.

It seems to me that we need to split this issue into two.

  1. There seems to be a bug in the value assigned to the Resource dimension. We're setting the value to be an ARN, while what it expects is the function, version or alias' name. This should be a bug.

  2. Currently, we don't support the ExecutedVersion dimension at all. This should be a feature request.

@huyphan - does that sound correct?

@peterwoodworth - could you apply these changes?

@nija-at nija-at assigned peterwoodworth and unassigned nija-at May 19, 2021
@huyphan
Copy link
Contributor Author

huyphan commented May 19, 2021

Thanks @nija-at and @peterwoodworth!

What @nija-at described is correct. Let me know if you want me to create a separate feature request for ExecutedVersion dimension.

@peterwoodworth peterwoodworth added bug This issue is a bug. and removed feature-request A feature should be added or improved. feature/enhancement A new API to make things easier or more intuitive. A catch-all for general feature requests. needs-triage This issue or PR still needs to be triaged. labels May 19, 2021
@peterwoodworth peterwoodworth added p2 effort/medium Medium work item – several days of effort effort/small Small work item – less than a day of effort and removed effort/medium Medium work item – several days of effort labels Jun 24, 2021
@peterwoodworth
Copy link
Contributor

@huyphan I can't promise I can get to actually fixing these any time in the next month. If you'd like to see the bug fixed, you could implement it yourself and I'd be happy to take a look at the PR. Also, go ahead with creating the feature request for executed version. It would be good to separate those from each other.

@peterwoodworth peterwoodworth removed their assignment Jun 24, 2021
@varju
Copy link
Contributor

varju commented Jun 22, 2022

Has anybody had time to look at this issue? In theory I think it might be a simple change, but I'm struggling to get CDK built locally so I can verify the change:

diff --git a/packages/@aws-cdk/aws-lambda/lib/lambda-version.ts b/packages/@aws-cdk/aws-lambda/lib/lambda-version.ts
index 1249c48746..a2fdf25c60 100644
--- a/packages/@aws-cdk/aws-lambda/lib/lambda-version.ts
+++ b/packages/@aws-cdk/aws-lambda/lib/lambda-version.ts
@@ -229,15 +229,25 @@ export class Version extends QualifiedFunctionBase implements IVersion {
   }
 
   public metric(metricName: string, props: cloudwatch.MetricOptions = {}): cloudwatch.Metric {
-    // Metrics on Aliases need the "bare" function name, and the alias' ARN, this differs from the base behavior.
-    return super.metric(metricName, {
-      dimensions: {
+    let dimensions;
+    if (this.lambda instanceof Alias) {
+      // Metrics on Aliases need the "bare" function name, and the alias' ARN, this differs from the base behavior.
+      dimensions = {
+        FunctionName: this.lambda.functionName,
+        Resource: `${this.lambda.functionArn}:${this.lambda.aliasName}`,
+        ExecutedVersion: this.version,
+      };
+    } else {
+      dimensions = {
         FunctionName: this.lambda.functionName,
         // construct the ARN from the underlying lambda so that alarms on an alias
         // don't cause a circular dependency with CodeDeploy
         // see: https://github.com/aws/aws-cdk/issues/2231
         Resource: `${this.lambda.functionArn}:${this.version}`,
-      },
+      };
+    }
+    return super.metric(metricName, {
+      dimensions: dimensions,
       ...props,
     });
   }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-lambda Related to AWS Lambda bug This issue is a bug. effort/small Small work item – less than a day of effort p2
Projects
None yet
Development

No branches or pull requests

4 participants