feat(plugins): add the chat analytics event vocabulary - #574
Conversation
zaira-bibi
changed the title
|
@claude-review |
|
Claude finished @zaira-bibi's task in 1m 59s —— View job Claude PR Review —
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| def test_schemas_forbid_extra_fields(schema: type[AnalyticsEventSchema]) -> None: | ||
| """extra="forbid" on the base class stops a producer smuggling content into a payload.""" | ||
| with pytest.raises(ValidationError): |
There was a problem hiding this comment.
Low — assert the specific exception. pytest.raises(Exception) will pass on any error, so this test would still pass if the assignment failed for an unrelated reason (e.g. a typo'd attribute name). A frozen dataclass raises dataclasses.FrozenInstanceError, so assert on that:
| def test_schemas_forbid_extra_fields(schema: type[AnalyticsEventSchema]) -> None: | |
| """extra="forbid" on the base class stops a producer smuggling content into a payload.""" | |
| with pytest.raises(ValidationError): | |
| context = CompletionAnalyticsContext(provider="openai", model="gpt-4o", rag_used=True, actor_id="7") | |
| with pytest.raises(FrozenInstanceError): | |
| context.model = "changed" # type: ignore[misc] |
(requires from dataclasses import FrozenInstanceError at the top.)
| registry = ToolRegistry() | ||
| registry.discover_plugin_tools() | ||
| # Every discovered tool must resolve to some category string, never raise. | ||
| for name in registry._categories: | ||
| assert isinstance(registry.category_for(name), str) |
There was a problem hiding this comment.
Low — this test is vacuous if discovery finds no tools. If discover_plugin_tools() registers zero tools (no MCP tools present in the test environment), the for loop body never runs and the test passes without asserting anything. Add a guard so it fails loudly rather than silently rubber-stamping:
registry.discover_plugin_tools()
assert registry._categories, "expected discovery to register at least one categorised tool"
for name in registry._categories:
...Same file reaches into registry._categories (a private attr) here and in test_reset_clears_categories. That's acceptable for a whitebox registry test, but a public assertion via category_for(name) alone would be slightly more robust to internal renames.
| event_type = "chat.tool_invoked" | ||
| version = 1 | ||
|
|
||
| conversation_id: str |
There was a problem hiding this comment.
conversation_id comes up in every schema, could we create a parent class for this?
There was a problem hiding this comment.
the natural approach fails at import time:
class ChatEvent(AnalyticsEventSchema): # no event_type/version
conversation_id: str
# TypeError: ChatEvent must declare ['event_type', 'version'] as class attributes
as core/analytics/schemas/base.py enforces that every subclass declares its own identity. if we give it dummy event_type/version then a concrete schema that forgot to declare its own would silently inherit them and register under the wrong type, defeating that guard.
if we use a plain BaseModel and do something like:
class ConversationScoped(BaseModel):
conversation_id: str
class ChatToolInvoked(ConversationScoped, AnalyticsEventSchema):
event_type = "chat.tool_invoked"
version = 1
tool_name: str
tool_category: str
this works but the DRY win is small (as it's one str field across four classes), and it also costs a non-obvious trick that exists purely to dodge a safety check. i'd rather skip this and keep the current version.
| await emit_event( | ||
| ChatConversationStarted.event_type, | ||
| ChatConversationStarted.version, | ||
| {"conversation_id": conversation_id, "provider": provider, "model": model}, |
There was a problem hiding this comment.
lets indent this dict just like its indented below
|
|
||
| # Category reported for a tool the registry never discovered. A missing category | ||
| # degrades one analytics dimension; it must never break emission. | ||
| UNKNOWN_TOOL_CATEGORY = "unknown" |
There was a problem hiding this comment.
lets move this to the existing chat/constants.py
| conversation_id: str | ||
| provider: str | ||
| model: str | ||
| message_length: int |
There was a problem hiding this comment.
message_length and tool_call_count are plain ints, so a producer bug (e.g. -1) validates fine and skews aggregates. Worth using NonNegativeInt / Field(ge=0) here.
| if isinstance(value, str) and value: | ||
| names.append(value) | ||
| break | ||
| return names |
There was a problem hiding this comment.
Records with no usable name are dropped silently. If either provider ever renames the key, this zeroes tool analytics with no trace. Worth a logger.warning on the miss (keys only, not values).
| return names | ||
|
|
||
|
|
||
| async def emit_conversation_started(conversation_id: str, provider: str, model: str, actor_id: str) -> None: |
There was a problem hiding this comment.
Each emit_* helper hand-lists its schema's fields in a dict, so a field added to the schema but forgotten here only fails at runtime. Building the payload from the schema instance (event.model_dump(mode="json")) would catch that at type-check time and collapse these four helpers into one.
| def test_schemas_forbid_extra_fields(schema: type[AnalyticsEventSchema]) -> None: | ||
| """extra="forbid" on the base class stops a producer smuggling content into a payload.""" | ||
| with pytest.raises(ValidationError): | ||
| schema(unexpected_field="leak") # type: ignore[call-arg] |
There was a problem hiding this comment.
This passes for the wrong reason: every schema has required fields, so this raises ValidationError on the missing fields even without extra="forbid". It'd stay green if the smuggling guard were removed. Use a fully valid payload plus one extra key, and assert on extra_forbidden.
|
|
||
| def test_completion_context_is_frozen() -> None: | ||
| context = CompletionAnalyticsContext(provider="openai", model="gpt-4o", rag_used=True, actor_id="7") | ||
| with pytest.raises(Exception): |
There was a problem hiding this comment.
pytest.raises(Exception) is too broad here, it'd pass on an unrelated bug too. A frozen dataclass raises FrozenInstanceError specifically, assert that.
e7fae13 to
b37f11d
Compare
Chat is the course-authoring surface: instructors use it to generate courses, and its tool registry exposes every other plugin's MCP tools. These four events therefore measure authoring activity (conversations, turns, completions) and authoring output (tool executions), not learner engagement. Payloads carry identifiers, lengths, flags and names only. No message content, conversation title, prompt, tool argument or tool output can enter a payload -- extra="forbid" on the base schema enforces that, and the schemas are the single place the permitted field set is declared. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two chat completion paths report tool executions in different
shapes: streaming tool_end events yield {"name": ...}, while the
non-streaming provider records {"tool", "tool_input", "output"}.
tool_names() reads both, so that mapping exists in exactly one place
instead of drifting between the two seams that will consume it.
It reads only the name. tool_input and output can hold course content
and learner-identifying data, and must never reach an analytics payload.
Categories live on the MCP Tool and are dropped by the LangChain
conversion, so ToolRegistry now records them while it converts.
Unknown names resolve to UNKNOWN_TOOL_CATEGORY rather than raising: a
missing category degrades one analytics dimension, it must never break
emission.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The route seams should not have to know event names, schema versions, or that LangChain tools carry no category. Each helper builds one payload, reading its event type and version off the schema class so a version bump cannot silently desync the emitted event from its registered schema, and emit_tool_invoked resolves the tool category from the registry so no call site needs to. Nothing is caught here: a failed analytics write propagates, matching emit_event's contract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b37f11d to
cb4759a
Compare
| it — so the mapping is recorded during discovery. Unknown names resolve to | ||
| :data:`UNKNOWN_TOOL_CATEGORY` rather than raising. | ||
| """ | ||
| if not self._initialized: |
There was a problem hiding this comment.
This pattern is being repeated multiple times across the ToolRegistry class. I would recommend writing instead:
class ToolRegistry:
"""Registry for managing LangChain tools."""
def __init__(self) -> None:
self._tools: dict[str, BaseTool] | None = None
def tools_dict(self) -> dict[str, BaseTool]:
if self._tools is None:
self._tools = self._discover_plugin_tools()
return self._tools
Interestingly, while writing this, I realised that we could replace many instance methods by static methods: _convert_mcp_to_langchain_tool, _get_handler_type_hints, _build_args_schema_from_handler, _json_schema_to_pydantic, _resolve_ref, _get_python_type etc. could all be moved out of the ToolRegistry class and made normal functions.
Also, refresh_tools is unused anywhere, and we should thus gete rid of the reset method.
Maybe this might be worth having a look if you take on this PR?
4 participants
What
Adds the chat plugin's analytics vocabulary — four
chat.*event schemas, a tool-name normaliser, a tool-category lookup, and four emit helpers — with no route wired up yet, so the whole thing is additive and independently reviewable.Changes
AnalyticsEventSchemasubclasses (chat.conversation_started,chat.message_sent,chat.completion_served,chat.tool_invoked) plus the frozenCompletionAnalyticsContext, registered fromChatPlugin.__init__tool_names()— one normaliser reading tool executions from both completion paths' differently-shaped recordsToolRegistry.category_for(), defaulting toUNKNOWN_TOOL_CATEGORYemit_*helpers that build each payload and hand it toemit_eventHow to Test
DuplicateEventTypeErrorfrom the new registration:analytics.pynow importingtools.py:uv run python -c "import sparkth.main"Notes
raw_eventstable, not DDL.extra="forbid"rejects anything else, andtool_names()reads only the name key — nevertool_inputoroutput, which can hold course content and learner-identifying data.tool_names()exists because the two completion paths disagree on shape: the streaming path'stool_endevents yield{"name": …}, while the non-streaming provider records{"tool", "tool_input", "output"}. Keeping that mapping in one place is what stops the two seams in stack 3/4 from drifting apart.emit_event's contract from feat(analytics): add emit_event for analytics producers #572 — a failed analytics write propagates rather than being hidden.This PR description was written with the assistance of an LLM (Claude).