Extract shared usage-metrics read path (#3905, PR 2) - #4134
Extract shared usage-metrics read path (#3905, PR 2)#4134barry47products wants to merge 16 commits into
Conversation
CodeRabbit's review of #4132 asked for both team_id and tag__team_id on the CustomTaggedItem tag-exists predicates; the earlier commit added only team_id. This closes the gap everywhere the shape is duplicated: chat_tag_exists_pair (filters.py), the four inline predicates in filtered_querysets (dashboard_querysets.py), and both predicates in the cost read path's _scoped_records (reporting.py). team_id alone catches a link row recorded under another team; tag__team_id catches a different shape - a locally-recorded link whose Tag itself belongs to another team. That shape is unreachable via the dashboard today (tag_ids are validated against a team-scoped ModelMultipleChoiceField), but chat_tag_exists_pair is a shared helper for API surfaces where that validation isn't in the path. The cost-tracking site keeps its own inline pair rather than importing chat_tag_exists_pair - its outer ref (session__chat_id) differs from the usage_metrics sites (chat_id).
… window test Regression coverage for the tag__team_id predicate: mirrors the existing cross-team-link tests with the other inconsistent-link shape - a CustomTaggedItem row with a LOCAL team_id whose tag belongs to a FOREIGN team. Added to test_dashboard_querysets.py (sessions/experiments/ participants, both chat- and message-targeted to exercise the _on_msg predicates too), test_metrics.py, and test_reporting_filters.py (chat- and message-targeted, covering both cost-path predicates). Also fixes test_window_is_half_open: asserting only total == 2 doesn't discriminate [start, end) from the (start, end] mutant, since a symmetric interior message cancels out and both windows give 2. Now asserts on which messages survived the window.
An independent mutation matrix on team_id=team.id found it was only 4/8 covered: every message-targeted test used a foreign tag, and every foreign-team_id test targeted a chat, so no test crossed both axes - the four _on_msg subqueries could lose team_id with the suite green. Adds the missing cross: a CustomTaggedItem with a FOREIGN link team_id but a LOCAL tag, attached to a MESSAGE, to test_dashboard_querysets.py (covering tag_on_msg/exp_tag_on_msg/part_tag_on_msg in one test), test_metrics.py, and test_reporting_filters.py - each with a positive control confirming the same shape still matches under a local team_id.
The messages leg of filtered_querysets used a plain tags__id__in M2M filter, the only one of the module's 9 tag-link sites that carried neither the team_id nor tag__team_id predicate. A cross-team CustomTaggedItem link on a local message could pull that message into the messages queryset while the same link correctly excluded the session, experiment, and participant from their querysets. Replace it with a team-scoped Exists against CustomTaggedItem for the message's own tags, matching the shape of the other 8 sites. Keeps message-only semantics (a tag on the chat rather than the message itself does not qualify the chat's messages here) rather than widening to the chat-or-message match the other legs use.
- dashboard_querysets.py's module docstring claimed to reproduce the dashboard's current semantics without exception; note the one deliberate divergence (tag links scoped to the reading team, per the CodeRabbit finding on PR #4132). - filters.py cited docs/issues/3905-usage-metrics-convergence, a path that only exists in this local clone (excluded via .git/info/exclude) and not in the repository; point at issue #3905 instead, which line 1 already cites. - UsageFilters.include_archived's docstring described it as applying to experiment enumeration and made explicit, but nothing consults it: filtered_querysets hardcodes is_archived=False, working_version=None, and the v2 API enumerates with get_all(). Rewrite the docstring to state this plainly so a follow-up wiring the flag into filtered_querysets doesn't assume the True default is safe there.
get_tag_analytics_data read CustomTaggedItem rows with no team_id or tag__team_id predicate, relying only on the already-team-scoped message queryset. A CustomTaggedItem row recorded under another team, pointing at one of this team's own messages, was picked up, and the foreign tag's user-authored name leaked into this team's dashboard breakdown while inflating the total. Add both predicates to match the 9 other tag-link sites, and add the first regression coverage for this method.
There was a problem hiding this comment.
Code Health Improved
(1 files improve in Code Health)
Gates Failed
Enforce advisory code health rules
(2 files with Large Method, Excess Number of Function Arguments, Missing Arguments Abstractions)
Our agent can fix these. Install it.
Gates Passed
3 Quality Gates Passed
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| dashboard_querysets.py | 2 advisory rules | 8.96 | Suppress |
| metrics.py | 1 advisory rule | 9.69 | Suppress |
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| services.py | 9.29 → 10.00 | Large Method |
Quality Gate Profile: Clean Code Collective
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
| def filtered_querysets( | ||
| team: Team, | ||
| *, | ||
| start_date: datetime | None = None, | ||
| end_date: datetime | None = None, | ||
| experiment_ids: list[int] | None = None, | ||
| platform_names: list[str] | None = None, | ||
| participant_ids: list[int] | None = None, | ||
| tag_ids: list[int] | None = None, | ||
| ) -> dict[str, Any]: | ||
| """Base querysets with the dashboard's common filters applied. Returns | ||
| `experiments`, `sessions`, `messages`, `participants` querysets plus the | ||
| resolved `start_date`/`end_date` (defaulting to the last 30 days).""" | ||
|
|
||
| if not end_date: | ||
| end_date = timezone.now() | ||
| if not start_date: | ||
| start_date = end_date - timedelta(days=30) | ||
|
|
||
| base_filters = {"created_at__gte": start_date, "created_at__lte": end_date} | ||
|
|
||
| experiments = Experiment.objects.filter(team=team, is_archived=False, working_version=None) | ||
| # Use Exists() to avoid join+distinct - prevents row explosion upfront for better performance | ||
| msg_exists = Exists( | ||
| ChatMessage.objects.filter( | ||
| chat=OuterRef("chat"), | ||
| created_at__gte=start_date, | ||
| created_at__lte=end_date, | ||
| ) | ||
| ) | ||
| sessions = ( | ||
| ExperimentSession.objects.filter(team=team) | ||
| .exclude(experiment_channel__platform=ChannelPlatform.EVALUATIONS) | ||
| .annotate(_has_msgs=msg_exists) | ||
| .filter(_has_msgs=True) | ||
| ) | ||
| messages = ChatMessage.objects.filter(chat__team=team, **base_filters).exclude( | ||
| chat__experiment_session__platform=ChannelPlatform.EVALUATIONS | ||
| ) | ||
| participants = Participant.objects.filter(team=team).exclude(platform=ChannelPlatform.EVALUATIONS) | ||
|
|
||
| if experiment_ids: | ||
| experiments = experiments.filter(id__in=experiment_ids) | ||
| sessions = sessions.filter(experiment_id__in=experiment_ids) | ||
| messages = messages.filter(chat__experiment_session__experiment_id__in=experiment_ids) | ||
| participants = participants.filter(experimentsession__experiment_id__in=experiment_ids).distinct() | ||
|
|
||
| if platform_names: | ||
| global_platforms = ChannelPlatform.team_global_platforms() | ||
| if not any(p in global_platforms for p in platform_names): | ||
| # only filter experiments if we're filtering by non-global platforms since all experiments | ||
| # will match the global platforms | ||
| experiments = experiments.filter( | ||
| Exists( | ||
| ExperimentChannel.objects.filter( | ||
| experiment=OuterRef("pk"), | ||
| platform__in=platform_names, | ||
| deleted=False, | ||
| ) | ||
| ) | ||
| ) | ||
| sessions = sessions.filter(platform__in=platform_names) | ||
| messages = messages.filter(chat__experiment_session__platform__in=platform_names) | ||
| participants = participants.filter(platform__in=platform_names) | ||
|
|
||
| if participant_ids: | ||
| experiments = experiments.filter(sessions__participant__id__in=participant_ids).distinct() | ||
| sessions = sessions.filter(participant__id__in=participant_ids) | ||
| messages = messages.filter(chat__experiment_session__participant__id__in=participant_ids) | ||
| participants = participants.filter(id__in=participant_ids) | ||
|
|
||
| if tag_ids: | ||
| chat_content_type = ContentType.objects.get_for_model(Chat) | ||
| message_content_type = ContentType.objects.get_for_model(ChatMessage) | ||
|
|
||
| # Sessions: chat or any message in it carries the tag (both the link row and its tag | ||
| # must belong to the reading team) | ||
| tag_on_chat, tag_on_msg = chat_tag_exists_pair(team, tag_ids, "chat_id") | ||
| sessions = sessions.annotate(_tchat=tag_on_chat, _tmsg=tag_on_msg).filter(Q(_tchat=True) | Q(_tmsg=True)) | ||
|
|
||
| # Experiments: any session's chat or messages carry the tag (both the link row and | ||
| # its tag must belong to the reading team) | ||
| exp_tag_on_chat = Exists( | ||
| CustomTaggedItem.objects.filter( | ||
| team_id=team.id, | ||
| tag__team_id=team.id, | ||
| content_type=chat_content_type, | ||
| object_id__in=Subquery( | ||
| Chat.objects.filter(experiment_session__experiment=OuterRef(OuterRef("id"))).values("id") | ||
| ), | ||
| tag_id__in=tag_ids, | ||
| ) | ||
| ) | ||
| exp_tag_on_msg = Exists( | ||
| CustomTaggedItem.objects.filter( | ||
| team_id=team.id, | ||
| tag__team_id=team.id, | ||
| content_type=message_content_type, | ||
| object_id__in=Subquery( | ||
| ChatMessage.objects.filter(chat__experiment_session__experiment=OuterRef(OuterRef("id"))).values( | ||
| "id" | ||
| ) | ||
| ), | ||
| tag_id__in=tag_ids, | ||
| ) | ||
| ) | ||
| experiments = experiments.annotate(_exp_tchat=exp_tag_on_chat, _exp_tmsg=exp_tag_on_msg).filter( | ||
| Q(_exp_tchat=True) | Q(_exp_tmsg=True) | ||
| ) | ||
|
|
||
| # Participants: any of their sessions' chats or messages carry the tag (both the | ||
| # link row and its tag must belong to the reading team) | ||
| part_tag_on_chat = Exists( | ||
| CustomTaggedItem.objects.filter( | ||
| team_id=team.id, | ||
| tag__team_id=team.id, | ||
| content_type=chat_content_type, | ||
| object_id__in=Subquery( | ||
| Chat.objects.filter(experiment_session__participant=OuterRef(OuterRef("id"))).values("id") | ||
| ), | ||
| tag_id__in=tag_ids, | ||
| ) | ||
| ) | ||
| part_tag_on_msg = Exists( | ||
| CustomTaggedItem.objects.filter( | ||
| team_id=team.id, | ||
| tag__team_id=team.id, | ||
| content_type=message_content_type, | ||
| object_id__in=Subquery( | ||
| ChatMessage.objects.filter(chat__experiment_session__participant=OuterRef(OuterRef("id"))).values( | ||
| "id" | ||
| ) | ||
| ), | ||
| tag_id__in=tag_ids, | ||
| ) | ||
| ) | ||
| participants = participants.annotate(_part_tchat=part_tag_on_chat, _part_tmsg=part_tag_on_msg).filter( | ||
| Q(_part_tchat=True) | Q(_part_tmsg=True) | ||
| ) | ||
|
|
||
| # Messages: the message's own tags carry the tag (both the link row and its tag must | ||
| # belong to the reading team). Message-only match - a tag on the chat (rather than the | ||
| # message itself) does not pull the chat's messages in here; that broader chat-or-message | ||
| # match is what the sessions/experiments/participants legs above use via | ||
| # `chat_tag_exists_pair`, not this one. | ||
| msg_tag_on_msg = Exists( | ||
| CustomTaggedItem.objects.filter( | ||
| team_id=team.id, | ||
| tag__team_id=team.id, | ||
| content_type=message_content_type, | ||
| object_id=OuterRef("id"), | ||
| tag_id__in=tag_ids, | ||
| ) | ||
| ) | ||
| messages = messages.filter(msg_tag_on_msg) | ||
|
|
||
| return { | ||
| "experiments": experiments, | ||
| "sessions": sessions, | ||
| "messages": messages, | ||
| "participants": participants, | ||
| "start_date": start_date, | ||
| "end_date": end_date, | ||
| } |
There was a problem hiding this comment.
❌ New issue: Large Method
filtered_querysets has 138 lines, threshold = 80
There was a problem hiding this comment.
@codescene-delta-analysis Both advisory findings accurately describe the code, and both come from this PR being a verbatim extraction rather than a rewrite.
filtered_querysets in dashboard_querysets.py is DashboardService.get_filtered_queryset_base moved unchanged: same 164-line body, same seven parameters (previously self plus six). That is why services.py shows as improved in the same analysis, going from 9.29 to 10.00 by losing Large Method. The method did not grow; it changed file, and the delta analysis scores both ends of the move.
Keeping it byte-identical is deliberate for this PR. The whole review argument here is "nothing changed except where the code lives", which is checkable by diffing the moved body against git history. Restructuring it in the same change would remove that property.
| def filtered_querysets( | ||
| team: Team, | ||
| *, | ||
| start_date: datetime | None = None, | ||
| end_date: datetime | None = None, | ||
| experiment_ids: list[int] | None = None, | ||
| platform_names: list[str] | None = None, | ||
| participant_ids: list[int] | None = None, | ||
| tag_ids: list[int] | None = None, | ||
| ) -> dict[str, Any]: | ||
| """Base querysets with the dashboard's common filters applied. Returns | ||
| `experiments`, `sessions`, `messages`, `participants` querysets plus the | ||
| resolved `start_date`/`end_date` (defaulting to the last 30 days).""" | ||
|
|
||
| if not end_date: | ||
| end_date = timezone.now() | ||
| if not start_date: | ||
| start_date = end_date - timedelta(days=30) | ||
|
|
||
| base_filters = {"created_at__gte": start_date, "created_at__lte": end_date} | ||
|
|
||
| experiments = Experiment.objects.filter(team=team, is_archived=False, working_version=None) | ||
| # Use Exists() to avoid join+distinct - prevents row explosion upfront for better performance | ||
| msg_exists = Exists( | ||
| ChatMessage.objects.filter( | ||
| chat=OuterRef("chat"), | ||
| created_at__gte=start_date, | ||
| created_at__lte=end_date, | ||
| ) | ||
| ) | ||
| sessions = ( | ||
| ExperimentSession.objects.filter(team=team) | ||
| .exclude(experiment_channel__platform=ChannelPlatform.EVALUATIONS) | ||
| .annotate(_has_msgs=msg_exists) | ||
| .filter(_has_msgs=True) | ||
| ) | ||
| messages = ChatMessage.objects.filter(chat__team=team, **base_filters).exclude( | ||
| chat__experiment_session__platform=ChannelPlatform.EVALUATIONS | ||
| ) | ||
| participants = Participant.objects.filter(team=team).exclude(platform=ChannelPlatform.EVALUATIONS) | ||
|
|
||
| if experiment_ids: | ||
| experiments = experiments.filter(id__in=experiment_ids) | ||
| sessions = sessions.filter(experiment_id__in=experiment_ids) | ||
| messages = messages.filter(chat__experiment_session__experiment_id__in=experiment_ids) | ||
| participants = participants.filter(experimentsession__experiment_id__in=experiment_ids).distinct() | ||
|
|
||
| if platform_names: | ||
| global_platforms = ChannelPlatform.team_global_platforms() | ||
| if not any(p in global_platforms for p in platform_names): | ||
| # only filter experiments if we're filtering by non-global platforms since all experiments | ||
| # will match the global platforms | ||
| experiments = experiments.filter( | ||
| Exists( | ||
| ExperimentChannel.objects.filter( | ||
| experiment=OuterRef("pk"), | ||
| platform__in=platform_names, | ||
| deleted=False, | ||
| ) | ||
| ) | ||
| ) | ||
| sessions = sessions.filter(platform__in=platform_names) | ||
| messages = messages.filter(chat__experiment_session__platform__in=platform_names) | ||
| participants = participants.filter(platform__in=platform_names) | ||
|
|
||
| if participant_ids: | ||
| experiments = experiments.filter(sessions__participant__id__in=participant_ids).distinct() | ||
| sessions = sessions.filter(participant__id__in=participant_ids) | ||
| messages = messages.filter(chat__experiment_session__participant__id__in=participant_ids) | ||
| participants = participants.filter(id__in=participant_ids) | ||
|
|
||
| if tag_ids: | ||
| chat_content_type = ContentType.objects.get_for_model(Chat) | ||
| message_content_type = ContentType.objects.get_for_model(ChatMessage) | ||
|
|
||
| # Sessions: chat or any message in it carries the tag (both the link row and its tag | ||
| # must belong to the reading team) | ||
| tag_on_chat, tag_on_msg = chat_tag_exists_pair(team, tag_ids, "chat_id") | ||
| sessions = sessions.annotate(_tchat=tag_on_chat, _tmsg=tag_on_msg).filter(Q(_tchat=True) | Q(_tmsg=True)) | ||
|
|
||
| # Experiments: any session's chat or messages carry the tag (both the link row and | ||
| # its tag must belong to the reading team) | ||
| exp_tag_on_chat = Exists( | ||
| CustomTaggedItem.objects.filter( | ||
| team_id=team.id, | ||
| tag__team_id=team.id, | ||
| content_type=chat_content_type, | ||
| object_id__in=Subquery( | ||
| Chat.objects.filter(experiment_session__experiment=OuterRef(OuterRef("id"))).values("id") | ||
| ), | ||
| tag_id__in=tag_ids, | ||
| ) | ||
| ) | ||
| exp_tag_on_msg = Exists( | ||
| CustomTaggedItem.objects.filter( | ||
| team_id=team.id, | ||
| tag__team_id=team.id, | ||
| content_type=message_content_type, | ||
| object_id__in=Subquery( | ||
| ChatMessage.objects.filter(chat__experiment_session__experiment=OuterRef(OuterRef("id"))).values( | ||
| "id" | ||
| ) | ||
| ), | ||
| tag_id__in=tag_ids, | ||
| ) | ||
| ) | ||
| experiments = experiments.annotate(_exp_tchat=exp_tag_on_chat, _exp_tmsg=exp_tag_on_msg).filter( | ||
| Q(_exp_tchat=True) | Q(_exp_tmsg=True) | ||
| ) | ||
|
|
||
| # Participants: any of their sessions' chats or messages carry the tag (both the | ||
| # link row and its tag must belong to the reading team) | ||
| part_tag_on_chat = Exists( | ||
| CustomTaggedItem.objects.filter( | ||
| team_id=team.id, | ||
| tag__team_id=team.id, | ||
| content_type=chat_content_type, | ||
| object_id__in=Subquery( | ||
| Chat.objects.filter(experiment_session__participant=OuterRef(OuterRef("id"))).values("id") | ||
| ), | ||
| tag_id__in=tag_ids, | ||
| ) | ||
| ) | ||
| part_tag_on_msg = Exists( | ||
| CustomTaggedItem.objects.filter( | ||
| team_id=team.id, | ||
| tag__team_id=team.id, | ||
| content_type=message_content_type, | ||
| object_id__in=Subquery( | ||
| ChatMessage.objects.filter(chat__experiment_session__participant=OuterRef(OuterRef("id"))).values( | ||
| "id" | ||
| ) | ||
| ), | ||
| tag_id__in=tag_ids, | ||
| ) | ||
| ) | ||
| participants = participants.annotate(_part_tchat=part_tag_on_chat, _part_tmsg=part_tag_on_msg).filter( | ||
| Q(_part_tchat=True) | Q(_part_tmsg=True) | ||
| ) | ||
|
|
||
| # Messages: the message's own tags carry the tag (both the link row and its tag must | ||
| # belong to the reading team). Message-only match - a tag on the chat (rather than the | ||
| # message itself) does not pull the chat's messages in here; that broader chat-or-message | ||
| # match is what the sessions/experiments/participants legs above use via | ||
| # `chat_tag_exists_pair`, not this one. | ||
| msg_tag_on_msg = Exists( | ||
| CustomTaggedItem.objects.filter( | ||
| team_id=team.id, | ||
| tag__team_id=team.id, | ||
| content_type=message_content_type, | ||
| object_id=OuterRef("id"), | ||
| tag_id__in=tag_ids, | ||
| ) | ||
| ) | ||
| messages = messages.filter(msg_tag_on_msg) | ||
|
|
||
| return { | ||
| "experiments": experiments, | ||
| "sessions": sessions, | ||
| "messages": messages, | ||
| "participants": participants, | ||
| "start_date": start_date, | ||
| "end_date": end_date, | ||
| } |
There was a problem hiding this comment.
❌ New issue: Excess Number of Function Arguments
filtered_querysets has 7 arguments, max arguments = 6
| @@ -0,0 +1,259 @@ | |||
| """Activity metrics over sessions, messages, and participants (#3905). | |||
There was a problem hiding this comment.
❌ New issue: Missing Arguments Abstractions
The average number of function arguments in this module is 4.11 across 18 functions. The average arguments threshold is 4.00
Product Description
The dashboard and the v2 usage API both report sessions, messages and active participants, but each computes them its own way, so the same team over the same window can see different numbers on the two surfaces. This PR changes none of those numbers - it moves both implementations into one place so the differences sit side by side and can be reconciled in a single follow-up, instead of being rediscovered whenever someone touches either surface.
Tag filtering across the dashboard, usage API and cost panel now honours only a team's own tags. Previously another team's tag link could influence what a team saw, and in the tag-analytics panel could put another team's tag name on their dashboard.
Second PR of #3905 (per the approved design there). Behaviour-preserving; the definition changes follow in PR 3.
Technical Description
apps/usage_metrics/.filters.pyholdsUsageFiltersand the shared chat-or-message tag match;dashboard_querysets.pyholds the dashboard'sget_filtered_queryset_basemoved verbatim;metrics.pyholds the v2 API's builders and aggregations as named metric functions plus timeseries variants.DashboardService.get_filtered_queryset_baseand the API's queryset/count/by-bucket helpers become delegations. The API's orchestration (zero-fill, bucket iteration, cost wiring, the grouped path) is untouched, and no existing test assertion changed -git diff base...HEADoverapps/dashboard/testsandapps/api/v2/usage/testsis 64 insertions, 0 deletions.test_characterisation.pypins both halves of six divergence classes so a wrong convergence in PR 3 turns red.team_idandtag__team_id, on all 9CustomTaggedItemExistssubqueries acrossusage_metricsandcost_tracking. This closes the CodeRabbit finding on Honour the dashboard tag filter in cost tracking reads #4132, which asked for both predicates. Each predicate is covered: removed individually at each site, at least one test reddens.tags__id__inM2M join, so onefiltered_querysets()call could return sessions/experiments/participants excluding an inconsistent-link chat while messages still included its messages. Andget_tag_analytics_datadereferencedtagged_item.tagforname/label, so a foreign link on one of your messages rendered their tag name in your panel - see Demo. Message-only matching is preserved in the first (a tag on the chat does not pull its messages in) and pinned by a control test.sessions_activeships scalar-only: the dashboard has two disagreeing per-period session-series implementations today, so picking one is PR 3's call.sessions_in_setupis new and unconsumed, the complement ofsessions_startedover non-evaluation sessions created in the window.experiment_ids=[]/participant_ids=[]as "matched nobody", the dashboard builder treats empty as "no filter".tag_ids=[]means "no filter" everywhere. Documented onUsageFilters, pinned at both poles.Migrations
No migrations -
apps/usage_metricsis models-free andmakemigrations --checkreports no changes.Demo
Dashboard on a dev team after the extraction: stat cards read the same 4 chatbots / 8 sessions / 24 messages / 6 active participants as before any code changed.
<attach screenshot-1786041676580-2.jpg>The tag-scoping fix. A
CustomTaggedItemowned by another team, carrying that team's tag, points at one of this team's messages. With the fix, only this team's own tags render:<attach screenshot-1786041738858-3.jpg>Same database state, same page, with the two predicates removed - the other team's tag name appears:
<attach screenshot-1786041843344-4.jpg>Docs and Changelog
Docs change is limited to the
usage_metricsrow indocs/architecture/package-map.md, included here.