From 5aabf8fa25e9f35d687e873885aaa57812d2a1e8 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Fri, 4 Sep 2026 16:22:10 +0100 Subject: [PATCH] fix(sales-dashboard): Correct understated API usage on the organisation chart The sales dashboard read API usage through its own Influx entry point, `get_event_list_for_organisation`, which was the only usage reader without a `group()` before `aggregateWindow`. Influx aggregates per series, so each label combination produced its own daily records, and the Python then appended them into one flat list per resource rather than summing them per day. Chart.js plotted the leading slice of that list, so a single client application's traffic was shown as the whole organisation's usage, with the x-axis labels taken from the first series only. Delete that function and have the sales dashboard call `get_usage_data`, the same entry point the customer-facing app uses, reshaping the result for Chart.js in `map_usage_data_to_daily_totals`. Both charts now share one query, so the grouping cannot go missing in one caller and not the other. Co-Authored-By: Claude Opus 5 --- api/app_analytics/influxdb_wrapper.py | 41 ---------- api/app_analytics/mappers.py | 33 ++++++++ api/sales_dashboard/views.py | 16 ++-- ...est_unit_app_analytics_influxdb_wrapper.py | 71 ----------------- .../test_unit_app_analytics_mappers.py | 77 +++++++++++++++++++ .../test_unit_sales_dashboard_views.py | 37 ++++++--- 6 files changed, 143 insertions(+), 132 deletions(-) diff --git a/api/app_analytics/influxdb_wrapper.py b/api/app_analytics/influxdb_wrapper.py index 2ad568a57708..1ad933b7258b 100644 --- a/api/app_analytics/influxdb_wrapper.py +++ b/api/app_analytics/influxdb_wrapper.py @@ -200,47 +200,6 @@ def get_events_for_organisation( return total -def get_event_list_for_organisation( - organisation_id: int, - date_start: datetime | None = None, - date_stop: datetime | None = None, -) -> tuple[dict[str, list[int]], list[str]]: - """ - Query influx db for usage for given organisation id - - :param organisation_id: an id of the organisation to get usage for - - :return: a number of request counts for organisation in chart.js scheme - """ - now = timezone.now() - if date_start is None: - date_start = now - timedelta(days=30) - - if date_stop is None: - date_stop = now - - results = InfluxDBWrapper.influx_query_manager( - filters=( - '|> filter(fn:(r) => r._measurement == "api_call") ' - f'|> filter(fn: (r) => r["organisation_id"] == "{organisation_id}")' - ), - extra='|> aggregateWindow(every: 24h, fn: sum, timeSrc: "_start")', - date_start=date_start, - date_stop=date_stop, - ) - dataset = defaultdict(list) - labels = [] # type: ignore[var-annotated] - - date_difference = date_stop - date_start - required_records = date_difference.days + 1 - for result in results: - for record in result.records: - dataset[record["resource"]].append(record["_value"]) - if len(labels) != required_records: - labels.append(record.values["_time"].strftime("%Y-%m-%d")) - return dataset, labels - - def get_multiple_event_list_for_organisation( organisation_id: int, project_id: int | None = None, diff --git a/api/app_analytics/mappers.py b/api/app_analytics/mappers.py index 998bba797bfd..ce5696ac9f93 100644 --- a/api/app_analytics/mappers.py +++ b/api/app_analytics/mappers.py @@ -153,6 +153,39 @@ def map_flux_tables_to_usage_data( return list(data_by_key.values()) +USAGE_DATA_RESOURCE_ATTRIBUTES: tuple[str, ...] = tuple( + column_name for resource in Resource if (column_name := resource.column_name) +) + + +def map_usage_data_to_daily_totals( + usage_data: Iterable[UsageData], +) -> tuple[list[str], dict[str, list[int]]]: + """ + Collapse usage data into a single total per day for each resource. + + Usage data holds a row per day *and* labels combination, so a day with + traffic from more than one client application appears more than once. Any + caller wanting whole-organisation totals has to sum across those rows. + + :return: the days covered, and a list of totals per resource aligned to them + """ + totals_by_day: dict[str, dict[str, int]] = {} + for data in usage_data: + totals = totals_by_day.setdefault( + str(data.day), + dict.fromkeys(USAGE_DATA_RESOURCE_ATTRIBUTES, 0), + ) + for resource_attr in USAGE_DATA_RESOURCE_ATTRIBUTES: + totals[resource_attr] += getattr(data, resource_attr) + + days = sorted(totals_by_day) + return days, { + resource_attr: [totals_by_day[day][resource_attr] for day in days] + for resource_attr in USAGE_DATA_RESOURCE_ATTRIBUTES + } + + def map_flux_tables_to_feature_evaluation_data( flux_tables: list[FluxTable], ) -> list[FeatureEvaluationData]: diff --git a/api/sales_dashboard/views.py b/api/sales_dashboard/views.py index 19cbc532c9af..5f84ebe72b32 100644 --- a/api/sales_dashboard/views.py +++ b/api/sales_dashboard/views.py @@ -23,9 +23,10 @@ from django.views.generic import ListView, TemplateView from app_analytics.influxdb_wrapper import ( - get_event_list_for_organisation, get_events_for_organisation, + get_usage_data, ) +from app_analytics.mappers import map_usage_data_to_daily_totals from core.helpers import get_current_site_url from environments.dynamodb.migrator import IdentityMigrator from environments.identities.models import Identity @@ -227,15 +228,14 @@ def organisation_info(request: HttpRequest, organisation_id: int) -> HttpRespons assert date_range.endswith("d") now = timezone.now() date_start = now - timedelta(days=int(date_range[:-1])) - event_list, labels = get_event_list_for_organisation( - organisation_id, date_start + labels, totals = map_usage_data_to_daily_totals( + get_usage_data(organisation_id, date_start=date_start, date_stop=now) ) - context["event_list"] = event_list - context["traits"] = mark_safe(json.dumps(event_list["traits"])) - context["identities"] = mark_safe(json.dumps(event_list["identities"])) - context["flags"] = mark_safe(json.dumps(event_list["flags"])) + context["traits"] = mark_safe(json.dumps(totals["traits"])) + context["identities"] = mark_safe(json.dumps(totals["identities"])) + context["flags"] = mark_safe(json.dumps(totals["flags"])) context["environment_documents"] = mark_safe( - json.dumps(event_list["environment-document"]) + json.dumps(totals["environment_document"]) ) context["labels"] = mark_safe(json.dumps(labels)) diff --git a/api/tests/unit/app_analytics/test_unit_app_analytics_influxdb_wrapper.py b/api/tests/unit/app_analytics/test_unit_app_analytics_influxdb_wrapper.py index f374c6f29df0..2d02da2061fe 100644 --- a/api/tests/unit/app_analytics/test_unit_app_analytics_influxdb_wrapper.py +++ b/api/tests/unit/app_analytics/test_unit_app_analytics_influxdb_wrapper.py @@ -17,7 +17,6 @@ InfluxDBWrapper, build_filter_string, get_current_api_usage, - get_event_list_for_organisation, get_events_for_organisation, get_feature_evaluation_data, get_multiple_event_list_for_feature, @@ -25,7 +24,6 @@ get_top_organisations, get_usage_data, ) -from organisations.models import Organisation # Given org_id = 123 @@ -127,29 +125,6 @@ def test_get_events_for_organisation__default_params__calls_query_api_with_expec assert call[2]["query"].replace(" ", "").replace("\n", "") == expected_query -@pytest.mark.freeze_time("2023-01-19T09:09:47.325132+00:00") -def test_get_event_list_for_organisation__default_params__calls_query_api_with_expected_query( - mock_influxdb_client: MagicMock, -) -> None: - # Given - query = ( - f'from(bucket:"{read_bucket}") ' - f"|> range(start: 2022-12-20T09:09:47.325132+00:00, stop: 2023-01-19T09:09:47.325132+00:00) " - f'|> filter(fn:(r) => r._measurement == "api_call") ' - f'|> filter(fn: (r) => r["organisation_id"] == "{org_id}") ' - f'|> drop(columns: ["organisation", "organisation_id", "type", "project", ' - f'"project_id", "environment", "environment_id", "host"]) ' - f'|> aggregateWindow(every: 24h, fn: sum, timeSrc: "_start")' - ) - mock_query_api = mock_influxdb_client.query_api.return_value - - # When - get_event_list_for_organisation(org_id) - - # Then - mock_query_api.query.assert_called_once_with(org=influx_org, query=query) - - @pytest.mark.parametrize( "project_id, environment_id, expected_filters", ( @@ -440,52 +415,6 @@ def test_get_feature_evaluation_data__default_params__calls_get_multiple_event_l ) -@pytest.mark.freeze_time("2023-01-19T09:09:47.325132+00:00") -def test_get_event_list_for_organisation__date_stop_set__returns_grouped_data( - mocker: MockerFixture, - organisation: Organisation, -) -> None: - # Given - - now = timezone.now() - one_day_ago = now - timedelta(days=1) - two_days_ago = now - timedelta(days=2) - date_stop = now - - record_mock1 = mock.MagicMock() - record_mock1.__getitem__.side_effect = lambda key: { - "resource": "resource23", - "_value": 23, - }.get(key) - record_mock1.values = {"_time": one_day_ago} - - record_mock2 = mock.MagicMock() - record_mock2.__getitem__.side_effect = lambda key: { - "resource": "resource24", - "_value": 24, - }.get(key) - record_mock2.values = {"_time": two_days_ago} - - result = mock.MagicMock() - result.records = [record_mock1, record_mock2] - - influx_mock = mocker.patch( - "app_analytics.influxdb_wrapper.InfluxDBWrapper.influx_query_manager" - ) - - influx_mock.return_value = [result] - - # When - dataset, labels = get_event_list_for_organisation( - organisation_id=organisation.id, - date_stop=date_stop, - ) - - # Then - assert dataset == {"resource23": [23], "resource24": [24]} - assert labels == ["2023-01-18", "2023-01-17"] - - @pytest.mark.freeze_time("2023-01-19T09:09:47.325132+00:00") @pytest.mark.parametrize("limit", ["10", ""]) def test_get_top_organisations__with_records__returns_organisation_totals( diff --git a/api/tests/unit/app_analytics/test_unit_app_analytics_mappers.py b/api/tests/unit/app_analytics/test_unit_app_analytics_mappers.py index cb65f647749f..b9e915f7eb5c 100644 --- a/api/tests/unit/app_analytics/test_unit_app_analytics_mappers.py +++ b/api/tests/unit/app_analytics/test_unit_app_analytics_mappers.py @@ -8,6 +8,7 @@ map_flux_tables_to_feature_evaluation_data, map_flux_tables_to_usage_data, map_influx_record_values_to_labels, + map_usage_data_to_daily_totals, ) @@ -107,3 +108,79 @@ def test_map_influx_record_values_to_labels__various_user_agents__returns_expect # Then assert result == expected + + +def test_map_usage_data_to_daily_totals__multiple_labels_per_day__sums_across_labels() -> ( + None +): + """ + Usage data holds a row per day and labels combination, so a day with + traffic from several client applications arrives as several rows. Totals + must sum across them, or a single client is reported as the whole + organisation's usage. + """ + # Given + usage_data = [ + UsageData( + day=date(2026, 6, 27), + flags=3_158, + labels={"client_application_name": "small-app"}, + ), + UsageData( + day=date(2026, 6, 27), + flags=238_574, + identities=10, + labels={"client_application_name": "busy-app"}, + ), + UsageData( + day=date(2026, 6, 28), + flags=240_000, + traits=5, + environment_document=1, + labels={"client_application_name": "busy-app"}, + ), + ] + + # When + labels, totals = map_usage_data_to_daily_totals(usage_data) + + # Then + assert labels == ["2026-06-27", "2026-06-28"] + assert totals == { + "flags": [241_732, 240_000], + "identities": [10, 0], + "traits": [0, 5], + "environment_document": [0, 1], + } + + +def test_map_usage_data_to_daily_totals__unordered_days__returns_days_in_order() -> ( + None +): + # Given + usage_data = [ + UsageData(day=date(2026, 6, 28), flags=2), + UsageData(day=date(2026, 6, 26), flags=1), + UsageData(day=date(2026, 6, 27), flags=3), + ] + + # When + labels, totals = map_usage_data_to_daily_totals(usage_data) + + # Then + assert labels == ["2026-06-26", "2026-06-27", "2026-06-28"] + assert totals["flags"] == [1, 3, 2] + + +def test_map_usage_data_to_daily_totals__no_usage_data__returns_empty_series() -> None: + # Given / When + labels, totals = map_usage_data_to_daily_totals([]) + + # Then + assert labels == [] + assert totals == { + "flags": [], + "identities": [], + "traits": [], + "environment_document": [], + } diff --git a/api/tests/unit/sales_dashboard/test_unit_sales_dashboard_views.py b/api/tests/unit/sales_dashboard/test_unit_sales_dashboard_views.py index 5967f0108b2f..89637acd6947 100644 --- a/api/tests/unit/sales_dashboard/test_unit_sales_dashboard_views.py +++ b/api/tests/unit/sales_dashboard/test_unit_sales_dashboard_views.py @@ -1,4 +1,4 @@ -from datetime import timedelta +from datetime import date, timedelta import pytest from django.test import Client, RequestFactory @@ -8,6 +8,7 @@ from pytest_mock import MockerFixture from rest_framework.test import APIClient +from app_analytics.dataclasses import UsageData from features.versioning.constants import DEFAULT_VERSION_LIMIT_DAYS from organisations.models import ( Organisation, @@ -65,23 +66,35 @@ def test_get_organisation_info__valid_organisation__returns_event_list( url = reverse("sales_dashboard:organisation_info", args=[organisation.id]) - event_list_mock = mocker.patch( - "sales_dashboard.views.get_event_list_for_organisation" - ) - event_list_mock.return_value = ( - {"traits": [], "identities": [], "flags": [], "environment-document": []}, - ["label1", "label2"], - ) + usage_data_mock = mocker.patch("sales_dashboard.views.get_usage_data") + usage_data_mock.return_value = [ + UsageData( + day=date(2026, 6, 27), + flags=3_158, + labels={"client_application_name": "small-app"}, + ), + UsageData( + day=date(2026, 6, 27), + flags=238_574, + labels={"client_application_name": "busy-app"}, + ), + ] mocker.patch("sales_dashboard.views.get_events_for_organisation") # When response = superuser_client.get(url) # Then - assert "label1" in str(response.content) - assert "label2" in str(response.content) - date_start = timezone.now() - timedelta(days=180) - event_list_mock.assert_called_once_with(organisation.id, date_start) + content = str(response.content) + assert "2026-06-27" in content + # Both client applications' usage for the day, not just one of them. + assert "241732" in content + now = timezone.now() + usage_data_mock.assert_called_once_with( + organisation.id, + date_start=now - timedelta(days=180), + date_stop=now, + ) def test_list_organisations__search_by_name__returns_matching_organisation(