fix agent check cascade and at-risk subscriber double-counting (#79)
## Summary - **`list_checks_for_agent` cascade**: the agent query only matched checks with `agent_token_id` set directly on the check record — checks on devices inheriting their agent via site/org/global default were never sent to the Go agent. Now uses left joins with the full cascade (device assignment → site → org default → global default cloud poller), matching the pattern from `list_agent_polling_targets`. - **At-risk summary double-counting**: `get_impact_summary` called `analyze_device_impact` per down AP, and each returned the full site subscriber count. With 3 APs down at a 147-sub site it showed 441 subs / $31k instead of 147 / $10k. Fixed by deduplicating impact per site. Same fix applied to `calculate_down_impact` in site impact summaries. ## Test plan - [x] 3 new cascade tests for `list_checks_for_agent` (site, org default, global default) - [x] 3 new tests for `get_impact_summary` (single site dedup, multi-site sum, zero case) - [x] All 8,607 existing tests pass - [ ] Deploy to staging, verify At Risk numbers on dashboard - [ ] Verify Go agent receives DNS/HTTP checks via cascade Reviewed-on: graham/towerops-web#79
This commit is contained in:
parent
d6f7dc570a
commit
2b50fc0732
4 changed files with 254 additions and 18 deletions
|
|
@ -123,17 +123,27 @@ defmodule Towerops.Dashboard do
|
|||
def get_impact_summary(organization_id) do
|
||||
down_devices = Devices.list_organization_devices(organization_id, %{"status" => "down"})
|
||||
|
||||
# Compute impact for each down device
|
||||
impacts =
|
||||
Enum.map(down_devices, fn device ->
|
||||
# Deduplicate by site: site-level impact is the same for every AP at a site,
|
||||
# so analyze one representative device per site to avoid counting subscribers N times.
|
||||
{with_site, without_site} = Enum.split_with(down_devices, & &1.site_id)
|
||||
|
||||
site_impacts =
|
||||
with_site
|
||||
|> Enum.group_by(& &1.site_id)
|
||||
|> Enum.map(fn {_site_id, devices} ->
|
||||
ImpactAnalysis.analyze_device_impact(organization_id, hd(devices).id)
|
||||
end)
|
||||
|
||||
no_site_impacts =
|
||||
Enum.map(without_site, fn device ->
|
||||
ImpactAnalysis.analyze_device_impact(organization_id, device.id)
|
||||
end)
|
||||
|
||||
# Aggregate — count unique sites with outages
|
||||
impacts = site_impacts ++ no_site_impacts
|
||||
|
||||
sites_with_outages =
|
||||
down_devices
|
||||
with_site
|
||||
|> Enum.map(& &1.site_id)
|
||||
|> Enum.reject(&is_nil/1)
|
||||
|> Enum.uniq()
|
||||
|> length()
|
||||
|
||||
|
|
@ -266,13 +276,9 @@ defmodule Towerops.Dashboard do
|
|||
defp calculate_down_impact(_organization_id, _down_devices, 0), do: {0, Decimal.new("0")}
|
||||
|
||||
defp calculate_down_impact(organization_id, down_devices, _down_count) do
|
||||
impacts =
|
||||
Enum.map(down_devices, fn device ->
|
||||
ImpactAnalysis.analyze_device_impact(organization_id, device.id)
|
||||
end)
|
||||
|
||||
subs = impacts |> Enum.map(& &1.total_subscribers) |> Enum.sum()
|
||||
mrr = impacts |> Enum.map(& &1.total_mrr) |> Enum.reduce(Decimal.new("0"), &Decimal.add/2)
|
||||
{subs, mrr}
|
||||
# All devices are at the same site, so site-level impact is identical for each.
|
||||
# Use one device to avoid counting site subscribers once per AP.
|
||||
impact = ImpactAnalysis.analyze_device_impact(organization_id, hd(down_devices).id)
|
||||
{impact.total_subscribers, impact.total_mrr}
|
||||
end
|
||||
end
|
||||
|
|
|
|||
|
|
@ -7,6 +7,7 @@ defmodule Towerops.Monitoring do
|
|||
|
||||
import Ecto.Query
|
||||
|
||||
alias Towerops.Agents.AgentAssignment
|
||||
alias Towerops.Devices.Device
|
||||
alias Towerops.Monitoring.Check
|
||||
alias Towerops.Monitoring.CheckResult
|
||||
|
|
@ -50,22 +51,90 @@ defmodule Towerops.Monitoring do
|
|||
end
|
||||
|
||||
@doc """
|
||||
Returns the list of service checks assigned to a specific agent.
|
||||
Returns the list of service checks for devices assigned to a specific agent.
|
||||
|
||||
Only returns http/tcp/dns checks (service checks that agents can execute).
|
||||
Uses the full agent assignment cascade (device → site → org → global default)
|
||||
to find checks, not just the check's own agent_token_id field.
|
||||
Only returns http/tcp/dns/ssl checks (service checks that agents can execute).
|
||||
"""
|
||||
def list_checks_for_agent(agent_token_id, opts \\ []) do
|
||||
global_default = Towerops.Settings.get_global_default_cloud_poller()
|
||||
is_global_default = agent_token_id == global_default
|
||||
|
||||
query =
|
||||
from c in Check,
|
||||
where: c.agent_token_id == ^agent_token_id,
|
||||
left_join: d in assoc(c, :device),
|
||||
left_join: s in assoc(d, :site),
|
||||
left_join: o in assoc(d, :organization),
|
||||
left_join: aa in AgentAssignment,
|
||||
on: aa.device_id == d.id and aa.enabled == true,
|
||||
left_join: disabled in AgentAssignment,
|
||||
on:
|
||||
disabled.device_id == d.id and disabled.agent_token_id == ^agent_token_id and
|
||||
disabled.enabled == false,
|
||||
where: c.check_type in ["http", "tcp", "dns", "ssl"],
|
||||
order_by: [asc: c.name]
|
||||
order_by: [asc: c.name],
|
||||
distinct: c.id
|
||||
|
||||
query
|
||||
|> where_check_agent_matches(agent_token_id, is_global_default)
|
||||
|> maybe_filter_by_enabled(opts[:enabled])
|
||||
|> Repo.all()
|
||||
end
|
||||
|
||||
# Builds the WHERE clause for matching checks to an agent via the full cascade:
|
||||
# direct check assignment → device assignment → site → org default → global default
|
||||
defp where_check_agent_matches(query, agent_token_id, is_global_default) do
|
||||
condition =
|
||||
[c]
|
||||
|> dynamic(c.agent_token_id == ^agent_token_id)
|
||||
|> or_device_assignment(agent_token_id)
|
||||
|> or_site_assignment(agent_token_id)
|
||||
|> or_org_default(agent_token_id)
|
||||
|> or_global_default(is_global_default)
|
||||
|
||||
from q in query, where: ^condition
|
||||
end
|
||||
|
||||
defp or_device_assignment(condition, agent_token_id) do
|
||||
dynamic(
|
||||
[c, d, s, o, aa, disabled],
|
||||
^condition or
|
||||
(not is_nil(d.id) and aa.agent_token_id == ^agent_token_id and is_nil(disabled.id))
|
||||
)
|
||||
end
|
||||
|
||||
defp or_site_assignment(condition, agent_token_id) do
|
||||
dynamic(
|
||||
[c, d, s, o, aa, disabled],
|
||||
^condition or
|
||||
(not is_nil(d.id) and is_nil(aa.agent_token_id) and is_nil(disabled.id) and
|
||||
not is_nil(s.id) and s.agent_token_id == ^agent_token_id)
|
||||
)
|
||||
end
|
||||
|
||||
defp or_org_default(condition, agent_token_id) do
|
||||
dynamic(
|
||||
[c, d, s, o, aa, disabled],
|
||||
^condition or
|
||||
(not is_nil(d.id) and is_nil(aa.agent_token_id) and is_nil(disabled.id) and
|
||||
(is_nil(s.id) or is_nil(s.agent_token_id)) and
|
||||
o.default_agent_token_id == ^agent_token_id)
|
||||
)
|
||||
end
|
||||
|
||||
defp or_global_default(condition, false), do: condition
|
||||
|
||||
defp or_global_default(condition, true) do
|
||||
dynamic(
|
||||
[c, d, s, o, aa, disabled],
|
||||
^condition or
|
||||
(not is_nil(d.id) and is_nil(aa.agent_token_id) and is_nil(disabled.id) and
|
||||
(is_nil(s.id) or is_nil(s.agent_token_id)) and
|
||||
is_nil(o.default_agent_token_id))
|
||||
)
|
||||
end
|
||||
|
||||
@doc """
|
||||
Gets a single check.
|
||||
"""
|
||||
|
|
|
|||
|
|
@ -6,6 +6,7 @@ defmodule Towerops.DashboardTest do
|
|||
import Towerops.OrganizationsFixtures
|
||||
|
||||
alias Towerops.Dashboard
|
||||
alias Towerops.Gaiia
|
||||
alias Towerops.Integrations
|
||||
|
||||
setup do
|
||||
|
|
@ -143,6 +144,116 @@ defmodule Towerops.DashboardTest do
|
|||
end
|
||||
end
|
||||
|
||||
describe "get_impact_summary/1" do
|
||||
test "does not multiply site subscribers by number of down APs", %{org: org} do
|
||||
site = site_fixture(org.id)
|
||||
|
||||
{:ok, _network_site} =
|
||||
Gaiia.upsert_network_site(org.id, %{
|
||||
gaiia_id: "gaiia-site-1",
|
||||
name: "Tower 3",
|
||||
account_count: 147,
|
||||
total_mrr: Decimal.new("10479.00")
|
||||
})
|
||||
|
||||
Gaiia.update_network_site_mapping(
|
||||
Gaiia.get_network_site(org.id, "gaiia-site-1"),
|
||||
%{site_id: site.id}
|
||||
)
|
||||
|
||||
# Create 3 APs at this site, all down
|
||||
for i <- 1..3 do
|
||||
device =
|
||||
device_fixture(%{
|
||||
organization_id: org.id,
|
||||
site_id: site.id,
|
||||
name: "AP #{i}"
|
||||
})
|
||||
|
||||
Towerops.Devices.update_device(device, %{status: :down})
|
||||
end
|
||||
|
||||
summary = Dashboard.get_impact_summary(org.id)
|
||||
|
||||
# Should be 147 subscribers (site total), NOT 147 * 3 = 441
|
||||
assert summary.subscribers_affected == 147
|
||||
assert Decimal.equal?(summary.mrr_at_risk, Decimal.new("10479.00"))
|
||||
assert summary.down_device_count == 3
|
||||
assert summary.sites_with_outages == 1
|
||||
end
|
||||
|
||||
test "sums subscribers across different sites correctly", %{org: org} do
|
||||
# Site 1 with 100 subscribers, 2 APs down
|
||||
site1 = site_fixture(org.id, %{name: "Tower A"})
|
||||
|
||||
{:ok, _} =
|
||||
Gaiia.upsert_network_site(org.id, %{
|
||||
gaiia_id: "site-a",
|
||||
name: "Tower A",
|
||||
account_count: 100,
|
||||
total_mrr: Decimal.new("5000.00")
|
||||
})
|
||||
|
||||
Gaiia.update_network_site_mapping(
|
||||
Gaiia.get_network_site(org.id, "site-a"),
|
||||
%{site_id: site1.id}
|
||||
)
|
||||
|
||||
for i <- 1..2 do
|
||||
device =
|
||||
device_fixture(%{
|
||||
organization_id: org.id,
|
||||
site_id: site1.id,
|
||||
name: "Site1 AP #{i}"
|
||||
})
|
||||
|
||||
Towerops.Devices.update_device(device, %{status: :down})
|
||||
end
|
||||
|
||||
# Site 2 with 50 subscribers, 1 AP down
|
||||
site2 = site_fixture(org.id, %{name: "Tower B"})
|
||||
|
||||
{:ok, _} =
|
||||
Gaiia.upsert_network_site(org.id, %{
|
||||
gaiia_id: "site-b",
|
||||
name: "Tower B",
|
||||
account_count: 50,
|
||||
total_mrr: Decimal.new("2500.00")
|
||||
})
|
||||
|
||||
Gaiia.update_network_site_mapping(
|
||||
Gaiia.get_network_site(org.id, "site-b"),
|
||||
%{site_id: site2.id}
|
||||
)
|
||||
|
||||
device =
|
||||
device_fixture(%{
|
||||
organization_id: org.id,
|
||||
site_id: site2.id,
|
||||
name: "Site2 AP 1"
|
||||
})
|
||||
|
||||
Towerops.Devices.update_device(device, %{status: :down})
|
||||
|
||||
summary = Dashboard.get_impact_summary(org.id)
|
||||
|
||||
# 100 + 50 = 150 total subscribers across 2 sites
|
||||
assert summary.subscribers_affected == 150
|
||||
assert Decimal.equal?(summary.mrr_at_risk, Decimal.new("7500.00"))
|
||||
assert summary.down_device_count == 3
|
||||
assert summary.sites_with_outages == 2
|
||||
end
|
||||
|
||||
test "returns zero impact when no devices are down", %{org: org} do
|
||||
summary = Dashboard.get_impact_summary(org.id)
|
||||
|
||||
assert summary.subscribers_affected == 0
|
||||
assert Decimal.equal?(summary.mrr_at_risk, Decimal.new("0"))
|
||||
assert summary.down_device_count == 0
|
||||
assert summary.sites_with_outages == 0
|
||||
end
|
||||
end
|
||||
|
||||
describe "list_site_summaries/1" do
|
||||
test "returns enriched site data", %{org: org} do
|
||||
{:ok, site} =
|
||||
|
|
|
|||
|
|
@ -185,6 +185,56 @@ defmodule Towerops.MonitoringTest do
|
|||
|
||||
assert Monitoring.list_checks_for_agent(agent2.id) == []
|
||||
end
|
||||
|
||||
test "returns checks for devices inheriting agent via site", %{organization: org, device: device, site: site} do
|
||||
{:ok, agent_token, _} = agent_token_fixture(org.id)
|
||||
Towerops.Sites.update_site(site, %{agent_token_id: agent_token.id})
|
||||
|
||||
{:ok, dns_check} =
|
||||
Monitoring.create_check(
|
||||
valid_check_attrs(org.id, %{
|
||||
name: "DNS Check",
|
||||
check_type: "dns",
|
||||
config: %{"hostname" => "example.com", "record_type" => "A"},
|
||||
device_id: device.id
|
||||
})
|
||||
)
|
||||
|
||||
checks = Monitoring.list_checks_for_agent(agent_token.id, enabled: true)
|
||||
ids = Enum.map(checks, & &1.id)
|
||||
assert dns_check.id in ids
|
||||
end
|
||||
|
||||
test "returns checks for devices inheriting agent via org default", %{organization: org, device: device} do
|
||||
{:ok, agent_token, _} = agent_token_fixture(org.id)
|
||||
Towerops.Organizations.update_organization(org, %{default_agent_token_id: agent_token.id})
|
||||
|
||||
{:ok, http_check} =
|
||||
Monitoring.create_check(valid_check_attrs(org.id, %{name: "HTTP Check", device_id: device.id}))
|
||||
|
||||
checks = Monitoring.list_checks_for_agent(agent_token.id, enabled: true)
|
||||
ids = Enum.map(checks, & &1.id)
|
||||
assert http_check.id in ids
|
||||
end
|
||||
|
||||
test "returns checks for devices inheriting global default cloud poller", %{organization: org, device: device} do
|
||||
{:ok, cloud_poller, _token} = Towerops.Agents.create_cloud_poller("Global Cloud Poller")
|
||||
Towerops.Settings.set_global_default_cloud_poller(cloud_poller.id)
|
||||
|
||||
{:ok, dns_check} =
|
||||
Monitoring.create_check(
|
||||
valid_check_attrs(org.id, %{
|
||||
name: "DNS Check",
|
||||
check_type: "dns",
|
||||
config: %{"hostname" => "example.com", "record_type" => "A"},
|
||||
device_id: device.id
|
||||
})
|
||||
)
|
||||
|
||||
checks = Monitoring.list_checks_for_agent(cloud_poller.id, enabled: true)
|
||||
ids = Enum.map(checks, & &1.id)
|
||||
assert dns_check.id in ids
|
||||
end
|
||||
end
|
||||
|
||||
describe "get_check/1 and get_check!/1" do
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue