From b328dfb96e0d40c4090807b1bd39a36eed2719e2 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Tue, 6 Jan 2026 13:09:51 -0600 Subject: [PATCH] Refactor complex functions to reduce cyclomatic complexity Refactored three highly complex functions to improve code maintainability: 1. PollerWorker.detect_sensor_changes (complexity 33 -> ~9): - Extracted threshold checking logic into separate functions - Split event creation into focused helper functions - Reduced nesting by using pipe operator pattern 2. PollerWorker.detect_and_log_changes (complexity 16 -> ~5): - Created separate functions for each interface change type - Extracted event building logic into dedicated functions - Improved readability with clear function names 3. EquipmentLive.Show.format_duration (complexity 13 -> ~5): - Extracted time unit conversion into separate functions - Simplified pluralization logic with pattern matching - Reduced nested conditionals These changes make the code easier to test, maintain, and understand while preserving all existing functionality. --- lib/towerops/snmp/poller_worker.ex | 634 ++++++++++--------- lib/towerops_web/live/equipment_live/show.ex | 28 +- 2 files changed, 341 insertions(+), 321 deletions(-) diff --git a/lib/towerops/snmp/poller_worker.ex b/lib/towerops/snmp/poller_worker.ex index dec0f64c..ee19d131 100644 --- a/lib/towerops/snmp/poller_worker.ex +++ b/lib/towerops/snmp/poller_worker.ex @@ -309,143 +309,156 @@ defmodule Towerops.Snmp.PollerWorker do end defp detect_and_log_changes(interface, current_attrs, equipment_id, timestamp) do - changes = [] + events = + [] + |> maybe_add_oper_status_event(interface, current_attrs, equipment_id, timestamp) + |> maybe_add_admin_status_event(interface, current_attrs, equipment_id, timestamp) + |> maybe_add_speed_change_event(interface, current_attrs, equipment_id, timestamp) + |> maybe_add_mac_change_event(interface, current_attrs, equipment_id, timestamp) - # Check operational status change - changes = - if interface.if_oper_status == current_attrs.if_oper_status do - changes - else - event = %{ - equipment_id: equipment_id, - event_type: if(current_attrs.if_oper_status == "up", do: "interface_up", else: "interface_down"), - severity: if(current_attrs.if_oper_status == "up", do: "info", else: "warning"), - message: "Interface #{interface.if_name} is now #{current_attrs.if_oper_status}", - metadata: %{ - interface_id: interface.id, - interface_name: interface.if_name, - old_status: interface.if_oper_status, - new_status: current_attrs.if_oper_status - }, - occurred_at: timestamp - } - - [{:event, event} | changes] - end - - # Check admin status change - changes = - if interface.if_admin_status == current_attrs.if_admin_status do - changes - else - event = %{ - equipment_id: equipment_id, - event_type: "interface_admin_status_change", - severity: "info", - message: - "Interface #{interface.if_name} admin status changed from #{interface.if_admin_status} to #{current_attrs.if_admin_status}", - metadata: %{ - interface_id: interface.id, - interface_name: interface.if_name, - old_status: interface.if_admin_status, - new_status: current_attrs.if_admin_status - }, - occurred_at: timestamp - } - - [{:event, event} | changes] - end - - # Check speed change (including initial discovery) - changes = - if interface.if_speed != current_attrs.if_speed && current_attrs.if_speed != nil do - # Determine if this is initial discovery or a change - is_initial = interface.if_speed == nil - - severity = - cond do - is_initial -> "info" - current_attrs.if_speed < interface.if_speed -> "warning" - true -> "info" - end - - message = - if is_initial do - "Interface #{interface.if_name} speed detected: #{format_speed(current_attrs.if_speed)}" - else - "Interface #{interface.if_name} speed changed from #{format_speed(interface.if_speed)} to #{format_speed(current_attrs.if_speed)}" - end - - event = %{ - equipment_id: equipment_id, - event_type: "interface_speed_change", - severity: severity, - message: message, - metadata: %{ - interface_id: interface.id, - interface_name: interface.if_name, - old_speed: interface.if_speed, - new_speed: current_attrs.if_speed - }, - occurred_at: timestamp - } - - [{:event, event} | changes] - else - changes - end - - # Check MAC address change (including initial discovery) - changes = - if interface.if_phys_address != current_attrs.if_phys_address && - current_attrs.if_phys_address != nil do - # Determine if this is initial discovery or a change - is_initial = interface.if_phys_address == nil - - message = - if is_initial do - "Interface #{interface.if_name} MAC address detected: #{current_attrs.if_phys_address}" - else - "Interface #{interface.if_name} MAC address changed from #{interface.if_phys_address} to #{current_attrs.if_phys_address}" - end - - event = %{ - equipment_id: equipment_id, - event_type: "interface_mac_change", - severity: if(is_initial, do: "info", else: "warning"), - message: message, - metadata: %{ - interface_id: interface.id, - interface_name: interface.if_name, - old_mac: interface.if_phys_address, - new_mac: current_attrs.if_phys_address - }, - occurred_at: timestamp - } - - [{:event, event} | changes] - else - changes - end - - # If there are changes, broadcast events and update interface - if changes != [] do - # Broadcast all events via PubSub - Enum.each(changes, fn {:event, event_attrs} -> - Phoenix.PubSub.broadcast( - Towerops.PubSub, - "equipment:events", - {:equipment_event, event_attrs} - ) - - Logger.debug("Broadcast event: #{event_attrs.message}") - end) - - # Update interface with new attributes + if events != [] do + broadcast_interface_events(events) Snmp.update_interface(interface, current_attrs) end end + defp maybe_add_oper_status_event(events, interface, current_attrs, equipment_id, timestamp) do + if interface.if_oper_status == current_attrs.if_oper_status do + events + else + event = build_oper_status_event(interface, current_attrs, equipment_id, timestamp) + [{:event, event} | events] + end + end + + defp build_oper_status_event(interface, current_attrs, equipment_id, timestamp) do + %{ + equipment_id: equipment_id, + event_type: if(current_attrs.if_oper_status == "up", do: "interface_up", else: "interface_down"), + severity: if(current_attrs.if_oper_status == "up", do: "info", else: "warning"), + message: "Interface #{interface.if_name} is now #{current_attrs.if_oper_status}", + metadata: %{ + interface_id: interface.id, + interface_name: interface.if_name, + old_status: interface.if_oper_status, + new_status: current_attrs.if_oper_status + }, + occurred_at: timestamp + } + end + + defp maybe_add_admin_status_event(events, interface, current_attrs, equipment_id, timestamp) do + if interface.if_admin_status == current_attrs.if_admin_status do + events + else + event = build_admin_status_event(interface, current_attrs, equipment_id, timestamp) + [{:event, event} | events] + end + end + + defp build_admin_status_event(interface, current_attrs, equipment_id, timestamp) do + %{ + equipment_id: equipment_id, + event_type: "interface_admin_status_change", + severity: "info", + message: + "Interface #{interface.if_name} admin status changed from #{interface.if_admin_status} to #{current_attrs.if_admin_status}", + metadata: %{ + interface_id: interface.id, + interface_name: interface.if_name, + old_status: interface.if_admin_status, + new_status: current_attrs.if_admin_status + }, + occurred_at: timestamp + } + end + + defp maybe_add_speed_change_event(events, interface, current_attrs, equipment_id, timestamp) do + if interface.if_speed != current_attrs.if_speed && current_attrs.if_speed != nil do + event = build_speed_change_event(interface, current_attrs, equipment_id, timestamp) + [{:event, event} | events] + else + events + end + end + + defp build_speed_change_event(interface, current_attrs, equipment_id, timestamp) do + is_initial = interface.if_speed == nil + severity = determine_speed_change_severity(is_initial, interface.if_speed, current_attrs.if_speed) + message = format_speed_change_message(interface.if_name, is_initial, interface.if_speed, current_attrs.if_speed) + + %{ + equipment_id: equipment_id, + event_type: "interface_speed_change", + severity: severity, + message: message, + metadata: %{ + interface_id: interface.id, + interface_name: interface.if_name, + old_speed: interface.if_speed, + new_speed: current_attrs.if_speed + }, + occurred_at: timestamp + } + end + + defp determine_speed_change_severity(true, _old_speed, _new_speed), do: "info" + defp determine_speed_change_severity(false, old_speed, new_speed) when new_speed < old_speed, do: "warning" + defp determine_speed_change_severity(false, _old_speed, _new_speed), do: "info" + + defp format_speed_change_message(if_name, true, _old_speed, new_speed) do + "Interface #{if_name} speed detected: #{format_speed(new_speed)}" + end + + defp format_speed_change_message(if_name, false, old_speed, new_speed) do + "Interface #{if_name} speed changed from #{format_speed(old_speed)} to #{format_speed(new_speed)}" + end + + defp maybe_add_mac_change_event(events, interface, current_attrs, equipment_id, timestamp) do + if interface.if_phys_address != current_attrs.if_phys_address && current_attrs.if_phys_address != nil do + event = build_mac_change_event(interface, current_attrs, equipment_id, timestamp) + [{:event, event} | events] + else + events + end + end + + defp build_mac_change_event(interface, current_attrs, equipment_id, timestamp) do + is_initial = interface.if_phys_address == nil + + message = + format_mac_change_message(interface.if_name, is_initial, interface.if_phys_address, current_attrs.if_phys_address) + + %{ + equipment_id: equipment_id, + event_type: "interface_mac_change", + severity: if(is_initial, do: "info", else: "warning"), + message: message, + metadata: %{ + interface_id: interface.id, + interface_name: interface.if_name, + old_mac: interface.if_phys_address, + new_mac: current_attrs.if_phys_address + }, + occurred_at: timestamp + } + end + + defp format_mac_change_message(if_name, true, _old_mac, new_mac) do + "Interface #{if_name} MAC address detected: #{new_mac}" + end + + defp format_mac_change_message(if_name, false, old_mac, new_mac) do + "Interface #{if_name} MAC address changed from #{old_mac} to #{new_mac}" + end + + defp broadcast_interface_events(events) do + Enum.each(events, fn {:event, event_attrs} -> + Phoenix.PubSub.broadcast(Towerops.PubSub, "equipment:events", {:equipment_event, event_attrs}) + Logger.debug("Broadcast event: #{event_attrs.message}") + end) + end + defp parse_interface_integer(value) when is_integer(value), do: value defp parse_interface_integer(_), do: nil @@ -592,197 +605,196 @@ defmodule Towerops.Snmp.PollerWorker do :ok else equipment_id = get_equipment_id_from_sensor(sensor) - events = [] + thresholds = extract_thresholds(sensor.metadata) - # Get threshold configuration from sensor metadata - warning_high = sensor.metadata["warning_high"] - critical_high = sensor.metadata["critical_high"] - warning_low = sensor.metadata["warning_low"] - critical_low = sensor.metadata["critical_low"] - - # Check for threshold violations events = - cond do - critical_high && current_value >= critical_high -> - # Critical high threshold exceeded - event = - build_sensor_event( - equipment_id, - sensor, - "sensor_threshold_critical", - "critical", - "#{sensor.sensor_descr} is critically high: #{format_sensor_value(current_value, sensor.sensor_unit)} (threshold: #{critical_high}#{sensor.sensor_unit})", - %{ - current_value: current_value, - previous_value: sensor.last_value, - threshold_type: "critical_high", - threshold_value: critical_high - }, - timestamp - ) + [] + |> maybe_add_threshold_event(sensor, current_value, timestamp, equipment_id, thresholds) + |> maybe_add_change_event(sensor, current_value, timestamp, equipment_id) - [event | events] + broadcast_sensor_events(events) + end + end - critical_low && current_value <= critical_low -> - # Critical low threshold exceeded - event = - build_sensor_event( - equipment_id, - sensor, - "sensor_threshold_critical", - "critical", - "#{sensor.sensor_descr} is critically low: #{format_sensor_value(current_value, sensor.sensor_unit)} (threshold: #{critical_low}#{sensor.sensor_unit})", - %{ - current_value: current_value, - previous_value: sensor.last_value, - threshold_type: "critical_low", - threshold_value: critical_low - }, - timestamp - ) + defp extract_thresholds(metadata) do + %{ + warning_high: metadata["warning_high"], + critical_high: metadata["critical_high"], + warning_low: metadata["warning_low"], + critical_low: metadata["critical_low"] + } + end - [event | events] + defp maybe_add_threshold_event(events, sensor, current_value, timestamp, equipment_id, thresholds) do + threshold_event = check_threshold_violation(sensor, current_value, timestamp, equipment_id, thresholds) - warning_high && current_value >= warning_high -> - # Warning high threshold exceeded - event = - build_sensor_event( - equipment_id, - sensor, - "sensor_threshold_warning", - "warning", - "#{sensor.sensor_descr} is high: #{format_sensor_value(current_value, sensor.sensor_unit)} (threshold: #{warning_high}#{sensor.sensor_unit})", - %{ - current_value: current_value, - previous_value: sensor.last_value, - threshold_type: "warning_high", - threshold_value: warning_high - }, - timestamp - ) + if threshold_event, do: [threshold_event | events], else: events + end - [event | events] - - warning_low && current_value <= warning_low -> - # Warning low threshold exceeded - event = - build_sensor_event( - equipment_id, - sensor, - "sensor_threshold_warning", - "warning", - "#{sensor.sensor_descr} is low: #{format_sensor_value(current_value, sensor.sensor_unit)} (threshold: #{warning_low}#{sensor.sensor_unit})", - %{ - current_value: current_value, - previous_value: sensor.last_value, - threshold_type: "warning_low", - threshold_value: warning_low - }, - timestamp - ) - - [event | events] - - true -> - # Check if value returned to normal from a threshold violation - was_over_threshold = - (warning_high && sensor.last_value >= warning_high) || - (critical_high && sensor.last_value >= critical_high) || - (warning_low && sensor.last_value <= warning_low) || - (critical_low && sensor.last_value <= critical_low) - - is_now_normal = - (warning_high == nil || current_value < warning_high) && - (critical_high == nil || current_value < critical_high) && - (warning_low == nil || current_value > warning_low) && - (critical_low == nil || current_value > critical_low) - - if was_over_threshold && is_now_normal do - event = - build_sensor_event( - equipment_id, - sensor, - "sensor_threshold_normal", - "info", - "#{sensor.sensor_descr} returned to normal: #{format_sensor_value(current_value, sensor.sensor_unit)}", - %{ - current_value: current_value, - previous_value: sensor.last_value - }, - timestamp - ) - - [event | events] - else - events - end - end - - # Check for significant value changes (spikes or drops) - # Only for percentage-based sensors (CPU, memory, storage) - events = - if sensor.sensor_unit == "%" do - change_percent = abs(current_value - sensor.last_value) - - cond do - # Spike: increase of 30% or more - change_percent >= 30 && current_value > sensor.last_value -> - event = - build_sensor_event( - equipment_id, - sensor, - "sensor_value_spike", - "warning", - "#{sensor.sensor_descr} spiked from #{format_sensor_value(sensor.last_value, sensor.sensor_unit)} to #{format_sensor_value(current_value, sensor.sensor_unit)}", - %{ - current_value: current_value, - previous_value: sensor.last_value, - change: change_percent - }, - timestamp - ) - - [event | events] - - # Drop: decrease of 30% or more - change_percent >= 30 && current_value < sensor.last_value -> - event = - build_sensor_event( - equipment_id, - sensor, - "sensor_value_drop", - "info", - "#{sensor.sensor_descr} dropped from #{format_sensor_value(sensor.last_value, sensor.sensor_unit)} to #{format_sensor_value(current_value, sensor.sensor_unit)}", - %{ - current_value: current_value, - previous_value: sensor.last_value, - change: change_percent - }, - timestamp - ) - - [event | events] - - true -> - events - end - else - events - end - - # Broadcast all events - Enum.each(events, fn event -> - Phoenix.PubSub.broadcast( - Towerops.PubSub, - "equipment:events", - {:equipment_event, event} + defp check_threshold_violation(sensor, current_value, timestamp, equipment_id, thresholds) do + cond do + thresholds.critical_high && current_value >= thresholds.critical_high -> + build_threshold_event( + sensor, + current_value, + timestamp, + equipment_id, + "critical_high", + thresholds.critical_high, + "critical", + "critically high" ) - Logger.debug("Sensor event: #{event.message}") - end) + thresholds.critical_low && current_value <= thresholds.critical_low -> + build_threshold_event( + sensor, + current_value, + timestamp, + equipment_id, + "critical_low", + thresholds.critical_low, + "critical", + "critically low" + ) + + thresholds.warning_high && current_value >= thresholds.warning_high -> + build_threshold_event( + sensor, + current_value, + timestamp, + equipment_id, + "warning_high", + thresholds.warning_high, + "warning", + "high" + ) + + thresholds.warning_low && current_value <= thresholds.warning_low -> + build_threshold_event( + sensor, + current_value, + timestamp, + equipment_id, + "warning_low", + thresholds.warning_low, + "warning", + "low" + ) + + true -> + check_returned_to_normal(sensor, current_value, timestamp, equipment_id, thresholds) end end + defp build_threshold_event( + sensor, + current_value, + timestamp, + equipment_id, + threshold_type, + threshold_value, + severity, + description + ) do + event_type = if severity == "critical", do: "sensor_threshold_critical", else: "sensor_threshold_warning" + + build_sensor_event( + equipment_id, + sensor, + event_type, + severity, + "#{sensor.sensor_descr} is #{description}: #{format_sensor_value(current_value, sensor.sensor_unit)} (threshold: #{threshold_value}#{sensor.sensor_unit})", + %{ + current_value: current_value, + previous_value: sensor.last_value, + threshold_type: threshold_type, + threshold_value: threshold_value + }, + timestamp + ) + end + + defp check_returned_to_normal(sensor, current_value, timestamp, equipment_id, thresholds) do + was_over_threshold = value_was_over_threshold?(sensor.last_value, thresholds) + is_now_normal = value_is_normal?(current_value, thresholds) + + if was_over_threshold && is_now_normal do + build_sensor_event( + equipment_id, + sensor, + "sensor_threshold_normal", + "info", + "#{sensor.sensor_descr} returned to normal: #{format_sensor_value(current_value, sensor.sensor_unit)}", + %{current_value: current_value, previous_value: sensor.last_value}, + timestamp + ) + end + end + + defp value_was_over_threshold?(last_value, thresholds) do + (thresholds.warning_high && last_value >= thresholds.warning_high) || + (thresholds.critical_high && last_value >= thresholds.critical_high) || + (thresholds.warning_low && last_value <= thresholds.warning_low) || + (thresholds.critical_low && last_value <= thresholds.critical_low) + end + + defp value_is_normal?(current_value, thresholds) do + (thresholds.warning_high == nil || current_value < thresholds.warning_high) && + (thresholds.critical_high == nil || current_value < thresholds.critical_high) && + (thresholds.warning_low == nil || current_value > thresholds.warning_low) && + (thresholds.critical_low == nil || current_value > thresholds.critical_low) + end + + defp maybe_add_change_event(events, sensor, current_value, timestamp, equipment_id) do + # Only check for spikes/drops on percentage sensors + if sensor.sensor_unit == "%" do + change_event = check_significant_change(sensor, current_value, timestamp, equipment_id) + if change_event, do: [change_event | events], else: events + else + events + end + end + + defp check_significant_change(sensor, current_value, timestamp, equipment_id) do + change_percent = abs(current_value - sensor.last_value) + + cond do + # Spike: increase of 30% or more + change_percent >= 30 && current_value > sensor.last_value -> + build_sensor_event( + equipment_id, + sensor, + "sensor_value_spike", + "warning", + "#{sensor.sensor_descr} spiked from #{format_sensor_value(sensor.last_value, sensor.sensor_unit)} to #{format_sensor_value(current_value, sensor.sensor_unit)}", + %{current_value: current_value, previous_value: sensor.last_value, change: change_percent}, + timestamp + ) + + # Drop: decrease of 30% or more + change_percent >= 30 && current_value < sensor.last_value -> + build_sensor_event( + equipment_id, + sensor, + "sensor_value_drop", + "info", + "#{sensor.sensor_descr} dropped from #{format_sensor_value(sensor.last_value, sensor.sensor_unit)} to #{format_sensor_value(current_value, sensor.sensor_unit)}", + %{current_value: current_value, previous_value: sensor.last_value, change: change_percent}, + timestamp + ) + + true -> + nil + end + end + + defp broadcast_sensor_events(events) do + Enum.each(events, fn event -> + Phoenix.PubSub.broadcast(Towerops.PubSub, "equipment:events", {:equipment_event, event}) + Logger.debug("Sensor event: #{event.message}") + end) + end + defp build_sensor_event(equipment_id, sensor, event_type, severity, message, metadata, timestamp) do base_metadata = %{ sensor_id: sensor.id, diff --git a/lib/towerops_web/live/equipment_live/show.ex b/lib/towerops_web/live/equipment_live/show.ex index 0381140c..c0f8d0bf 100644 --- a/lib/towerops_web/live/equipment_live/show.ex +++ b/lib/towerops_web/live/equipment_live/show.ex @@ -156,20 +156,19 @@ defmodule ToweropsWeb.EquipmentLive.Show do defp format_uptime(_), do: "N/A" defp format_duration(total_seconds) do - weeks = div(total_seconds, 604_800) - days = div(rem(total_seconds, 604_800), 86_400) - hours = div(rem(total_seconds, 86_400), 3600) - minutes = div(rem(total_seconds, 3600), 60) - seconds = rem(total_seconds, 60) + {weeks, remainder} = seconds_to_weeks(total_seconds) + {days, remainder} = seconds_to_days(remainder) + {hours, remainder} = seconds_to_hours(remainder) + {minutes, seconds} = seconds_to_minutes(remainder) parts = Enum.reject( [ - if(weeks > 0, do: "#{weeks} week#{if weeks > 1, do: "s"}"), - if(days > 0, do: "#{days} day#{if days > 1, do: "s"}"), - if(hours > 0, do: "#{hours} hour#{if hours > 1, do: "s"}"), - if(minutes > 0, do: "#{minutes} minute#{if minutes > 1, do: "s"}"), - if(seconds > 0, do: "#{seconds} second#{if seconds > 1, do: "s"}") + format_time_part(weeks, "week"), + format_time_part(days, "day"), + format_time_part(hours, "hour"), + format_time_part(minutes, "minute"), + format_time_part(seconds, "second") ], &is_nil/1 ) @@ -180,6 +179,15 @@ defmodule ToweropsWeb.EquipmentLive.Show do end end + defp seconds_to_weeks(seconds), do: {div(seconds, 604_800), rem(seconds, 604_800)} + defp seconds_to_days(seconds), do: {div(seconds, 86_400), rem(seconds, 86_400)} + defp seconds_to_hours(seconds), do: {div(seconds, 3600), rem(seconds, 3600)} + defp seconds_to_minutes(seconds), do: {div(seconds, 60), rem(seconds, 60)} + + defp format_time_part(0, _unit), do: nil + defp format_time_part(1, unit), do: "1 #{unit}" + defp format_time_part(count, unit), do: "#{count} #{unit}s" + defp format_device_age(nil), do: "N/A" defp format_device_age(datetime) do