feat(infra): alarm on ERROR-level server log lines via CloudWatch Logs metric filters - #475
Conversation
…s metric filters Closes #99. The metric-based error-rate, latency, and throughput coverage landed with #300 (PR #442) and alarm routing is #443 (PR #474); the one piece of #99 still open was monitoring log-level error patterns. - infra/log_alarms.tf: metric filters on the server log group matching the JSON level field (ERROR always, WARN only alongside the dashboard), publishing log_error_events / log_warn_events under <metrics_namespace>/Logs, and the <stack>-server-log-errors alarm (Sum > alarm_log_error_threshold, default 0, in two consecutive 5-minute periods). Independent of the metrics pipeline, so it survives guardian_metrics_enabled = false. - cloudwatch_log_alarms_enabled (default true) gates it; a plan-time precondition requires guardian_log_format = json. - Dashboard gains a 'Server log lines by level' widget when the filters exist; new outputs cloudwatch_log_alarms_enabled, log_metrics_namespace, server_log_errors_alarm_name. - Docs: SERVER_AWS_DEPLOY.md log-level alarms section and verify step, infra/README.md, docs/architecture/infra.md, docs/PRODUCTION.md.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (11)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughCloudWatch Logs now provide independent ERROR and WARN metric filters. Terraform provisions a sustained ERROR alarm, an optional dashboard widget, configuration variables, outputs, deployment verification, and documentation for JSON log requirements. ChangesCloudWatch log monitoring
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Server
participant CloudWatchLogs
participant CloudWatchMetricAlarm
participant CloudWatchDashboard
Server->>CloudWatchLogs: Emit JSON ERROR and WARN log lines
CloudWatchLogs->>CloudWatchMetricAlarm: Count ERROR lines
CloudWatchMetricAlarm->>CloudWatchMetricAlarm: Evaluate two 5-minute periods
CloudWatchLogs->>CloudWatchDashboard: Publish ERROR and WARN counts
Merge Risk: ⚪ Minimal · up to The log alarm configuration, dashboard behavior, and JSON log matching are consistent across the enabled configurations reviewed. The change is ready to merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
A rabbit checks the logs at night Comment |
|
|
||
| # 5. Log metric filters are attached and counting (skipped when | ||
| # cloudwatch_log_alarms_enabled = false: LOG_ALARM is empty then). | ||
| # Datapoints appear only while log lines flow, i.e. once the service is |
There was a problem hiding this comment.
ALB health checks do not emit span-close lines: status_root is not instrumented, and track_http only records metrics. A healthy but quiet service can therefore have missing datapoints. Please remove the health-check claim and clarify that default_value = "0" publishes zeros only while logs are being ingested without matching ERROR events, otherwise missing data is expected and handled by treat_missing_data = "notBreaching".
zeljkoX
left a comment
zeljkoX
left a comment
There was a problem hiding this comment.
LGTM
Seems like this PR slightly depend on 474 and its changes that should be incorporated into this PR once it lands.
Resolves the additive conflicts with the alarm-notification stack and, as requested in the #474 review, wires the server-log-errors alarm to local.effective_alarm_actions and appends local.alarm_description_links to its description like every other alarm. Documents that the managed SNS topic cascades off with the metrics pipeline, so in the metrics-off mode the log alarm reaches only operator-supplied alarm_actions.
…lters ALB health checks hit status_root, which is not instrumented, and the HTTP metrics middleware records metrics only, so a healthy but quiet service ingests no log lines. default_value = "0" publishes zeros only while lines are being ingested without matching ERROR events; otherwise the data is missing and treat_missing_data = notBreaching keeps the alarm OK.
There was a problem hiding this comment.
🟡 Changes recommended
The alarm can include a dead dashboard link when metrics are disabled, and documentation/comment corrections remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds independent CloudWatch log-level monitoring with ERROR/WARN metric filters, an ERROR alarm, dashboard visibility, configuration, outputs, and documentation.
Changes:
- Adds Terraform log filters, alarm configuration, and outputs.
- Adds a server log-level dashboard widget.
- Updates deployment, production, and architecture documentation.
File summaries
| File | Description |
|---|---|
infra/variables.tf |
Adds log alarm configuration variables. |
infra/terraform.tfvars.example |
Documents new settings. |
infra/README.md |
Updates infrastructure monitoring documentation. |
infra/outputs.tf |
Exposes log monitoring outputs. |
infra/observability.tf |
Adds the log-level dashboard widget. |
infra/log_alarms.tf |
Defines log metric filters and the ERROR alarm. |
infra/ecs.tf |
Uses normalized log format handling. |
infra/data.tf |
Adds log monitoring locals. |
docs/SERVER_AWS_DEPLOY.md |
Documents setup and verification. |
docs/PRODUCTION.md |
Updates production observability guidance. |
docs/architecture/infra.md |
Maps the new infrastructure resources. |
Review details
Suppressed comments (2)
docs/SERVER_AWS_DEPLOY.md:854
- The log check is guarded on an empty
LOG_ALARM, but the earlier metrics notification check is unconditional. In the supportedcloudwatch_metrics_enabled = falsemode,metrics_missing_alarm_nameis empty, so copying this verification block fails at step 4 before reaching the independent log-alarm check; guard the metrics-specific test on$ALARMor explicitly tell operators to skip it when the metrics pipeline is disabled.
# 5. Log metric filters are attached and counting (skipped when
# cloudwatch_log_alarms_enabled = false: LOG_ALARM is empty then).
# default_value = "0" publishes zeros only while log lines are being
# ingested without matching ERROR events; a healthy but quiet service
# (ALB health checks log nothing at the default filter) ingests no
# lines, so an empty Datapoints list is expected and is handled by
# treat_missing_data = "notBreaching", not a sign the filter is missing.
infra/log_alarms.tf:24
default_valueis emitted once for a period in which the log group has events but none match; it is not emitted for every non-matching event. The current comment overstates the datapoint frequency, which can lead to incorrect expectations about metric sums and dashboard continuity.
# default_value = "0" makes each filter emit 0 for every non-matching
# event, so the series is continuous while log lines are being ingested:
# the dashboard draws zeros instead of gaps and the alarm clears on real
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| count = var.cloudwatch_log_alarms_enabled ? 1 : 0 | ||
|
|
||
| alarm_name = "${var.stack_name}-server-log-errors" | ||
| alarm_description = "Guardian server logged more than ${var.alarm_log_error_threshold} ERROR-level line(s) per 5-minute period in two consecutive periods (absolute count, independent of request volume; query the server log group for level = \"ERROR\")${local.alarm_description_links}" |
The alarm exists with the metrics pipeline off, where the <stack>-server dashboard and the ADOT sidecar are not provisioned, so its description now uses a variant suffix without the dashboard link and sidecar stream note in that mode. Also notes in the verify block that steps 1-4 need the metrics pipeline while step 5 does not.
github-project-automation
Bot
moved this from Review
to Done
in OZ Development for Miden
Closes #99.
Per the triage comment, the metric-based error-rate, latency, and throughput coverage landed with #300 (PR #442) and alarm routing is #443 (PR #474). The one piece of #99 still open was monitoring log-level error patterns; this PR adds it.
What
infra/log_alarms.tf: CloudWatch Logs metric filters on the server log group matching the JSONlevelfield the server emits withguardian_log_format = "json".log_error_events(always) andlog_warn_events(only alongside the dashboard, its sole consumer) are published under<metrics_namespace>/Logs, kept apart from the scraped metrics so "metrics arriving inmetrics_namespace" stays the ADOT pipeline health check.<stack>-server-log-errors:Sum > alarm_log_error_threshold(default 0) in each of two consecutive 5-minute periods. It is the absolute complement to the rate alarms: on a low-traffic stack a few failures never move a percentage that ALB health checks dominate, but every one is an ERROR line.cloudwatch_log_alarms_enabled(default true), independent ofguardian_metrics_enabled/cloudwatch_metrics_enabled, so it keeps working with the metrics pipeline off. A plan-time precondition on the filter rejectstext/compactlog formats with the flag on.cloudwatch_log_alarms_enabled,log_metrics_namespace,server_log_errors_alarm_name.docs/SERVER_AWS_DEPLOY.md;infra/README.md,docs/architecture/infra.md,docs/PRODUCTION.mdupdated.Design notes
adotandca-initstreams. The ADOT Collector writes console-encoded (non-JSON) lines, which a JSON pattern never matches; collector faults still surface through<stack>-metrics-missing. Anything that is not a JSON object with an uppercaselevel(panic text from a crash-looping task, for instance) is invisible to these filters, so the alarm covers logged faults, not liveness. Documented.ERRORfor some client-caused rejections before mapping them to 4xx (signature/cosigner checks inmetadata/auth, credential validation inconfigure_account), so a persistently misconfigured client can trip the default threshold. Documented withalarm_log_error_thresholdas the tolerance knob; downgrading those call sites towarn!is the longer-term server-side fix.main. Once feat(infra): route CloudWatch alarms to per-environment Slack channels #474 lands, the new alarm'salarm_actions = var.alarm_actionsbecomeslocal.effective_alarm_actionslike the others (one-line rebase).default_value = "0"keeps the series continuous while log lines flow (zeros on the dashboard instead of gaps); with no lines at all the alarm falls back totreat_missing_data = notBreaching.Summary by CodeRabbit
New Features
Documentation