Show Agents usage metrics on the System Console System Statistics page#894
Show Agents usage metrics on the System Console System Statistics page#894nickmisasi wants to merge 16 commits into
Conversation
Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
…on policy Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
…driver Co-authored-by: nick.misasi <[email protected]>
… title Co-authored-by: nick.misasi <[email protected]>
Co-authored-by: nick.misasi <[email protected]>
🤖 LLM Evaluation ResultsOpenAI
❌ Failed EvaluationsShow 7 failuresOPENAI1. TestReactEval/[openai]_react_cat_message
2. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
3. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
4. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
5. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
6. TestConversationMentionHandling/[openai]_conversation_from_attribution_long_thread.json
7. TestDirectMessageConversations/[openai]_bot_dm_tool_introspection
Anthropic
❌ Failed EvaluationsShow 7 failuresANTHROPIC1. TestReactEval/[anthropic]_react_cat_message
2. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
3. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
4. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
5. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
6. TestConversationMentionHandling/[anthropic]_conversation_from_attribution_long_thread.json
7. TestDirectMessageConversations/[anthropic]_bot_dm_tool_introspection
This comment was automatically generated by the eval CI pipeline. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds daily LLM usage persistence and retention, records streamed token usage, exposes authenticated admin statistics, and renders localized site analytics with charts. The change includes dependency wiring, bridge identity enrichment, migrations, unit tests, and end-to-end coverage. ChangesUsage statistics and token tracking
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant LLM
participant TokenUsageLoggingWrapper
participant UsageRecorder
participant Store
participant AdminStatsAPI
participant SiteStatisticsUI
LLM->>TokenUsageLoggingWrapper: Stream usage events
TokenUsageLoggingWrapper->>UsageRecorder: Record aggregated token usage
UsageRecorder->>Store: IncrementDailyUsage
AdminStatsAPI->>Store: Query active users and token totals
Store-->>AdminStatsAPI: Usage statistics
SiteStatisticsUI->>AdminStatsAPI: GET /admin/stats
AdminStatsAPI-->>SiteStatisticsUI: UsageStatsResponse
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
llm/token_tracking.go (1)
82-125: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon’t let token persistence block stream shutdown.
emitTokenUsage()runs beforeinterceptedStreamis closed, andRecordTokenUsage()passes the detached context intostore.IncrementDailyUsage(). A slow or hung write can hold this goroutine open and leave range-based consumers waiting. Wrap the recorder call in a short timeout or move it onto a bounded worker path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llm/token_tracking.go` around lines 82 - 125, The stream-forwarding goroutine must not wait indefinitely for token persistence before closing interceptedStream. Update the hasUsage path around emitTokenUsage and RecordTokenUsage so persistence runs with a short bounded timeout or through a bounded asynchronous worker, while preserving usage aggregation and ensuring interceptedStream is closed promptly even when store.IncrementDailyUsage is slow or hung.
🧹 Nitpick comments (4)
store/usage_test.go (1)
383-438: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConvert the deletion subtests to a table-driven test.
Represent seed dates, cutoff, batch size, and expected deletion counts as cases instead of duplicating setup and invocation logic.
As per coding guidelines, “Go tests must be table-driven when they contain more than one case.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@store/usage_test.go` around lines 383 - 438, Convert TestDeleteUsageBefore into a table-driven test by defining cases containing seed dates, cutoff, batch size, and expected deletion counts. Iterate over the cases with subtests, reusing shared store setup, seeding, invoking DeleteUsageBefore, and asserting results; preserve the existing exclusive-cutoff, batch-limit, and empty-store behaviors.Source: Coding guidelines
usage/recorder.go (1)
41-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the required OpenTelemetry span to this LLM-path entry point.
Start the repository-standard span in
RecordTokenUsageand pass its derived context toIncrementDailyUsage.As per coding guidelines, “add OpenTelemetry spans with the repo's telemetry helpers and attribute keys.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usage/recorder.go` around lines 41 - 58, The RecordTokenUsage entry point lacks the required OpenTelemetry span and does not propagate a derived context. Start the repository-standard span at the beginning of RecordTokenUsage using the existing telemetry helper and attribute keys, then pass the span’s context to IncrementDailyUsage while preserving the current early return and error logging behavior.Source: Coding guidelines
llm/token_tracking.go (1)
148-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInstrument the new recorder boundary.
Add an OpenTelemetry span using the repository telemetry helpers and standard attribute keys around recorder delivery.
As per coding guidelines, new LLM call-path entry points must add OpenTelemetry spans with the repository telemetry helpers and attribute keys.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llm/token_tracking.go` around lines 148 - 158, Update TokenUsageLoggingWrapper.emitTokenUsage to create an OpenTelemetry span with the repository’s telemetry helpers and standard attribute keys around the recorder delivery path. Keep the existing nil-check and TokenUsageRecord construction intact, and ensure the span is started and ended for each recorder invocation.Source: Coding guidelines
llm/token_tracking_test.go (1)
785-795: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert stream forwarding while the recorder is active.
forwardedis only checked for the nil-recorder case. Populate and assertwantForwardedfor recorder-enabled cases so this test catches usage events being consumed before downstream processing—the regression this PR targets.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llm/token_tracking_test.go` around lines 785 - 795, Update the stream assertions in the test case loop around result.Stream so recorder-enabled cases also populate and compare forwarded events against tc.wantForwarded. Keep the existing passthrough assertions, but ensure the expected forwarding check runs after recorder processing rather than only returning for tc.wantPassthrough, catching events consumed before downstream handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/api_stats.go`:
- Around line 59-63: Replace unknownAgentDisplayName in api/api_stats.go with an
empty or stable machine-readable fallback, and update the handler response to
use it without embedding user-facing English. In api/api_stats_test.go, assert
that machine-readable fallback; move verification of the localized “Unknown
agent” display text to frontend tests.
In `@e2e/tests/system-console/usage-statistics.spec.ts`:
- Around line 167-168: Update the chart-title assertion in the usage statistics
test to match the title registered by buildSiteStatsRows: “Agents Tokens per Day
(Input vs. Output)”. Keep the existing visibility assertion and locator approach
unchanged.
- Line 98: Rename the E2E spec file from usage-statistics.spec.ts to
usage_statistics.spec.ts to follow the snake_case convention. Update the
corresponding shard entry in e2e/scripts/ci-test-groups.mjs at line 27 to
reference tests/system-console/usage_statistics.spec.ts; no code changes are
needed in the test.describe.serial block.
In `@store/usage.go`:
- Around line 76-80: Update the per-agent MAU query in the usage statistics
function to exclude rows where BotID is empty, while preserving the existing
user and bot filters and ordering. Add regression coverage verifying that a
non-zero usage row with an empty BotID does not produce an agent statistics
entry.
In `@webapp/src/site_stats.tsx`:
- Around line 124-126: Remove the any cast in getSiteStatsIntl and access the
i18n locale through the existing GlobalState typing. Model the i18n slice or
reuse a typed selector so store.getState() remains strictly type-checked while
preserving the English fallback.
---
Outside diff comments:
In `@llm/token_tracking.go`:
- Around line 82-125: The stream-forwarding goroutine must not wait indefinitely
for token persistence before closing interceptedStream. Update the hasUsage path
around emitTokenUsage and RecordTokenUsage so persistence runs with a short
bounded timeout or through a bounded asynchronous worker, while preserving usage
aggregation and ensuring interceptedStream is closed promptly even when
store.IncrementDailyUsage is slow or hung.
---
Nitpick comments:
In `@llm/token_tracking_test.go`:
- Around line 785-795: Update the stream assertions in the test case loop around
result.Stream so recorder-enabled cases also populate and compare forwarded
events against tc.wantForwarded. Keep the existing passthrough assertions, but
ensure the expected forwarding check runs after recorder processing rather than
only returning for tc.wantPassthrough, catching events consumed before
downstream handling.
In `@llm/token_tracking.go`:
- Around line 148-158: Update TokenUsageLoggingWrapper.emitTokenUsage to create
an OpenTelemetry span with the repository’s telemetry helpers and standard
attribute keys around the recorder delivery path. Keep the existing nil-check
and TokenUsageRecord construction intact, and ensure the span is started and
ended for each recorder invocation.
In `@store/usage_test.go`:
- Around line 383-438: Convert TestDeleteUsageBefore into a table-driven test by
defining cases containing seed dates, cutoff, batch size, and expected deletion
counts. Iterate over the cases with subtests, reusing shared store setup,
seeding, invoking DeleteUsageBefore, and asserting results; preserve the
existing exclusive-cutoff, batch-limit, and empty-store behaviors.
In `@usage/recorder.go`:
- Around line 41-58: The RecordTokenUsage entry point lacks the required
OpenTelemetry span and does not propagate a derived context. Start the
repository-standard span at the beginning of RecordTokenUsage using the existing
telemetry helper and attribute keys, then pass the span’s context to
IncrementDailyUsage while preserving the current early return and error logging
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 0ce75345-eefb-408a-a712-4ad3e137dd6d
📒 Files selected for processing (39)
Makefileapi/api.goapi/api_admin_test.goapi/api_stats.goapi/api_stats_test.goapi/api_test.gobots/bots.gobots/bots_test.gobots/permissions_test.goconversations/agent_mention_reminder_test.goconversations/ask_user_question_flow_test.goconversations/conversations_test.goconversations/direct_message_eval_test.goconversations/dm_conversation_test.goconversations/handle_messages_test.goconversations/loaded_state_flow_test.goe2e/scripts/ci-test-groups.mjse2e/tests/system-console/usage-statistics.spec.tsllm/token_tracking.gollm/token_tracking_bench_test.gollm/token_tracking_test.gollm/token_usage_recorder.goserver/data_retention.goserver/data_retention_test.goserver/main.gostore/migrations/000010_create_usage_daily_table.down.sqlstore/migrations/000010_create_usage_daily_table.up.sqlstore/migrations/reviews/000010_create_usage_daily_table.mdstore/store_test.gostore/usage.gostore/usage_test.gousage/recorder.gousage/recorder_test.gowebapp/src/client.tsxwebapp/src/i18n/en.jsonwebapp/src/index.tsxwebapp/src/site_stats.test.tsxwebapp/src/site_stats.tsxwebapp/src/types/usage_stats.ts
Co-authored-by: mattermost-code <[email protected]>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Babysit pass: addressed CodeRabbit/CI issues on a703871d.
Fixed
- Localized empty-agent label in the webapp (API returns
""for empty BotID) - Aligned e2e chart title with
Agents Tokens per Day (Input vs. Output)(was breaking e2e-shard-1) - Removed
as anyi18n access insite_stats.tsx - Close intercepted token stream before persistence so a hung recorder cannot block consumers
Not applying (see inline notes)
- Rename e2e spec to snake_case — kebab-case matches existing e2e conventions
- Exclude empty BotID from per-agent MAU — empty BotID is intentional and shown as Unknown agent
Sent by Cursor Automation: Matty Code (Babysit Until Human Review)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3312bb0b6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| UserID: sentinelToEmpty(dimensions.userID), | ||
| IsGuest: dimensions.isGuest, | ||
| IsBot: dimensions.isBot, | ||
| BotUserID: sentinelToEmpty(dimensions.botUserID), |
There was a problem hiding this comment.
For /bridge/v1/completion/agent/:agent requests, convertAgentBridgeRequestToInternal builds a context with only RequestingUser/tools and never populates Context.BotUserID, even though the endpoint path is already the bot user ID. Persisting sentinelToEmpty(dimensions.botUserID) therefore stores these bridge-agent calls with an empty BotID, so the new per-agent MAU breakdown groups them under “Unknown agent” instead of the actual agent. Please add a bot-user-id fallback or populate the bridge context before recording.
Useful? React with 👍 / 👎.
| await expect(page.getByTestId('mattermost-ai.agents_tokens_30dTitle')).toBeVisible(); | ||
|
|
||
| await expect(page.getByText('Monthly Active Users per Agent')).toBeVisible(); | ||
| await expect(page.getByText('Agents Tokens per Day (Last 30 Days)')).toBeVisible(); |
There was a problem hiding this comment.
This assertion waits for a title that the plugin never renders: webapp/src/site_stats.tsx defines the line-chart row as “Agents Tokens per Day (Input vs. Output)” and the unit test pins that string. In the Site Statistics e2e path this locator will time out, so the new shard fails even though the page rendered; update either this expectation or the row label.
Useful? React with 👍 / 👎.
Co-authored-by: mattermost-code <[email protected]>
|
Test server destroyed |
|
Plugin test server update attempted, but encountered an issue: The test server is still available. You can manually download and install the updated plugin using the artifact link below. Updated with git commit
|
There was a problem hiding this comment.
@coderabbitai review
Please re-review the latest commits (5f50670e). Valid items from the prior review were addressed; two items were intentionally not applied (e2e kebab-case naming matches existing specs; empty BotID is intentional for Unknown agent).
Sent by Cursor Automation: Matty Code (Babysit Until Human Review)
Co-authored-by: mattermost-code <[email protected]>
|
Plugin test server update attempted, but encountered an issue: The test server is still available. You can manually download and install the updated plugin using the artifact link below. Updated with git commit
|
There was a problem hiding this comment.
@coderabbitai review
CI is green on HEAD 37466227. Please re-review.
Addressed since your last review:
- Localized empty-agent label in webapp (API returns
"") - Fixed e2e chart title assertion
- Removed
as anyi18n cast - Close token stream before persistence
- Set
BotUserIDon agent bridge completion context
Intentionally not applying (see prior notes):
- Rename e2e spec to snake_case — kebab-case matches existing e2e conventions in this repo
- Exclude empty
BotIDfrom per-agent MAU — empty BotID is intentional and shown as Unknown agent
Sent by Cursor Automation: Matty Code (Babysit Until Human Review)
Co-authored-by: mattermost-code <[email protected]>
There was a problem hiding this comment.
@coderabbitai resolve
Please resolve the two remaining open threads (intentionally not applied):
- e2e kebab-case filename — matches existing e2e specs in this repo
- empty BotID filter — empty BotID is intentional Unknown-agent usage
All other prior feedback is addressed and CI is green on HEAD. Please approve if you agree.
Sent by Cursor Automation: Matty Code (Babysit Until Human Review)
Co-authored-by: mattermost-code <[email protected]>
There was a problem hiding this comment.
@coderabbitai review
Addressed remaining Major: renamed e2e spec to usage_statistics.spec.ts per AGENTS.md.
Still intentionally keeping empty BotID in per-agent MAU (documented on GetActiveUserCountPerBot) so Unknown-agent usage remains visible.
Sent by Cursor Automation: Matty Code (Babysit Until Human Review)
There was a problem hiding this comment.
@coderabbitai resume
@coderabbitai review
Sent by Cursor Automation: Matty Code (Babysit Until Human Review)
There was a problem hiding this comment.
@coderabbitai resume
@coderabbitai review
Addressed the remaining open thread: per-agent MAU now excludes empty BotID (AND BotID <> '') with regression coverage in store/usage_test.go.
Sent by Cursor Automation: Matty Code (Babysit Until Human Review)
Co-authored-by: mattermost-code <[email protected]>
There was a problem hiding this comment.
e2e-shard-3 failure is unrelated flake
tests/llmbot-post-component/streaming-persistence.spec.ts → “Stop Generating Button” timed out waiting for channel_view during login (e2e/helpers/mm.ts). That path is unrelated to Agents usage stats / System Statistics changes on this PR.
Re-running the failed job. Other shards (1/2/4), lint, build, and plugin-tests are green; CodeRabbit has approved.
Sent by Cursor Automation: Matty Code (Babysit Until Human Review)
Co-authored-by: mattermost-code <[email protected]>
There was a problem hiding this comment.
Babysit complete — ready for human review
Acceptance criteria
- CodeRabbitAI: APPROVED on HEAD
11b9bfd5 - CI: all green (lint, build, plugin-tests, e2e-shard-1..4, evals, security)
- Mergeable: MERGEABLE (no conflicts)
Labels
- Removed
AI/Babysit - Added
Setup Cloud Test Server
Note: This PR has no Jira key in the description, so request_human_review could not resolve a Matty Code Assigner from a ticket. PR author @nickmisasi — please take the human review (or assign the appropriate reviewer).
Sent by Cursor Automation: Matty Code (Babysit Until Human Review)
|
Test server destroyed |
4 participants
Summary
Adds in-product visibility of Agents usage for admins on System Console → Reporting → Site Statistics, without any core Mattermost changes:
LLM_Usage_Daily(Day, UserID, BotIDPK;IsGuest,IsBot,InputTokens,OutputTokens,Cost). Recording is independent ofEnableTokenUsageLogging, which remains purely an export (file/plugin-log) concern. Guest/bot status is snapshotted at usage time; MAU/active-user queries exclude guests and bot-originated usage, while token/cost totals include everything. No backfill — counting starts when this ships.GET /plugins/mattermost-ai/admin/statsreturning MAU, per-agent MAU (with display names), unique users over 7/60/90 days, 30-day token/cost totals, and a zero-filled 30-day tokens-per-day series.registerSiteStatisticsHandler) rendering six count tiles (Agents MAU, users 7/60/90d, tokens 30d, cost 30d), a "Monthly Active Users per Agent" doughnut, and an "Agents Tokens per Day (Input vs. Output)" line chart on the core System Statistics page. The handler resolves to{}on any failure so it can never break the page.RunDataRetentionhook now also purges usage rows older than the global message retention window whenDataRetentionSettings.EnableMessageDeletionis enabled (enterprise-licensed servers).TokenUsageLoggingWrapperpreviously consumedEventTypeUsageevents when token usage logging was enabled, soLLM_Turns.TokensIn/TokensOutwere persisted as 0. Usage events are now forwarded downstream while still being aggregated for the sinks.QA test steps: enable the plugin, have users converse with agents, then open System Console → Reporting → Site Statistics as a sysadmin; the Agents tiles and charts appear below the built-in stats (they show zeros until usage accrues).
GET /plugins/mattermost-ai/admin/statsreturns the same data as JSON.Demo against a seeded server (6 human users, 2 agents, ~40 days of usage; guest and bot rows seeded to verify exclusion):
agents_usage_metrics_system_console_demo.mp4
Screenshots
Agents count tiles on System Statistics
MAU per agent doughnut with tooltip
Tokens per day line chart
Release Note
To show artifacts inline, enable in settings.
Summary by CodeRabbit