canonical-ubuntu-qa team mailing list archive

[Tim Andersson <mp+468247@xxxxxxxxxxxxxxxxxx>]

Tim Andersson has proposed merging ~andersson123/autopkgtest-cloud:apache-logging-monitoring-check-error-log-too into autopkgtest-cloud:master.

Commit message:
fix: web: also check error log in apache-request-monitoring

This script had a fatal flaw - we were only checking
/var/log/apache2/access.log and not /var/log/apache2/error.log, meaning
there were no records of 500's in the data and subsequently in the
grafana KPI, which is arguably one of the most important http response
status codes we'd want to keep track of.

This commit amends the issue by checking both the access.log and the
error.log.

It also makes the script a little bit more reliable as one of the
functions before could return None, which isn't ideal.

Requested reviews:
  Canonical's Ubuntu QA (canonical-ubuntu-qa)

For more details, see:
https://code.launchpad.net/~andersson123/autopkgtest-cloud/+git/autopkgtest-cloud/+merge/468247

We don't see any 500's in the grafana KPI, and the reason is that this script foolishly wasn't checking the error.log as well as the access.log. This commit changes the script to do so.
-- 
Your team Canonical's Ubuntu QA is requested to review the proposed merge of ~andersson123/autopkgtest-cloud:apache-logging-monitoring-check-error-log-too into autopkgtest-cloud:master.

diff --git a/charms/focal/autopkgtest-web/webcontrol/apache-request-monitoring b/charms/focal/autopkgtest-web/webcontrol/apache-request-monitoring
index f3702eb..115155e 100755
--- a/charms/focal/autopkgtest-web/webcontrol/apache-request-monitoring
+++ b/charms/focal/autopkgtest-web/webcontrol/apache-request-monitoring
@@ -22,9 +22,8 @@ def get_influx_client():
     return InfluxDBClient(hostname, port, username, password, database)
 
 
-def get_log_lines(last_x_minutes):
+def get_log_lines(last_x_minutes, apache_logfile):
     current_time = datetime.datetime.now()
-    apache_logfile = "/var/log/apache2/access.log"
     apache_logs = []
     with open(apache_logfile, "r") as f:
         apache_logs = f.read().splitlines()
@@ -47,10 +46,10 @@ def get_log_lines(last_x_minutes):
             return_lines.append(logline)
         else:
             return return_lines
+    return []
 
 
-def parse_http_codes(loglines):
-    data = {}
+def parse_http_codes(loglines, data):
     for line in loglines:
         code = line.split(" ")[8]
         if code not in data:
@@ -75,8 +74,12 @@ def main():
     )
     args = parser.parse_args()
     influx_client = get_influx_client()
-    loglines = get_log_lines(PROCESSED_TIME_RANGE)
-    data = parse_http_codes(loglines)
+    data = {}
+    for apache_logfile in ["access.log", "error.log"]:
+        loglines = get_log_lines(
+            PROCESSED_TIME_RANGE, f"/var/log/apache2/{apache_logfile}"
+        )
+        data = parse_http_codes(loglines, data)
     for _, i in data.items():
         if args.dry_run:
             print("Dry run enabled, would submit:\n%s" % str(i))