feat(mcp-analytics): separate mcp properties in the taxonomic filter - #69575
Conversation
MCP analytics events ($mcp_tool_call and friends) carry a known property schema. Surface that schema as a dedicated "MCP properties" taxonomic filter group, the way autocapture separates its element properties: - derive an mcp_properties group in the core taxonomy from the $mcp_* event properties, and mark $mcp_tool_name as the primary property of $mcp_tool_call so tool-call rows and funnel steps describe themselves by tool name - add a TaxonomicFilterGroupType.MCPProperties group to both the legacy picker and the rebuild, only materialized when the picker is scoped to canonical $mcp_* events; options remap to event properties so selection produces plain event property filters - seed $mcp_is_error into Suggested filters when scoped to $mcp_tool_call so success/failure splits are one click away - offer the group (ahead of Event properties) in the default property picker, insight property filters, breakdowns, and the MCP analytics dashboard filter bar Generated-By: PostHog Code Task-Id: 25a24ca3-78e9-4c59-b439-c4181a705285
|
Hey @pauldambra! 👋 It looks like your git author email on this PR isn't your
You can fix it for this repo with: git config user.email "you@posthog.com"Or set it globally with |
🤖 CI report✅ Bundle size — 🟢 -1.96 MiB (-2.8%)Uncompressed size of every built Total: 68.07 MiB · 🟢 -1.96 MiB (-2.8%)
Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report ✅ Eager graph — within budgetHow much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy
🟢 Largest files eagerly shipped from
|
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 668 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 277.3 KiB | ../node_modules/.pnpm/posthog-js@1.399.1/node_modules/posthog-js/dist/rrweb.js |
| 266.9 KiB | ../node_modules/.pnpm/@posthog+icons@0.37.4_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 234.6 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 221.5 KiB | ../node_modules/.pnpm/posthog-js@1.399.1/node_modules/posthog-js/dist/module.js |
| 164.0 KiB | src/queries/validators.js |
| 154.2 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 105.9 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Dist folder size — 🟢 -157.34 MiB (-10.9%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1283.76 MiB · 🟢 -157.34 MiB (-10.9%)
⚠️ Playwright — 1 flaky
🎭 Playwright report · View test results →
- service facet passes serviceNames to API (chromium)
These issues are not necessarily caused by your changes.
Annoyed by this section? Help fix flakies and failures and it will go green!
The new TaxonomicFilterGroupType enum value flows into the generated frontend/src/queries/schema.json and posthog/schema_enums.py; CI's schema freshness check (schema:build:json && git diff --exit-code) failed without them. Generated-By: PostHog Code Task-Id: 25a24ca3-78e9-4c59-b439-c4181a705285
Adds a Storybook story showing the picker scoped to $mcp_tool_call (Suggested filters leading with tool name + error state, and the dedicated MCP properties group), and renders MCP group rows through PropertyKeyInfo so the schema shows friendly labels instead of raw $mcp_* keys. Also carries pr-demo-mcp-properties.gif temporarily so a PR comment can embed the recorded demo via a commit-pinned raw URL; removed again in the next commit so it never reaches the merged tree. Generated-By: PostHog Code Task-Id: 25a24ca3-78e9-4c59-b439-c4181a705285
The recording stays reachable through the commit-pinned raw URL on the previous commit; the branch tree goes back to code only. Generated-By: PostHog Code Task-Id: 25a24ca3-78e9-4c59-b439-c4181a705285
🎥 Demo: MCP properties in the taxonomic filter (Storybook)Recorded from the new story What the recording walks through:
Recording the video surfaced a real gap, fixed in the same commit: MCP group rows rendered raw Housekeeping: the gif rides on commit 68a1408 (pinned raw URL) and was removed from the tree in the next commit, so it never reaches the merged tree. The story also gives this setup permanent visual-regression coverage. Created with PostHog Code |
|
|
||
| /** The properties funnels and paths between tool calls pivot on, shown first. | ||
| * The rest of the known schema follows in taxonomy (alphabetical) order. */ | ||
| const PRIORITY_MCP_PROPERTIES: string[] = [ |
There was a problem hiding this comment.
could this be derived from CORE_FILTER_DEFINITIONS_BY_GROUP["mcp_properties"] somehow?
might be better to have a property on those objects rather than duplicate the names here
pauldambra
left a comment
pauldambra
left a comment
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
QA Swarm review complete — four independent perspectives (qa-team specialists + generalists, paul-reviewer, xp-reviewer, security-audit). See inline comments and the summary comment.
|
Note 🤖 Automated comment by QA Swarm — not written by a human Multi-perspective review: qa-team (specialists + generalists), paul-reviewer, xp-reviewer, security-audit Verdict: 💬 APPROVE WITH NITSStructurally sound where it matters most: the new group type never leaks into persisted data (all commit paths serialize as plain Key findings
ConvergenceIndependently flagged by 2+ reviewers: the duplicated priority lists (paul + xp + 3 qa-team personas), the cross-group duplicate rows (4 qa-team personas), the excluded-properties bypass (3 personas), the missing rebuild suggestion (2), the over-broad prefix heuristic (3), the unsynced taxonomy mirror (2), and the group-literal dual-write (paul + xp, who both concluded it follows the file pair's existing convention). Findings without a diff anchor
Reviewer summaries
Automated by QA Swarm — not a human review |
- derive PRIORITY_MCP_PROPERTIES from POSTHOG_EVENT_PROMOTED_PROPERTIES so the picker ordering and event-inspector promotion share one source of truth - record rebuild recents under an option-declared group (mirrors legacy getItemGroup resolution) so MCP-tab picks don't grow near-duplicate Recent rows across picker variants; guarded by a hook test - move the mcp_properties taxonomy mirror below every event_properties mutation and add a sync test so late additions can't silently miss it - guard isMCPAnalyticsEventName against non-string entries, memoize getMCPPropertyFilterOptions, and correct the suggested-properties comment (the rebuild doesn't seed primary properties yet) Generated-By: PostHog Code Task-Id: 25a24ca3-78e9-4c59-b439-c4181a705285
When the MCP properties tab is in a picker, the known $mcp_* schema is now excluded from the Event properties group (server-side via the existing excluded_properties mechanism), so each property renders in exactly one place — mirroring how autocapture's element properties exist only in the Elements group. Unknown $mcp_* keys a team ingests still surface under Event properties, and pickers that don't request the MCP tab are untouched. Applied to both picker variants via a shared helper, with parameterized drift-guard tests on each side. Generated-By: PostHog Code Task-Id: 25a24ca3-78e9-4c59-b439-c4181a705285
🦔 Hogbox preview · ✅ ready▶ Open the preview
commit |
👀 Auto-assigned reviewersThese soft owners were skipped because they only have minor changes here. Nothing blocks merge, so self-assign if you'd like a look:
Soft owners come from |
…limit Adding props.taxonomicGroupTypes as a 17th input to the taxonomicGroups selector pushed it past kea's SelectorTuple type cap (max 16), so tsc failed with TS2322 (tuple not assignable) and TS7006 (implicit any on the inline props selector). Fold the MCP-excluded event properties into the existing eventNamesWithPrimaryProperties selector (already combined for this exact reason) instead of adding a new input, keeping taxonomicGroups at 16 deps. Generated-By: PostHog Code Task-Id: be28193b-c899-48db-b149-f20d248054ef
|
Note 🤖 stamphog reviewed All of the extensive "QA Swarm / paul-reviewer / xp-reviewer / security-audit" review rounds are posted under the PR author's own account (@pauldambra), not by any distinct bot or human reviewer — so despite the appearance of multi-perspective independent review, there is no genuine independent assurance here, and this PR also touches generated public API/query-schema surfaces across several products and the MCP service.
Gate mechanics and policy version
Updated in place — this replaces 2 earlier stamphog review(s) on this PR. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additive taxonomy/UI feature (new "MCP properties" picker tab) with no data model, migration, auth, billing, or CI changes; the generated schema/enum touches are just a new backwards-compatible enum literal. The prior automated review's three MEDIUM findings (duplicated priority list, cross-variant recents/telemetry drift, duplicate property listing) are all visibly fixed in the current diff and backed by new tests, and remaining LOW items are acknowledged hardening notes, not blockers.
- Author wrote 60% of the modified lines and has 387 merged PRs in these paths (familiarity MODERATE).
- 👍 on the PR from chatgpt-codex-connector[bot], hex-security-app[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 509L, 19F substantive, 792L/26F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (792L, 26F, cross-cutting, feat) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ 280e388 · reviewed head 44356e4 |
|
Note 🤖 Automated comment by QA Swarm — not written by a human Follow-up review of the delta since efd7bd5 ( Verdict:
|
| Reviewer | Assessment |
|---|---|
| 🔍 qa-team | HIGH risk, REQUEST CHANGES — verified typecheck break + three convergent MEDIUMs; product logic itself sound |
| 👤 paul | "nothing blocking — happy for you to ship"; soft flags on module-load fragility and test coupling |
| 📐 xp | "solid, careful delta"; assert recents presence not exclusivity; memoization is mild premature optimization (optional) |
| 🛡 security-audit | 0 findings — excluded-properties param carries only repo constants; remap bounded to the picker's own groups; localStorage same-origin |
Automated by QA Swarm — not a human review
- resolve the recents declared-group remap against allGroups, not the visible tabs, matching legacy getItemGroup for pickers that request a curated tab without its canonical group (reachable via the activity-log picker) - key the rebuild's shared fetch cache on group-level excludedProperties / propertyAllowList, which are fetch-time params that now vary by picker - content-key the groups-context memo dep so per-render array literals don't rebuild the context - decouple the recents hook test from cross-test localStorage/timer state, exercise the curated-tab-only path, and pin a concrete key in the exclusion assertions - soften the popover doc claim and note the undefined-group-types gate degrades toward benign duplication Generated-By: PostHog Code Task-Id: 25a24ca3-78e9-4c59-b439-c4181a705285
|
✅ Visual changes approved by @pauldambra — baseline updated in 2 changed, 2 new. Changed
New
|
|
Note 🤖 Automated comment by QA Swarm — not written by a human Round-2 review of the fix delta ( Verdict: 💬 APPROVE WITH NITSEvery area flagged for scrutiny verified sound: the Remaining items (all LOW/NIT)
Reviewer summaries
Automated by QA Swarm — not a human review |
- sort exclusion/allowlist sets before stringifying into the shared fetch cache key, and pin the keying with a regression test so dropping the entries can't silently cross-serve lists between pickers - content-key eventNames in the groups-context memo like its sibling dep, with a note contrasting the legacy selector's stabilization strategy - update the cohortsModel cache-key-shape doc to point at remoteKey instead of a stale positional enumeration - rescope the exclusion comments to the mechanism precedent and move the recents test unmount into finally Generated-By: PostHog Code Task-Id: 25a24ca3-78e9-4c59-b439-c4181a705285
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
…changes - the invalidation predicate matched only the 'taxonomic-list' key family, so cohort create/rename/delete left cached 'taxonomic-list-search' results serving stale entries for the staleTime window on large-cohort-count teams; match both families and document them (flagged while reviewing the new exclusion-aware cache keys, which made the key contract load-bearing) - state the backend set-semantics assumption the sorted cache keys lean on, and scope the dedupe claim to exclusions - parameterize the cache-key regression test over both params and pin the order-independence the sort adds - replace the stale cohort-invalidation TODO with the wired-up reality Generated-By: PostHog Code Task-Id: 25a24ca3-78e9-4c59-b439-c4181a705285
pauldambra
left a comment
pauldambra
left a comment
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
QA Swarm round-3 review of the hardening commit 6f8b91f — one convergent MEDIUM (pre-existing cohort search-cache invalidation gap the new doc enshrined), the rest LOW/NIT refinements to the round-2 fixes. See inline comments and the summary comment.
|
Note 🤖 Automated comment by QA Swarm — not written by a human Round-3 review of the hardening commit Verdict: 💬 APPROVE WITH NITSThe commit's load-bearing changes all verified sound under adversarial review: the spread-copy sort is deterministic and non-mutating for everything that can flow in, the sorted-key/unsorted-URL merge cannot cross-serve wrong data against the current backends (set-conversion + fixed ORDER BY, verified server-side), three personas independently mutation-tested the regression test and confirmed it genuinely fails when the keying is dropped, and the eventNames content-keying changes no observable memo behavior. Key findings
Reviewer summaries
Automated by QA Swarm — not a human review |
- export the two key-family literals from useGroupList and consume them in the cohortsModel invalidation predicate, so a future key family can't silently escape invalidation the way the search family did - collapse the cache-key test's two-stage wait into a single exact waitFor (fetches fire synchronously from effects, so the count is deterministic) Generated-By: PostHog Code Task-Id: 25a24ca3-78e9-4c59-b439-c4181a705285
…undone - an undone delete restored the cohort without re-invalidating, so pickers kept serving the deleted-state cache for the staleTime window - move the key-family constants next to the cache they describe (useTaxonomicResource), removing the models-to-hook edge - assert the differing exclusion/allowlist params actually reach the request URL (coverage the parameterized rewrite had dropped) and correct the waitFor mechanism comment Generated-By: PostHog Code Task-Id: 25a24ca3-78e9-4c59-b439-c4181a705285
|
Note 🤖 Automated comment by QA Swarm — not written by a human Round-4 (final convergence) review of Verdict: 💬 APPROVE WITH NITS — quality loop convergedpaul-reviewer and xp-reviewer returned zero findings ("ship it" / "this is fine — no smell survives"). qa-team verified the three scrutiny points sound — no module cycle or load-order hazard from the new import edge, the exact
Not actioned (deliberate): the family-registry suggestion — the enumeration is now type-checked through shared constants, both families are documented at the predicate, and a registry for exactly two values contradicts the same round's xp verdict ("no superfluous parts"); revisit if a third key family ever appears. Reviewer summaries
Automated by QA Swarm — not a human review |
4 updated Run: 0258c250-bf34-4bf1-88a8-2298019523f2 Co-authored-by: pauldambra <984817+pauldambra@users.noreply.github.com>
2 participants
Problem
MCP analytics captures tool call events (
$mcp_tool_calland friends) with a known$mcp_*property schema, but in the taxonomic filter that schema is buried in the generic event properties list. The target use case is building funnels between different tool calls, or user paths between tool calls, split by success or failure. We want the event and its expected properties separated out for clarity, the way autocapture separates its element properties.Changes
mcp_propertiesgroup in the core taxonomy, derived from the$mcp_*event properties (no duplication), and$mcp_tool_namemarked as theprimary_propertyof$mcp_tool_call. The primary property makes tool call rows, funnel step labels, and suggested filters describe themselves by tool name.TaxonomicFilterGroupType.MCPProperties("MCP properties") group, defined in both the legacy picker (taxonomicFilterLogic.tsx) and the rebuild (buildTaxonomicGroups.tsx) from a shared helper (utils/mcpProperties.ts). It only materializes when the picker is scoped to canonical$mcp_*events, so nothing changes for anyone else. Options are priority ordered (tool name, category, is error, error type, duration, client, intent first) and remap to event properties on selection, so picking one produces a plain event property filter.$mcp_is_erroris seeded into Suggested filters when scoped to$mcp_tool_call, mirroring the autocapturetext/selectorseeding, so the success/failure split is one click away.$mcp_tool_callalso gets promoted properties for event detail views.With this, the funnel flow is: pick the "MCP tool call" event per step, filter each step by tool name from the leading MCP tab (or straight from Suggested filters), and break down by
$mcp_is_error.Note
Two decisions here need explicit sign-off per the taxonomic filter guardrails:
$mcp_*events, which is a brand-new context with no existing selection behavior to regress. Non-MCP pickers are untouched (verified by test).legacy-control/legacy-pill(shared code path) andrebuild-menu(separate group table), with tests on both sides guarding drift. The rebuild arm needed an explicitMCPProperties -> Eventfilter type mapping because it commits with the tab's own group type.A natural follow-up (not in this PR): an "MCP tool calls" pseudo-event group for the funnel/trends step picker that lists tool names directly and expands to
$mcp_tool_call+ tool name filter, likeAutocaptureEventsdoes for element text. That needs anhas_mcp_tool_callproject event existence flag in the app context (cache shape change inget_default_event_info), so it's left out here.How did you test this code?
Automated only, no manual UI verification was possible in this environment:
utils/mcpProperties.test.ts(new): catches the taxonomy drift that would empty or misorder the MCP tab, the gating regression that would surface it for legacy unprefixedmcp_*events, loss of theprimary_propertywiring, and (rebuild side) loss of the eventNames gating or the event-properties remap on options.taxonomicFilterLogic.test.ts(extended): same gating assertions for the legacy variant, so a one-sided edit that diverges the two arms fails.utils.test.ts(extended): the one-wayMCPProperties -> PropertyFilterType.Eventmapping, without which rebuild-arm selections produce a typeless filter.TaxonomicFilter/,PropertyFilters/,BreakdownFilter/,ActionFilter/suites pass (51 suites, 923 tests).hogli ci:preflight --fixis green.Automatic notifications
Docs update
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
/modifying-taxonomic-filter(before any change) and/writing-tests(before the tests).ErrorTrackingProperties/ActivityLogPropertiespattern (curated options with agroup: EventPropertiesremap) rather than an endpoint-backed list, so the expected schema shows even before every property has been ingested; rejected moving the$mcp_*definitions out ofevent_propertiesin the taxonomy (would break existing label lookups) in favor of deriving the new group from them.Created with PostHog Code