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

[SVLS-4422] Support metrics with timestamps when the Extension is running #522

Merged
merged 8 commits into from
Mar 27, 2024
Merged
45 changes: 45 additions & 0 deletions src/metrics/listener.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,51 @@ describe("MetricsListener", () => {
expect(distributionMock).toHaveBeenCalledWith("my-metric", 10, undefined, ["tag:a", "tag:b"]);
});

it("only sends metrics with timestamps to the API when the extension is enabled", async () => {
const flushScope = nock(EXTENSION_URL).post("/lambda/flush", JSON.stringify({})).reply(200);
mock({
"/opt/extensions/datadog-agent": Buffer.from([0]),
});
nock("https://api.example.com").post("/api/v1/distribution_points?api_key=api-key").reply(200, {});

const distributionMock = jest.fn();
(StatsDClient as any).mockImplementation(() => {
return {
distribution: distributionMock,
close: (callback: any) => callback(undefined),
};
});

jest.spyOn(Date, "now").mockImplementation(() => 1487076708000);

const metricTimeOneMinuteAgo = new Date(Date.now() - 60000);
const kms = new MockKMS("kms-api-key-decrypted");
const listener = new MetricsListener(kms as any, {
apiKey: "api-key",
apiKeyKMS: "",
enhancedMetrics: false,
logForwarding: false,
shouldRetryMetrics: false,
localTesting: true,
siteURL,
});

await listener.onStartInvocation({});
listener.sendDistributionMetricWithDate(
"my-metric-with-a-timestamp",
10,
metricTimeOneMinuteAgo,
false,
"tag:a",
"tag:b",
);
listener.sendDistributionMetric("my-metric-without-a-timestamp", 10, false, "tag:a", "tag:b");
await listener.onCompleteInvocation();
expect(flushScope.isDone()).toBeTruthy();
expect(nock.isDone()).toBeTruthy();
expect(distributionMock).toHaveBeenCalledWith("my-metric-without-a-timestamp", 10, undefined, ["tag:a", "tag:b"]);
});

it("logs metrics when logForwarding is enabled with custom timestamp", async () => {
const spy = jest.spyOn(process.stdout, "write");
// jest.spyOn(Date, "now").mockImplementation(() => 1487076708000);
Expand Down
19 changes: 14 additions & 5 deletions src/metrics/listener.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,17 +140,24 @@ export class MetricsListener {
forceAsync: boolean,
...tags: string[]
) {
const dist = new Distribution(name, [{ timestamp: metricTime, value }], ...tags);

if (this.isExtensionRunning) {
this.statsDClient?.distribution(name, value, undefined, tags);
return;
const isMetricTimeValid = Date.parse(metricTime.toString()) > 0;
if (isMetricTimeValid) {
// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, thanks for explaining!

this.statsDClient?.distribution(name, value, undefined, tags);
return;
}
}
if (this.config.logForwarding || forceAsync) {
writeMetricToStdout(name, value, metricTime, tags);
return;
}

const dist = new Distribution(name, [{ timestamp: metricTime, value }], ...tags);

if (!this.apiKey) {
const errorMessage = "api key not configured, see https://dtdg.co/sls-node-metrics";
logError(errorMessage);
Expand All @@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@duncanista duncanista Mar 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yessir 👍

this.sendDistributionMetricWithDate(name, value, metricTime, forceAsync, ...tags);
}

private async createProcessor(config: MetricsConfig, apiKey: Promise<string>) {
Expand Down
Loading