feat(telemetry): add CLI version and agent/human marker to usage events (CLI-12) - #757
feat(telemetry): add CLI version and agent/human marker to usage events (CLI-12)#757soustruh wants to merge 1 commit into
Conversation
…ts (CLI-12) The telemetry team (KIDS-522) reports on kbagent usage from the ext.keboola.cli. events. Their report needs the CLI version and an agent-vs-human marker. The stored event body carried neither. Elasticsearch keeps the body, not the server-stamped envelope or the headers, so neither value reached them. Every usage event now writes params.cliContext: the User-Agent (version + OS/arch/Python) and, when set, the conversation id. The CLI path reads the id from KBAGENT_CONVERSATION_ID. The serve middleware reads it from the request's X-Conversation-ID header.
keboola-pr-reviewer-bot
left a comment
keboola-pr-reviewer-bot
left a comment
There was a problem hiding this comment.
Verdict: auto_approve (risk 2/5) · profile keboola-mcp-server
Additive telemetry enrichment — CLI version + conversation-id marker on usage events, well-tested, no auth or transport change.
Concerns:
src/keboola_agent_cli/server/app.py: Caller-supplied X-Conversation-ID header forwarded verbatim into telemetry body.
zajca
left a comment
zajca
left a comment
There was a problem hiding this comment.
Actionable findings from the automated review.
| # KIDS reads the CLI version (auto-update adoption) and an agent-vs-human | ||
| # marker off the stored event body, so both ride in params.cliContext (CLI-12). | ||
| cli_context: dict[str, Any] = {"userAgent": build_user_agent()} | ||
| if conversation_id: |
There was a problem hiding this comment.
Reviewed by Opus.
The new marker is purely presence-based: any non-empty conversation id makes the event look like an agent run (gotchas.md:5019-5021 states this explicitly). But kbagent serve unconditionally exports a session id into its own process env — commands/serve.py:270-271 sets KBAGENT_CONVERSATION_ID=serve-<UTC-ts>-<8hex> — and _default_conversation_id's docstring (commands/serve.py:36-38) says the serve- prefix is precisely how observability filters a human-driven session. The serve startup banner (commands/serve.py:354, :373) then tells the operator to export KBAGENT_CONVERSATION_ID=serve-... in a second terminal for their own kbagent http workflow.
Failure scenario: a human runs kbagent serve, copies the banner's export KBAGENT_CONVERSATION_ID=serve-20260912T101500Z-ab12cd34 into a second terminal, and runs kbagent config list. emit_cli_invocation (telemetry.py:227) reads that env var, _send_event writes cliContext.conversationId, and the KIDS report counts a hand-typed command as an agent run. The same applies to serve's own scheduled kbagent http child processes, which inherit the env var. The signal this PR exists to deliver is therefore biased upward for exactly the flow the CLI itself recommends.
Consider either excluding serve--prefixed ids from the marker, or emitting an explicit source/interface field instead of inferring intent from the id's presence — and at minimum documenting the serve- caveat in the gotchas.md block so the report author does not read presence as agent.
| conversation_id: str | None = None | ||
| for key, value in scope.get("headers") or []: | ||
| if key == b"x-conversation-id": | ||
| conversation_id = value.decode("latin-1") or None |
There was a problem hiding this comment.
Reviewed by Opus.
The header value is decoded and forwarded unmodified into params.cliContext.conversationId (telemetry.py:325), which is POSTed to /v2/storage/events and stored in the customer project's event log. There is no length cap and no control-character filtering, even though the module deliberately bounds the other free-text field it stores — results.error is truncated at _MAX_ERROR_LEN = 1000 specifically to keep the event under the events API's 200 KB cap (telemetry.py:52-54, _resolve_error_text).
Failure scenario: an authenticated serve client (a buggy integration, or a caller abusing the endpoint) sends X-Conversation-ID with several KB of arbitrary bytes — up to the ~16 KB h11 header budget, including control characters that survive the latin-1 decode. Every mutating request then writes that blob into the project's Storage event log, bloating events and injecting junk into whatever dashboard reads cliContext; past the API cap the POST is rejected and swallowed at DEBUG, silently dropping serve telemetry.
A short truncation plus a printable-character filter on the value (and, for symmetry, on the KBAGENT_CONVERSATION_ID read at telemetry.py:227, which is likewise unbounded and unstripped) would match the treatment the module already gives results.error.
3 participants
Why
The telemetry team (KIDS-522) reports on kbagent usage from the
ext.keboola.cli.events. Their report needs the CLI version and an agent-vs-human marker. The stored event body carried neither. Elasticsearch keeps the request body, not the server-stamped envelope or the headers, so neither value reached the team.What
_send_eventwritesparams.cliContexton every usage event. It holdsuserAgent(thekeboola-cli/<version> (<os>; <arch>; <impl> <pyver>)User-Agent, which carries the version and platform) and, when set,conversationId.KBAGENT_CONVERSATION_ID/--conversation-id. The serve middleware reads it from the request'sX-Conversation-IDheader. A request that sends the header is marked. One without it is not.gotchas.mddocumentsparams.cliContext, tagged(since vNEXT).Tests
test_telemetry.py: the success event carriescliContext.userAgent, and a setKBAGENT_CONVERSATION_IDlands incliContext.conversationId.test_serve_telemetry.py: the request'sX-Conversation-IDreachessend_serve_event, and its absence sendsNone.Related