fix: block forbidden chars from metric names #460
Conversation
There was a problem hiding this comment.
Hi, thanks so much for taking the time to write this PR up! I'm sorry that you ran into this metric naming issue.
This is a good PR, technically well written, conforms with our conventions, and includes good tests. I appreciate your hard work!
I'm not sure if this something we'd merge at this time, but I'm open to other feedback from additional users and we can revisit this conclusion in the future.
My reasoning is that we do document the format of metric names both implicitly in our lambda examples and explicitly in the datadog metric documentation.
If we did choose to validate the format in the Lambda function itself, we'd prefer to implement it in the Datadog Lambda extension, as this would benefit all users across all runtimes (not just nodejs). That also allows us to change the format in one place if that is ever needed in the future.
Finally this implementation does throw an unhandled exception when an invalid metric name is sent, which we wouldn't want to do as it would crash the running Lambda function.
We can leave this PR in an open state for now and see if other users would like this feature, but I don't intend to merge it at this time.
Thanks again!
| @@ -255,6 +260,14 @@ export function extractArgs<TEvent>(isResponseStreamFunction: boolean, ...args: | |||
| * @param tags The tags associated with the metric. Should be of the format "tag:value". | |||
| */ | |||
| export function sendDistributionMetricWithDate(name: string, value: number, metricTime: Date, ...tags: string[]) { | |||
| if (FORBIDDEN_METRIC_NAME_CHARS.some((c) => name.includes(c))) { | |||
| throw new MalformedMetricName( | |||
There was a problem hiding this comment.
This would cause the customer's lambda function to crash, which I don't think we'd want to do at any point
|
|
What does this PR do?
Block calling metric functions with names that will cause
datadog-agentto fail.Motivation
I should probably have known better, but it took me a merged PR all the way to prod and an error message in DataDog Serverless before I realized my code didn't work, and a clone of
datadog-agentGo code to realize what I'd messed up. So I figured this could help other people.Testing Guidelines
I added a new Jest case.
Additional Notes
I guess this is technically a breaking change. But if anybody would be affected by it we'd really be doing them a favor here as they would still have errors in their logs, only further down the pipeline. 🤷🏻
Types of Changes
Check all that apply