From 86db804729e1e5c375080d9c34af9fdcbf7c76b9 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Wed, 25 Mar 2026 11:03:43 -0500 Subject: [PATCH] fix: correct 32-bit SNMP counter wrap detection for all traffic calculations (#163) The wrap detection only applied the 2^32 correction when the previous counter was in the upper half of the 32-bit range (> 2^31). On fast links, counters wrap regardless of where in the range they sit. Three graph rendering paths had no wrap detection at all, just max(0). Consolidated all bps calculations into a single public Capacity.calculate_bps/3 that always tries the 2^32 correction for negative deltas. 64-bit HC counter resets remain clamped to zero since the corrected delta stays negative. Reviewed-on: https://git.mcintire.me/graham/towerops-web/pulls/163 --- lib/towerops/capacity.ex | 25 +++++++---- .../graphql/resolvers/time_series.ex | 4 +- lib/towerops_web/live/device_live/show.ex | 11 +---- lib/towerops_web/live/graph_live/show.ex | 29 ++++--------- test/towerops/capacity_test.exs | 42 +++++++++++++++++-- 5 files changed, 66 insertions(+), 45 deletions(-) diff --git a/lib/towerops/capacity.ex b/lib/towerops/capacity.ex index 9d5e0bb8..2ed65de9 100644 --- a/lib/towerops/capacity.ex +++ b/lib/towerops/capacity.ex @@ -161,18 +161,27 @@ defmodule Towerops.Capacity do end end - defp calculate_bps(nil, _, _), do: 0.0 - defp calculate_bps(_, nil, _), do: 0.0 + @doc """ + Calculates bits per second from two consecutive octet counter readings. - defp calculate_bps(prev_octets, curr_octets, time_diff) do + Handles 32-bit SNMP counter wraps (ifInOctets/ifOutOctets wrap at 2^32) + and 64-bit HC counter resets (device reboot). Always tries a 2^32 + correction when the delta is negative — this works because: + + - 32-bit wrap: corrected delta is positive and correct + - 64-bit reset: corrected delta is still very negative, clamped to 0 + """ + def calculate_bps(nil, _, _), do: 0.0 + def calculate_bps(_, nil, _), do: 0.0 + + def calculate_bps(prev_octets, curr_octets, time_diff) do delta = curr_octets - prev_octets - # When prev is in the upper half of 32-bit range and delta is negative, - # the 32-bit counter likely wrapped around 2^32. Add 2^32 to recover - # the correct delta. If it's still negative after the correction, it's - # a device reboot/reset on 64-bit HC counters — clamp to 0. delta = - if delta < 0 and prev_octets > 2_147_483_648 do + if delta < 0 do + # Try 32-bit counter wrap correction. For 64-bit HC counter resets + # (e.g. 10 TB → 1 MB), the corrected delta stays negative and gets + # clamped to 0 below. delta + 4_294_967_296 else delta diff --git a/lib/towerops_web/graphql/resolvers/time_series.ex b/lib/towerops_web/graphql/resolvers/time_series.ex index 8751de17..77849126 100644 --- a/lib/towerops_web/graphql/resolvers/time_series.ex +++ b/lib/towerops_web/graphql/resolvers/time_series.ex @@ -158,8 +158,8 @@ defmodule ToweropsWeb.GraphQL.Resolvers.TimeSeries do defp compute_bps(_, nil, _time_diff), do: 0.0 defp compute_bps(octets1, octets2, time_diff) do - ((octets2 - octets1) * 8 / time_diff) - |> max(0) + octets1 + |> Towerops.Capacity.calculate_bps(octets2, time_diff) |> Float.round(2) end diff --git a/lib/towerops_web/live/device_live/show.ex b/lib/towerops_web/live/device_live/show.ex index 12bad1fc..a99f198b 100644 --- a/lib/towerops_web/live/device_live/show.ex +++ b/lib/towerops_web/live/device_live/show.ex @@ -1263,16 +1263,7 @@ defmodule ToweropsWeb.DeviceLive.Show do end defp calculate_counter_bps(prev_octets, curr_octets, time_diff) do - delta = curr_octets - prev_octets - - delta = - if delta < 0 and prev_octets > 2_147_483_648 do - delta + 4_294_967_296 - else - delta - end - - max(0.0, delta * 8 / time_diff) + Towerops.Capacity.calculate_bps(prev_octets, curr_octets, time_diff) end defp build_traffic_json_datasets(all_stats, device) do diff --git a/lib/towerops_web/live/graph_live/show.ex b/lib/towerops_web/live/graph_live/show.ex index 7ad29dc0..df6979f0 100644 --- a/lib/towerops_web/live/graph_live/show.ex +++ b/lib/towerops_web/live/graph_live/show.ex @@ -887,21 +887,15 @@ defmodule ToweropsWeb.GraphLive.Show do stats |> Enum.chunk_every(2, 1, :discard) |> Enum.map(fn [stat1, stat2] -> - # Skip if octets are nil if is_nil(stat1.if_in_octets) or is_nil(stat2.if_in_octets) do nil else time_diff = stat2.checked_at |> DateTime.diff(stat1.checked_at, :second) |> max(1) - - bps = - ((stat2.if_in_octets - stat1.if_in_octets) * 8 / time_diff) - |> max(0) - |> :erlang.float() - |> Float.round(2) + bps = Towerops.Capacity.calculate_bps(stat1.if_in_octets, stat2.if_in_octets, time_diff) %{ x: DateTime.to_unix(stat2.checked_at, :millisecond), - y: bps + y: Float.round(bps, 2) } end end) @@ -912,21 +906,15 @@ defmodule ToweropsWeb.GraphLive.Show do stats |> Enum.chunk_every(2, 1, :discard) |> Enum.map(fn [stat1, stat2] -> - # Skip if octets are nil if is_nil(stat1.if_out_octets) or is_nil(stat2.if_out_octets) do nil else time_diff = stat2.checked_at |> DateTime.diff(stat1.checked_at, :second) |> max(1) - - bps = - ((stat2.if_out_octets - stat1.if_out_octets) * 8 / time_diff) - |> max(0) - |> :erlang.float() - |> Float.round(2) + bps = Towerops.Capacity.calculate_bps(stat1.if_out_octets, stat2.if_out_octets, time_diff) %{ x: DateTime.to_unix(stat2.checked_at, :millisecond), - y: -bps + y: -Float.round(bps, 2) } end end) @@ -1381,16 +1369,15 @@ defmodule ToweropsWeb.GraphLive.Show do previous -> time_diff = current.polled_at |> DateTime.diff(previous.polled_at, :second) |> max(1) - in_bps = calculate_bps(current.if_in_octets, previous.if_in_octets, time_diff) - out_bps = calculate_bps(current.if_out_octets, previous.if_out_octets, time_diff) + in_bps = calculate_live_bps(current.if_in_octets, previous.if_in_octets, time_diff) + out_bps = calculate_live_bps(current.if_out_octets, previous.if_out_octets, time_diff) [%{interface_id: current.interface_id, in_bps: in_bps, out_bps: out_bps}] end end - # Calculate BPS from octets difference - defp calculate_bps(current_octets, previous_octets, time_diff) do + defp calculate_live_bps(current_octets, previous_octets, time_diff) do if current_octets && previous_octets do - max((current_octets - previous_octets) * 8 / time_diff, 0) + Towerops.Capacity.calculate_bps(previous_octets, current_octets, time_diff) else 0.0 end diff --git a/test/towerops/capacity_test.exs b/test/towerops/capacity_test.exs index 45933514..c35e8688 100644 --- a/test/towerops/capacity_test.exs +++ b/test/towerops/capacity_test.exs @@ -83,7 +83,7 @@ defmodule Towerops.CapacityTest do assert_in_delta result.max_bps, 133_333.33, 1.0 end - test "clamps to zero for small counter decreases (device reset or glitch)", %{interface: interface} do + test "applies 32-bit wrap correction for small counter decreases", %{interface: interface} do now = DateTime.utc_now() t1 = DateTime.add(now, -120, :second) t2 = DateTime.add(now, -60, :second) @@ -94,7 +94,9 @@ defmodule Towerops.CapacityTest do checked_at: t1 }) - # Small decrease - treated as reset/glitch, not a 32-bit wrap + # Counter decreased — could be 32-bit wrap on a fast link. + # We always apply 2^32 correction for negative deltas within 32-bit range. + # Corrected delta: (4,294,967,296 - 5,000,000) + 1,000,000 = 4,290,967,296 insert_interface_stat(interface.id, %{ if_in_octets: 1_000_000, if_out_octets: 6_000_000, @@ -103,11 +105,43 @@ defmodule Towerops.CapacityTest do result = Capacity.calculate_throughput(interface.id, DateTime.add(now, -180, :second)) - assert result.in_bps == 0.0 + # 4,290,967,296 * 8 / 60 = 572,128,972.8 bps + assert_in_delta result.in_bps, 572_128_972.8, 1.0 assert_in_delta result.out_bps, 133_333.33, 1.0 end - test "recovers correct rate when 32-bit counter wraps", %{interface: interface} do + test "recovers correct rate when 32-bit counter wraps with prev in lower half of range", %{ + interface: interface + } do + now = DateTime.utc_now() + t1 = DateTime.add(now, -120, :second) + t2 = DateTime.add(now, -60, :second) + + # Counter at 1.36B (lower half of 32-bit range, < 2^31) + # This is the exact scenario from production data + insert_interface_stat(interface.id, %{ + if_in_octets: 1_360_303_152, + if_out_octets: 2_961_644_622, + checked_at: t1 + }) + + # Counter wrapped — high-bandwidth link pushed past 2^32 + insert_interface_stat(interface.id, %{ + if_in_octets: 105_884_342, + if_out_octets: 3_220_277_392, + checked_at: t2 + }) + + result = Capacity.calculate_throughput(interface.id, DateTime.add(now, -180, :second)) + + # in: (4,294,967,296 - 1,360,303,152) + 105,884,342 = 3,040,548,486 bytes + # bps = 3,040,548,486 * 8 / 60 = 405,406,464.8 + assert_in_delta result.in_bps, 405_406_464.8, 1.0 + # out: positive delta, no wrap needed = (3,220,277,392 - 2,961,644,622) * 8 / 60 + assert_in_delta result.out_bps, 34_484_369.33, 1.0 + end + + test "recovers correct rate when 32-bit counter wraps near max", %{interface: interface} do now = DateTime.utc_now() t1 = DateTime.add(now, -120, :second) t2 = DateTime.add(now, -60, :second)