Skip to content

Commit

Permalink
fix: Allow non-integral latency thresholds and durations (#587)
Browse files Browse the repository at this point in the history
Fixes #586.

Adds the `integral: false` option to [`Duration.toMillisecond()`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.Duration.html#towbrmillisecondsopts) calls to allow the original non-integral/sub-millisecond threshold values to be provided to CloudWatch. These four lines are the only uses of `toMilliseconds` in the package.

I also created a new test for the `LatencyAlarmFactory` class to test these specific cases. To verify that they were working, I ran the tests without the fix in place and got the expected error messages: [gist](https://gist.github.com/sajidanw/37fd1ddd63ff763a911bb3a1cf58b2b8).

---

_By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license_
  • Loading branch information
sajidanw authored Oct 28, 2024
1 parent fb22a52 commit 1eff500
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 4 deletions.
8 changes: 4 additions & 4 deletions lib/common/monitoring/alarms/LatencyAlarmFactory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ export class LatencyAlarmFactory {
ComparisonOperator.GREATER_THAN_THRESHOLD,
...props,
disambiguator,
threshold: props.maxLatency.toMilliseconds(),
threshold: props.maxLatency.toMilliseconds({ integral: false }),
alarmNameSuffix,
// we will dedupe any kind of latency issue to the same ticket
alarmDedupeStringSuffix: this.alarmFactory
Expand Down Expand Up @@ -200,7 +200,7 @@ export class LatencyAlarmFactory {
ComparisonOperator.GREATER_THAN_THRESHOLD,
...props,
disambiguator,
threshold: props.maxLatency.toMilliseconds(),
threshold: props.maxLatency.toMilliseconds({ integral: false }),
alarmNameSuffix,
// we will dedupe any kind of latency issue to the same alarm
alarmDedupeStringSuffix: this.alarmFactory
Expand Down Expand Up @@ -230,7 +230,7 @@ export class LatencyAlarmFactory {
ComparisonOperator.GREATER_THAN_THRESHOLD,
...props,
disambiguator,
threshold: props.maxDuration.toMilliseconds(),
threshold: props.maxDuration.toMilliseconds({ integral: false }),
alarmNameSuffix,
// we will dedupe any kind of latency issue to the same ticket
alarmDedupeStringSuffix: this.alarmFactory
Expand Down Expand Up @@ -264,7 +264,7 @@ export class LatencyAlarmFactory {
ComparisonOperator.GREATER_THAN_THRESHOLD,
...props,
disambiguator,
threshold: props.maxDuration.toMilliseconds(),
threshold: props.maxDuration.toMilliseconds({ integral: false }),
alarmNameSuffix,
// we will dedupe any kind of latency issue to the same ticket
alarmDedupeStringSuffix: this.alarmFactory
Expand Down
67 changes: 67 additions & 0 deletions test/common/alarm/LatencyAlarmFactory.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import { Duration, Stack } from "aws-cdk-lib";
import { Metric } from "aws-cdk-lib/aws-cloudwatch";
import { Construct } from "constructs";

import {
AlarmFactory,
AlarmFactoryDefaults,
LatencyAlarmFactory,
LatencyType,
MetricFactoryDefaults,
noopAction,
} from "../../../lib";

const stack = new Stack();
const construct = new Construct(stack, "SampleConstruct");

const globalMetricDefaults: MetricFactoryDefaults = {
namespace: "DummyNamespace",
};
const globalAlarmDefaults: AlarmFactoryDefaults = {
alarmNamePrefix: "DummyServiceAlarms",
actionsEnabled: true,
datapointsToAlarm: 6,
// we do not care about alarm actions in this test
action: noopAction(),
};
const factory = new AlarmFactory(construct, {
globalMetricDefaults,
globalAlarmDefaults,
localAlarmNamePrefix: "prefix",
});

const metric = new Metric({
metricName: "DummyMetric1",
namespace: "DummyCustomNamespace",
dimensionsMap: { CustomDimension: "CustomDimensionValue" },
});

const latencyAlarmFactory = new LatencyAlarmFactory(factory);

test("addLatencyAlarm: non-integral millisecond thresholds do not throw error", () => {
latencyAlarmFactory.addLatencyAlarm(metric, LatencyType.P99, {
maxLatency: Duration.millis(0.5),
});
});

test("addIntegrationLatencyAlarm: non-integral millisecond thresholds do not throw error", () => {
latencyAlarmFactory.addIntegrationLatencyAlarm(metric, LatencyType.P99, {
maxLatency: Duration.millis(2.5),
});
});

test("addDurationAlarm: non-integral millisecond durations do not throw error", () => {
latencyAlarmFactory.addDurationAlarm(metric, LatencyType.P99, {
maxDuration: Duration.millis(0.5),
});
});

test("addJvmGarbageCollectionDurationAlarm: non-integral millisecond durations do not throw error", () => {
latencyAlarmFactory.addJvmGarbageCollectionDurationAlarm(
metric,
LatencyType.P99,
{
maxDuration: Duration.millis(2.5),
},
);
});

0 comments on commit 1eff500

Please sign in to comment.