graham/towerops
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Refactor complex functions to reduce cyclomatic complexity

Browse source 
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.
This commit is contained in:
Graham McIntire 2026-01-06 13:09:51 -06:00
parent 77b41e0b24
commit b328dfb96e
No known key found for this signature in database
2 changed files with 341 additions and 321 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

634
lib/towerops/snmp/poller_worker.ex
View file

@ -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,

28
lib/towerops_web/live/equipment_live/show.ex
View file

@ -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