fix: implemented a stats script and updated telemetry tests - #82
Merged
shamikkarkhanis merged 3 commits intoFeb 20, 2026
Merged
Conversation
Contributor
Reviewer's GuideImplements Phase 2b in-memory analytics for the telemetry cog by introducing metrics dataclasses, wiring them into the existing event pipeline, adding a demo stats script, and expanding tests and lint configuration to cover the new behavior. Sequence diagram for_telemetry_event_processing_with_in_memory_metricssequenceDiagram
actor DiscordUser
participant DiscordClient
participant DiscordBot
participant Telemetry
participant TelemetryQueue
participant TelemetryWorker
DiscordUser->>DiscordClient: invoke_interaction
DiscordClient->>DiscordBot: interaction_create
DiscordBot->>Telemetry: log_interaction(interaction, interaction_type, command_name)
Telemetry->>TelemetryQueue: put(TelemetryEvent type=interaction, data)
loop background_consumer
TelemetryWorker->>TelemetryQueue: get()
TelemetryQueue-->>TelemetryWorker: TelemetryEvent
TelemetryWorker->>Telemetry: _dispatch_event(event)
alt interaction_event
Telemetry->>Telemetry: _log_interaction(data)
Telemetry->>Telemetry: _record_interaction_metrics(data)
else completion_event
Telemetry->>Telemetry: _log_completion(...)
Telemetry->>Telemetry: _record_completion_metrics(data)
end
end
DiscordBot->>Telemetry: get_metrics()
Telemetry-->>DiscordBot: TelemetryMetrics_snapshot
Class diagram for in_memory_telemetry_analytics_phase_2bclassDiagram
class TelemetryEvent {
+str event_type
+dict~str, Any~ data
}
class CommandLatencyStats {
+int count
+float total_ms
+float min_ms
+float max_ms
+record(duration_ms float) void
+avg_ms float
}
class TelemetryMetrics {
+datetime boot_time
+int total_interactions
+defaultdict~str, int~ interactions_by_type
+defaultdict~str, int~ command_invocations
+set~int~ unique_user_ids
+defaultdict~int, int~ guild_interactions
+defaultdict~str, int~ completions_by_status
+defaultdict~str, defaultdict~str, int~~ command_failures
+defaultdict~str, int~ error_types
+defaultdict~str, CommandLatencyStats~ command_latency
}
class Telemetry {
-commands.Bot bot
-logging.Logger log
-dict~int, tuple~str, float~~ _pending
-asyncio.Queue~TelemetryEvent~ _queue
-TelemetryMetrics _metrics
+__init__(bot commands.Bot)
-_dispatch_event(event TelemetryEvent) void
+log_interaction(interaction Any, interaction_type str, command_name str) None
+log_completion(interaction_id int, status str, duration_ms float, command_name str, error_type str) None
+log_command_failure(command_name str, status str, error_type str) None
+get_metrics() TelemetryMetrics
-_record_interaction_metrics(data dict~str, Any~) None
-_record_completion_metrics(data dict~str, Any~) None
}
Telemetry --> TelemetryMetrics : owns
TelemetryEvent <.. Telemetry : consumes
TelemetryMetrics --> CommandLatencyStats : uses
TelemetryMetrics "*" --> "1" CommandLatencyStats : latency_per_command
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Consider returning a copy or read-only view from
get_metrics()(or clearly documenting it) so callers of the stats API cannot accidentally mutate the internalTelemetryMetricsstate.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider returning a copy or read-only view from `get_metrics()` (or clearly documenting it) so callers of the stats API cannot accidentally mutate the internal `TelemetryMetrics` state.
## Individual Comments
### Comment 1
<location> `capy_discord/exts/core/telemetry.py:336-338` </location>
<code_context>
+ # ANALYTICS
+ # ========================================================================================
+
+ def get_metrics(self) -> TelemetryMetrics:
+ """Return the current in-memory metrics snapshot."""
+ return self._metrics
+
+ def _record_interaction_metrics(self, data: dict[str, Any]) -> None:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Returning the live TelemetryMetrics instance exposes internal state to mutation by callers.
External callers (e.g. a /stats handler) can mutate this object, affecting live counters and future readings. If this should be read-only, return a shallow copy or an immutable snapshot (e.g. a DTO), or clearly document that callers are allowed to mutate it. Using the internal instance also couples callers to this implementation and may complicate future changes (like adding locking or swapping out the metrics store).
Suggested implementation:
```python
def get_metrics(self) -> TelemetryMetrics:
"""Return an immutable snapshot of the current in-memory metrics."""
# Return a deep copy so external callers cannot mutate internal state.
return copy.deepcopy(self._metrics)
```
1. Add `import copy` near the top of `capy_discord/exts/core/telemetry.py` alongside the other imports, for example:
`import copy`
2. If you prefer not to deep copy (e.g. for performance reasons), you could instead:
- Introduce a DTO or `@dataclass` that represents a read-only snapshot and construct it from `self._metrics`, or
- Add a `to_snapshot()`/`copy()` method on `TelemetryMetrics` and call that from `get_metrics` instead of `copy.deepcopy`.
</issue_to_address>
### Comment 2
<location> `capy_discord/exts/core/telemetry.py:361-363` </location>
<code_context>
+ def _record_completion_metrics(self, data: dict[str, Any]) -> None:
+ """Update in-memory counters from a completion event."""
+ m = self._metrics
+ status = data.get("status", "unknown")
+ command_name = data.get("command_name", "unknown")
+ duration_ms = data.get("duration_ms", 0.0)
+
+ m.completions_by_status[status] += 1
</code_context>
<issue_to_address>
**issue (bug_risk):** Defaulting missing duration_ms to 0.0 risks skewing latency metrics and hides data issues.
Using 0.0 makes missing durations indistinguishable from real near-zero latencies and will bias aggregates downward, while also masking upstream data problems. Instead, consider either skipping latency updates when duration_ms is missing/None, or logging a warning and not updating command_latency for that event so malformed events are visible and don’t affect stats.
</issue_to_address>
### Comment 3
<location> `tests/capy_discord/exts/test_telemetry.py:275-283` </location>
<code_context>
+ assert m.error_types["UserFriendlyError"] == 1
+
+
+def test_record_completion_metrics_latency_stats(cog):
+ cog._record_completion_metrics({"command_name": "ping", "status": "success", "duration_ms": 10.0})
+ cog._record_completion_metrics({"command_name": "ping", "status": "success", "duration_ms": 30.0})
+
+ stats = cog.get_metrics().command_latency["ping"]
+ assert stats.count == 2
+ assert stats.avg_ms == 20.0
+ assert stats.min_ms == 10.0
+ assert stats.max_ms == 30.0
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a unit test for CommandLatencyStats with zero observations
This exercises the multi-observation path, but there’s no coverage for the zero-observation case. Please add a small test that constructs `CommandLatencyStats` directly, verifies its initial `min_ms`/`max_ms` values, and asserts `avg_ms == 0.0` when `count` is still zero.
Suggested implementation:
```python
def test_record_completion_metrics_latency_stats(cog):
cog._record_completion_metrics({"command_name": "ping", "status": "success", "duration_ms": 10.0})
cog._record_completion_metrics({"command_name": "ping", "status": "success", "duration_ms": 30.0})
stats = cog.get_metrics().command_latency["ping"]
assert stats.count == 2
assert stats.avg_ms == 20.0
assert stats.min_ms == 10.0
assert stats.max_ms == 30.0
def test_command_latency_stats_zero_observations():
stats = CommandLatencyStats()
# Initial state with no observations
assert stats.count == 0
assert stats.min_ms == float("inf")
assert stats.max_ms == 0.0
# avg_ms should be 0.0 when count is zero
assert stats.avg_ms == 0.0
```
1. At the top of `tests/capy_discord/exts/test_telemetry.py`, add an import for `CommandLatencyStats`, e.g.:
`from capy_discord.exts.telemetry import CommandLatencyStats`
(adjust the import path to match where `CommandLatencyStats` is actually defined).
2. If `CommandLatencyStats` uses different initial values for `min_ms`/`max_ms` than `float("inf")` and `0.0`, update the corresponding assertions in `test_command_latency_stats_zero_observations` to match the real defaults.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+275
to
+283
| def test_record_completion_metrics_latency_stats(cog): | ||
| cog._record_completion_metrics({"command_name": "ping", "status": "success", "duration_ms": 10.0}) | ||
| cog._record_completion_metrics({"command_name": "ping", "status": "success", "duration_ms": 30.0}) | ||
|
|
||
| stats = cog.get_metrics().command_latency["ping"] | ||
| assert stats.count == 2 | ||
| assert stats.avg_ms == 20.0 | ||
| assert stats.min_ms == 10.0 | ||
| assert stats.max_ms == 30.0 |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a unit test for CommandLatencyStats with zero observations
This exercises the multi-observation path, but there’s no coverage for the zero-observation case. Please add a small test that constructs CommandLatencyStats directly, verifies its initial min_ms/max_ms values, and asserts avg_ms == 0.0 when count is still zero.
Suggested implementation:
def test_record_completion_metrics_latency_stats(cog):
cog._record_completion_metrics({"command_name": "ping", "status": "success", "duration_ms": 10.0})
cog._record_completion_metrics({"command_name": "ping", "status": "success", "duration_ms": 30.0})
stats = cog.get_metrics().command_latency["ping"]
assert stats.count == 2
assert stats.avg_ms == 20.0
assert stats.min_ms == 10.0
assert stats.max_ms == 30.0
def test_command_latency_stats_zero_observations():
stats = CommandLatencyStats()
# Initial state with no observations
assert stats.count == 0
assert stats.min_ms == float("inf")
assert stats.max_ms == 0.0
# avg_ms should be 0.0 when count is zero
assert stats.avg_ms == 0.0- At the top of
tests/capy_discord/exts/test_telemetry.py, add an import forCommandLatencyStats, e.g.:
from capy_discord.exts.telemetry import CommandLatencyStats
(adjust the import path to match whereCommandLatencyStatsis actually defined). - If
CommandLatencyStatsuses different initial values formin_ms/max_msthanfloat("inf")and0.0, update the corresponding assertions intest_command_latency_stats_zero_observationsto match the real defaults.
shamikkarkhanis
requested changes
Feb 11, 2026
shamikkarkhanis
left a comment
shamikkarkhanis
left a comment
Member
There was a problem hiding this comment.
check the sourcery bug risk comments !
shamikkarkhanis
requested changes
Feb 12, 2026
shamikkarkhanis
left a comment
shamikkarkhanis
left a comment
Member
There was a problem hiding this comment.
======================================================================================================= warnings summary =======================================================================================================
tests/capy_discord/exts/test_telemetry.py::test_dispatch_unknown_event_type
tests/capy_discord/exts/test_telemetry.py::test_consumer_processes_events
tests/capy_discord/exts/test_telemetry.py::test_failure_internal_error_categorized
tests/capy_discord/exts/test_telemetry.py::test_completion_event_enqueued
/Users/shamik/Documents/capy/capy-discord/tests/capy_discord/exts/test_telemetry.py:22: DeprecationWarning: There is no current event loop
b.wait_until_ready = MagicMock(return_value=asyncio.Future())
-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
take a look at these test warnings
shamikkarkhanis
approved these changes
Feb 20, 2026
shamikkarkhanis
deleted the
feature/capr-33-implement-interaction-logging
branch
February 20, 2026 19:09
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implemented Phase 2b: In Memory Analytics for our Telemetry service.
Summary by Sourcery
Add in-memory telemetry analytics and stats demonstration for the Discord bot.
New Features:
Enhancements:
Build:
Tests: