feat(metrics): allow setting functionName via constructor parameter and environment variable

[steven10172]

Summary

This PR ensures that a manually set functionName is not overwritten by middy and class decorators. If functionName is not already defined a default value of context.functionName will be used with middy and class decorators.

Changes

Please provide a summary of what's being changed

• Add hasFunctionName() to metrics class to support checking if the functionName is already defined
• Add functionName to metric constructor to support setting the functionName used for cold starts
• Add POWERTOOLS_METRICS_FUNCTION_NAME environment variable to support setting the functionName via IaC
• Do not call setFunctionName with default context.functionName in middy and decorator if functionName is already defined

This PR supports setting a custom functionName to be used with cold starts, that is more stable as context.functionName will change often due to the lambda being replaced when the infrastructure is updated.

Please add the issue number below, if no issue is present the PR might get blocked and not be reviewed

Issue number: #3642

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

[boring-cyborg]

Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need.
In the meantime, check out the #typescript channel on our Powertools for AWS Lambda Discord: Invite link

[steven10172]

@dreamorosi it looks like one of the tests failed with a network error. Would you mind rerunning it?

[leandrodamascena]

I'm starting the review now.

[dreamorosi]

Please ignore my previous review comment.

After looking at the implementation I think we should go back to the drawing board and avoid overcomplicating things.

The function name here refers to the Lambda function resource, not the decorated/wrapped function, so there's no reason for adding a property in every place like we are doing.

I know the discussion in the linked issue went into that direction, but I still stand by my my first proposal in this comment.

With the current implementation, we're introducing some unnecessary complexity, for example what happens if we do this?

const metrics = new Metrics({});
metrics.setFunctionName('foo');

export const handler = middy().use(logMetrics(metrics, { functionName: 'bar' )).handler(async () => {})

Right now bar would silently take precedence. We could add a warning signaling that this is happening, but I think the entire issue would be bypassed by just doing this:

const metrics = new Metrics({
  functionName: 'foo'
});

and potentially deprecating the setFunctionName() method.

---

Apologies for the confusion and any rework caused, but I think this is the best path forward for customers.

[am29d]

I think the idea with the middy and decorator option is to have one new Metrics() objects that is consumed by many functions, so you have the same configuration for all Lambdas, which have different resource names. If we don't allow middy/decorator function configuration, every function would use the same functionName or you'd need to create 100s of new Metric().

I agree that we need strive for simplicity, and having ENV variable to control this parameter is enough, since it is 1:1 mapping between resource name and environment variable. This means we keep three additions, except the middy/decorator option?

• .setFunctionName() remains as-is
• functionName is a Metric constructor parameter
• The middy handler only sets functionName if it's not defined
• Support POWERTOOLS_FUNCTION_NAME for better CDK static definition of the value as it's likely a more stable infra value compared to function code

We can still deprecate setFunctionName() in the future in favour of constructor and ENV.

[dreamorosi]

I think the idea with the middy and decorator option is to have one new Metrics() objects that is consumed by many functions

How does that work? This dimension is specifically for the ColdStart metric and there can only ever be 1 handler for each sandbox. Here we are not talking about decorating/wrapping arbitrary functions because this doesn't apply there. So in practice there's only ever a 1:1 mapping between Metrics instance and actual Lambda function handler.

With that said, if you really want to do this, you can use the setFunctionName() method I guess, but I don't think we should add yet another env variable for this. Both the constructor parameter and the method are not opinionated about where the actual runtime value comes from, and customers can easily do this:

const metrics = new Metrics({
  functionName: process.env.FOO
});

// or

metrics.functionName(process.env.FOO);

[dreamorosi]

Hi @steven10172, just wanted to check in to see if you were still interested in completing the PR.

If not, or no longer available (it's fine, things change!), let us know so we can take over the implementation.

[steven10172]

I intend to still work on this.

I have no objections to your recommendations and example code. I was not intending to deprecate the other method, but with your buy-in I'm all for it. As I was working through my implementation I recognized there was a non ideal pattern for the non decorator/middy path, thanks for the suggestion of updating the coldstart method.

[sonarqubecloud]

Please retry analysis of this Pull-Request directly on SonarQube Cloud

[dreamorosi]

Thanks for updating the PR, I gave it a quick look and I don't see any major issue.

I'll pull the branch and make a final review tomorrow morning.

[dreamorosi]

Thank you for working on this and addressing all our comments throughout the past couple weeks.

I have made a couple small commits to align the PR with the suggested branch, but the logic stays the same.

[leandrodamascena]

Hey @steven10172 and @dreamorosi! Great job you guys did here! I just left 3 comments that at least are worth checking out, so I'm going to request changes.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
17.3% Duplication on New Code

See analysis details on SonarQube Cloud

[boring-cyborg]

Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience!