[SVLS-4422] Support metrics with timestamps when the Extension is running #522
Conversation
|
Do not merge before #519 |
should I just change the base to |
| @@ -167,7 +174,9 @@ export class MetricsListener { | |||
| } | |||
|
|
|||
| public sendDistributionMetric(name: string, value: number, forceAsync: boolean, ...tags: string[]) { | |||
| this.sendDistributionMetricWithDate(name, value, new Date(Date.now()), forceAsync, ...tags); | |||
| // The Extension doesn't support distribution metrics with timestamps. Use sendDistributionMetricWithDate instead. | |||
| const metricTime = this.isExtensionRunning ? new Date(0) : new Date(Date.now()); | |||
There was a problem hiding this comment.
What's the purpose of setting a metric time if the extension is still gonna ignore it? (as understood by the purpose of the PR)
There was a problem hiding this comment.
I wasn't too sure if changing the type for this function's parameter would introduce a breaking change to users or not, so I figured I'd rather just not change the type of the metricTime param from Date -> Date | undefined. But if it's not going to change much then I can just update the type to Date | undefined and pass undefined here
There was a problem hiding this comment.
I think the workaround makes sense. Changing the API would definitely make a breaking change, so that's a no in the mean time. Can you leave a note in the form of // TODO: Next breaking change, change API for metric?
| // Only create the processor to submit metrics to the API when a user provides a valid timestamp as | ||
| // Dogstatsd does not support timestamps for distributions. | ||
| this.currentProcessor = this.createProcessor(this.config, this.apiKey); | ||
| } else { |
There was a problem hiding this comment.
Let's try to avoid having if-else blocks. Also, wouldn't this need a return? This seems like it would process the metric if the extension is running but if this.config.logForwarding is set to true, it will both process it and then write it to stdout.
There was a problem hiding this comment.
The if here just creates the processor since it's typically never needed when using the extension, except for this special case, otherwise always send to statsd. Later on the metric is added to the processor at the end of the function (unchanged). So if logForwarding is true and the extension is running, then it will return on L153 and not process
There was a problem hiding this comment.
You are right, thanks for explaining!
What does this PR do?
Allows
sendDistributionMetricWithDateto submit a metric with a timestamp when the Extension is running. Neither hot-shots nor Dogstatsd supports historical distribution metrics (more on the latter here and here).The ability to send metrics with timestamps already existed but did not work when using the Extension.
Motivation
lambda_metricdatadog-lambda-python#398Testing Guidelines
Unit test + manual tests. Example of one invocation using the following:
Additional Notes
Types of Changes
Check all that apply