RFC: Log Alarm L2 Construct - #962
Conversation
| a scheduled Logs query (a query string + aggregation over one or more log groups) and compares the aggregated result | ||
| against a threshold using an M-out-of-N evaluation. It also can surface matching log lines in the alarm notification. | ||
| The construct wraps the `AWS::CloudWatch::LogAlarm` CloudFormation resource, reuses the existing | ||
| `ComparisonOperator`/`TreatMissingData` enums, and — like `Alarm` and `PromQLAlarm` — extends `AlarmBase` so it is |
There was a problem hiding this comment.
Please note that ComparisonOperator we use for log alarm doesn't support LessThanLowerOrGreaterThanUpperThreshold | LessThanLowerThreshold | GreaterThanUpperThreshold being supported under Metric Alarm. These operations are supported for anomaly detection metric alarm and it is not supported for log alarm.
There was a problem hiding this comment.
We can still use this enum and validate the parameter at synth. ( Probably, PromQL should also be doing the similar thing if they are reusing the enum ?)
There was a problem hiding this comment.
I see you already covered below:
comparisonOperatormust be a static-threshold operator (anomaly-detection operators rejected).
| ## Summary | ||
|
|
||
| `LogAlarm` lets customers alarm directly on logs. Instead of a metric + threshold + evaluation periods, a log alarm runs | ||
| a scheduled Logs query (a query string + aggregation over one or more log groups) and compares the aggregated result |
| logGroupIdentifiers: [logGroup.logGroupName], | ||
| scheduledQueryRole: queryRole, | ||
| schedule: { | ||
| frequency: Duration.minutes(5), |
There was a problem hiding this comment.
nit: Should we just name it as rate to align it with scheduled expression rate expression naming ?
| import { Duration } from 'aws-cdk-lib'; | ||
|
|
||
| declare const logGroup: logs.LogGroup; | ||
| declare const queryRole: iam.IRole; |
There was a problem hiding this comment.
Can we see if SQ ( as in GA) supports anything to help the role creation ? If so, customers can use that to simplify the role creationrather than having their own code to create one.
If there is one, we can also replicate the similar thing for log line role as well.
| declare const logAlarm: cloudwatch.LogAlarm; | ||
| declare const topic: sns.Topic; | ||
|
|
||
| logAlarm.addAlarmAction(new cloudwatch_actions.SnsAction(topic)); |
There was a problem hiding this comment.
Let's see if we can add validation on supported actions for log alarm as well on synth ?
| ### Are there any open issues that need to be addressed later? | ||
|
|
||
| - Alignment of `ScheduledQueryConfiguration` field constraints across the Coral model, CFN schema, and Hermes validation | ||
| (tracked in EVENTS-4979): `LogGroupIdentifiers` required→optional, `StartTimeOffset` range `[1, 2592000]`, |
There was a problem hiding this comment.
Can't we just publish this with updated schema given we have already started the deployments? Or we plan to update it once we complete all deployments?
There was a problem hiding this comment.
Also, curious if these validations will be done at L1 construct based on CFN schema being published ?
| readonly treatMissingData?: TreatMissingData; // @default - service default | ||
| readonly actionLogLineCount?: number; // 0–50; requires role when > 0 | ||
| readonly actionLogLineRole?: IRole; | ||
| readonly alarmActions?: IAlarmAction[]; |
There was a problem hiding this comment.
Should we have default values for these as well or we can mark default none ? We can follow what is being followed for metric alarm. Same goes for other optional parameters like actionLogLineRole
| readonly actionsEnabled?: boolean; // @default true | ||
| readonly treatMissingData?: TreatMissingData; // @default - service default | ||
| readonly actionLogLineCount?: number; // 0–50 | ||
| readonly actionLogLineRole?: IRole; // auto-created (trusts cloudwatch.amazonaws.com) when count > 0 |
There was a problem hiding this comment.
Do you mean auto created only when not passed ? Also, we plan to auto create it , would we have all required parameters in place to auto create it ? Let's also see what further reviewer has take on this.
|
|
||
| ### Why should we _not_ do this? | ||
|
|
||
| There is no strong reason not to. The `ScheduledQueryConfiguration` field constraints are final (`logGroupIdentifiers` |
There was a problem hiding this comment.
Do we have to mention this now given we are going with what will be the final spec?
The
ScheduledQueryConfigurationfield constraints are final (logGroupIdentifiers
is optional, and thestartTimeOffsetrange is fixed), and the construct tracks the published
@aws-cdk/aws-service-spec
|
|
||
| ### Are there any open issues that need to be addressed later? | ||
|
|
||
| - None. The `ScheduledQueryConfiguration` field constraints are final; the L2 tracks the published |
There was a problem hiding this comment.
Again, we may not have to mention this "The ScheduledQueryConfiguration field constraints are final; the L2 tracks the published @aws-cdk/aws-service-spec for the AWS::CloudWatch::LogAlarm schema." unless needed. It will confuse the reviewer further.
|
Changes now looks good to me. |
2 participants
Tracking issue: #960
This RFC proposes a new
LogAlarmL2 construct inaws-cdk-lib/aws-cloudwatch, wrapping theAWS::CloudWatch::LogAlarmCloudFormation resource. It follows the acceptedPromQLAlarmprecedent (extendsAlarmBase, reuses existing enums, interchangeable viaIAlarm).A proof-of-concept construct with unit tests and an integration test has been implemented and deployed against a real account, confirming it creates a valid
AWS::CloudWatch::LogAlarm.See
text/0960-logalarm-l2.md.