No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vvzvlad/gitmost#403
Reference in New Issue
Block a user
Delete Branch "feat/402-collab-mcp-metrics"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Наблюдаемость collab-цикла и MCP — 6 семейств метрик к разбору инцидента per-cell правок (#400). Продолжение контура #355; всё за существующим гейтом
METRICS_PORT(не задан → ни коллекторов, ни листенера, все хелперы — дешёвые no-op). closes #402Pass 1 (сервер, коммит 1):
collab_doc_load_duration_seconds{size_bucket}—onLoadDocument, тайминг ТОЛЬКО реальных DB-load путей (already-loaded early-return не таймится). Вторая половина цикла симметрично store.collab_docs_open(gauge, read-on-scrape черезcollect()изhocuspocus.getDocumentsCount()— не инкрементится, поэтому не дрейфует) +collab_doc_loads_total/collab_doc_unloads_total(afterLoad/afterUnload). Трэшинг-детектор:rate(loads) ≈ rate(unloads)при маломdocs_open.collab_connect_duration_seconds—onConnect → connected, корреляция черезWeakMapпо общемуrequest(без утечки; текущий клиент = один сокет на документ).collab_auth_duration_seconds— вокругonAuthenticate(успех и провал).size_bucket(lt64k|lt256k|lt1m|ge1m, ровно 4 значения) на load и на существующийcollab_store_duration_seconds; единаяsizeBucket().storeDocumentвозвращает длину уже посчитанногоydocState— второгоencodeStateAsUpdateнет.Pass 2 (MCP, коммит 2):
3+5.
mcp_tool_duration_seconds{tool}иcollab_connect_timeouts_total.packages/mcpНЕ зависит от prom-client/сервера — только опциональный колбэкDocmostMcpConfig.onMetric(name, value, labels?), который хост маппит в registry.createDocmostMcpServermonkeypatch'итserver.registerToolДО регистрации инструментов → один чок-поинт таймит все инструменты (shared + inline), лейблtool= имя регистрации (bounded). Таймаут-коннекта эмитится один раз, только в колбэке 25-секундного таймера. Standalone (stdio, без колбэка) и выключенные метрики → полный no-op.How verified
Стенд из чистого состояния (rebase на develop):
pnpm --filter @docmost/mcp build— чисто.tsc -p apps/server— 0 ошибок в затронутых файлах.jest src/integrations/metrics src/collaboration— 121 passed (10 suites) (в т.ч. новые: границыsizeBucket, и что при снятомMETRICS_PORTвсе новые хелперы — no-op, registry null,docsOpenSourceне вызывается).node --test packages/mcp/test/unit/tool-timing.test.mjs— 4/4 (обёртка сохраняет возврат/throw, standalone no-op).METRICS_PORTвсе 9 семейств регистрируются и скрейпятся,collab_docs_openчитает актуальный размер черезcollect(), лейблыsize_bucket/toolприсутствуют, boot-crash prom-client нет.Внутренний цикл: 2 паса, по внутреннему ревью на каждый — блокеров нет. Ключевое перепроверено: гейт
METRICS_PORTдержит все новые инструменты (защита от boot-crash #369), gauge read-on-scrape не дрейфует, connect-WeakMapбез утечки (сверено с типами@hocuspocus/server—requestесть в обоих хуках),packages/mcpбез зависимости от prom-client, чок-поинт ловит все инструменты, no-op при выключенных метриках.Операционные заметки (для DevOps, вне кода)
collab_store_duration_secondsпоявился лейблsize_bucket→ в VM рядом со старой безлейбловой серией появится новая. Запросы дашборда обновить наsum without(size_bucket)(...); старая серия дотлеет за retention.packages/mcp/build/— gitignored, пересобирается в CI/Docker (pnpm --filter @docmost/mcp build), не коммичу.Checklist
packages/mcp stays free of prom-client/server: it only calls an optional DocmostMcpConfig.onMetric(name, value, labels?) sink the host provides. - client.ts: onMetric on the config; fires collab_connect_timeouts_total once, only inside the 25s connect-timeout callback (the connect-vs-unload signal). cleanup() clears the timer on every other finish path, so no double-count. - index.ts: createDocmostMcpServer monkeypatches server.registerTool (before registerShared + inline tools are registered) to wrap every handler with timeToolHandler — times in a finally on success AND throw, re-throws unchanged, emits mcp_tool_duration_seconds{tool=<registered name>} (bounded cardinality). Single choke point catches all tools. - mcp.service.ts: the per-request config resolver injects onMetric ONLY when isMetricsEnabled(), routing mcp_tool_duration_seconds -> observeMcpTool and collab_connect_timeouts_total -> incConnectTimeout. Disabled / standalone (stdio, no onMetric) -> undefined -> zero-overhead no-op. New node:test unit (tool-timing.test.mjs) covers the wrapper's value/throw preservation and the standalone no-op. packages/mcp/build/ is gitignored, not committed (CI rebuilds it). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>Передаю на ревью. Детали и прогоны — в теле PR. Ключевое для проверки: всё за гейтом
METRICS_PORT(защита от boot-crash prom-client, #369 — все инструменты создаются только вinit(), хелперы — no-op при снятом гейте);collab_docs_open— read-on-scrape черезcollect(), нигде не инкрементится (не дрейфует);packages/mcpостаётся без зависимости от prom-client (только опциональный колбэк); чок-поинтregisterToolловит все инструменты, лейблtoolbounded. 2 паса (сервер + MCP), по внутреннему ревью на каждый.Отдельно вынес операционную заметку в тело PR: у
collab_store_duration_secondsдобавился лейблsize_bucket— запросы дашборда надо обновить наsum without(size_bucket). Панели/алерт — DevOps-работа после выката (по тикету), в PR не входят.🤖 Авто-ревью — feat(metrics): наблюдаемость collab-цикла и MCP (#402, follow-up #355)
Вердикт: CHANGES (2 DO — 1 тест-лок + 1 док-формулировка). Отличная observability-работа: кардинальность строго ограничена, no-op-гейт корректен и покрыт, регрессий нет. Гейт зелёный (чистый клон-рум): server
tsc0, metrics jest 25 passed, mcpnode --test4 passed, mcp-пакет собран.✅ Что проверено независимо (9 аспектов)
size_bucket= ровно 4 фикс-значения (sizeBucket(), покрыт границами + non-finite→lt64k);mcp_tool_duration{tool}= имя РЕГИСТРАЦИИ тулзы (статическиеSHARED_TOOL_SPECS.*.mcpName+ строковые литералы, прослежено до всех call-site; хост дополнительно клампит?? 'other'). Счётчики/histogram connect/auth/docs_open — без лейблов. НИ одного per-request/page/user-лейбла. metrics-DoS-поверхности нет.nullдоinit()(раннийreturn if (!enabled)), все хелперы null-safe (x?.observe). Тест: при выключенных метрикахgetMetricsRegistry()===null, хелперы не бросают,registerDocsOpenSourceНЕ вызывает источник (бросающая fn — не срабатывает).startedAt, не таймится); store переиспользует единственныйY.encodeStateAsUpdate(нет второго encode,bytesдефолт 0 при throc); auth оборачиваетdoAuthenticate(тело дословно) вfinally— фейлы тоже таймятся.CollabMetricsExtension— WeakMap по общемуrequest; при отклонённой авторизации (connectedне сработал) запись GC'ится с request → лика нет.docs_open— read-on-scrape черезcollect()(не inc/dec, не дрейфует).observeCollabStoreсигнатура сменилась(seconds)→(bytes,seconds)— ОБА (единственных) caller'а обновлены;ydocStateприсваивается безусловно (одинreturn, нет пути в обход); монкипатчregisterToolкорректен для фикс-3-арг сигнатуры SDK (без double-wrap/corruption). Метрики не могут уронить hot-path (sizeBucketне бросает;onMetricnull-safe).onMetric(name,value,labels)— чистая граница (пакет@docmost/mcpне тянет prom-client); монкипатчregisterTool— верный единый choke-point (иregisterShared, и инлайн-регистрации).🔧 DO — доработать и перезапросить ревью (2)
registerToolне залочен тестом —packages/mcp/test/unit/tool-timing.test.mjsтестируетtimeToolHandlerИЗОЛИРОВАННО;createDocmostMcpServerнигде не конструируется, так что «монкипатч оборачивает КАЖДУЮ тулзу и прокидываетconfig.onMetric» не проверяется. Мутация (сломатьargs.slice(0,-1)/ детект handler-арга / порядок capture) — НИ один тест не падает, хотя это единственный choke-point всего MCP-тайминга (и он «слегка магический» — зависит от «handler = последний арг» стабильности SDK). Фикс: интеграционный тест —createDocmostMcpServer({..., onMetric: spy}), прогнать одну тулзу черезInMemoryTransport/Client, assert эмитаmcp_tool_duration_secondsс{ tool: "<имя регистрации>" }. Залочит и «оборачивает все тулзы», и «лейбл = имя регистрации».mcp.service.ts-коммент говорит, что при выключенных метриках тул-таймер «no-op с zero overhead». Монкипатч оборачивает каждый handler БЕЗУСЛОВНО, так что на каждый вызов тулзы всё равно выполняетсяperformance.now()+ async try/finally-фрейм даже приonMetric===undefined(аргумент?.()короткозамыкается —/1000и{tool}не строятся, это ок). Практически ничтожно при LLM-темпе, но «zero» буквально неверно. Фикс: переформулировать в «negligible overhead».⛔ DROP — калибровочный лог (кодеру НЕ делать)
[below-threshold]suggestion/low[test-coverage] «включённые» проверки (size_bucket реально эмитится /docs_open collect()читает источник / счётчики inc) есть только на ВЫКЛЮЧЕННОМ (no-throw) пути — реестр это module-singleton, гейтящийся наMETRICS_PORTпри импорте, так что включить его in-process неудобно; два контракт-критичных теста (гейт + границы) на месте. Реальное ограничение, но ниже порога DO.[speculative]suggestion/low[stability]timeToolHandlerfinally иclient.tsconnectTimer зовутonMetric/onMetricFnбез try/catch — бросок хоста замаскировал бы результат тулзы / подвесил connect. На практике недостижимо: хост-колбэк зовёт только null-safe?.observe/?.incна валидированных number/string; standalone —onMetricundefined. Belt-and-suspenders, не обязателен.🔍 fan-out: security · stability · conventions · documentation · regressions · test-coverage · simplification · architecture · coherence → judge. Гейт (чистый клон-рум): server tsc 0 · metrics jest 25 · mcp node --test 4. Независимо: прослежена кардинальность
toolдо статических имён, проверены всеobserveCollabStorecaller'ы, границыsizeBucketв тесте, WeakMap-no-leak. База develop16b476a2.DO-1: add an integration test (test/mock/tool-timing-server.test.mjs) that constructs a real createDocmostMcpServer with a spy onMetric, links a Client over InMemoryTransport, invokes get_workspace (no input schema, so the wrapped handler always runs) and asserts onMetric fired with ("mcp_tool_duration_seconds", <number>, { tool: "get_workspace" }). This locks that the monkeypatch wraps every tool AND labels with the registration name — which the isolated timeToolHandler unit test does not. Mutation-verified (mislabel -> test fails). The tool's expected backend failure (ECONNREFUSED) is tolerated: the wrapper times in a finally on throw too, so the metric fires. DO-2: reword the mcp.service.ts "zero overhead when disabled" comment to "negligible overhead" — the registerTool wrapper still runs performance.now() + an async try/finally per tool call when onMetric is undefined (the `onMetric?.()` short-circuits the label build; cost is immaterial at tool-call rate), so "zero" was literally inaccurate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>Поправил оба DO, перезапрашиваю ревью.
DO-1 (монкипатч registerTool не залочен). Добавил интеграционный тест
test/mock/tool-timing-server.test.mjs: строит реальныйcreateDocmostMcpServer({..., onMetric: spy}), линкуетClientчерезInMemoryTransport.createLinkedPair(), дёргает одну тулзу и проверяет эмит("mcp_tool_duration_seconds", <number>, { tool: "get_workspace" }). Выбралget_workspace(без input-схемы → обёрнутый handler гарантированно исполняется; имя регистрации =SHARED_TOOL_SPECS.getWorkspace.mcpName). Бэкенд-фейл (ECONNREFUSED наapiUrl: 127.0.0.1:1) допускается — обёртка таймит вfinallyи на throw, метрика всё равно эмитится. Мутация: подменил лейбл на"mutant"→ тест краснеет; откатил. Теперь залочено и «оборачивает все тулзы», и «лейбл = имя регистрации».DO-2 (оверклейм «zero overhead»). Переформулировал коммент в
mcp.service.tsна «negligible overhead»: обёртка безусловно гоняетperformance.now()+ async try/finally на каждый вызов тулзы даже приonMetric===undefined(короткозамыкается?.(), лейбл/объект не строятся; на темпе tool-call'ов ничтожно), так что «zero» было буквально неверно.Гейт:
node --testtool-timing — 5/5; полный mcp-набор 483 pass / 2 fail (те же предсуществующие markdown-converter кейсы, к метрикам отношения нет).packages/mcp/build/не коммичу (gitignored).По DROP согласен: try/catch вокруг
onMetric— belt-and-suspenders (хост-колбэк зовёт только null-safe?.observe/?.incна валидированных значениях), не тащу.🤖 Авто-ревью (фикс-раунд) — оба DO закрыты → PASS ✅
Фикс-раунд
7a9d719 → 8f5f587: +87 (новый интеграционный тест) + док-реворд (прод-кодmcp.service.ts— только комментарий, поведение не менялось). Гейт: mcpnode --test5 pass (4 unit + 1 новый integration), metrics jest 25 passed.Адверсариальная проверка обоих DO
registerToolне залочен) — ЗАКРЫТ. Новыйtest/mock/tool-timing-server.test.mjs: настоящий интеграционный тест —createDocmostMcpServer({onMetric})+ реальныйClientчерезInMemoryTransport, вызов живой тулзы, assert эмитаmcp_tool_duration_secondsс{ tool: <имя регистрации> }. Мутация (сломать реконструкцию аргументов монкипатчаslice(0,-1)→slice(0), т.е. никогда не оборачивает) — тест падает (через реальную пересборку пакета). Choke-point теперь реально залочен. ✓onMetric?.()short-circuits so no label/object is built». Точно и честно. ✓Остальное (кардинальность ограничена, no-op-гейт покрыт, тайминги/WeakMap/read-on-scrape корректны, регрессий нет) подтверждено в прошлом раунде. Approve.
Гейт: mcp node --test 5 pass, metrics jest 25 passed. Мутация монкипатча (
slice(0,-1)→slice(0)) роняет новый integration-тест. Диф фикса от прошлого head7a9d719(тест + comment-only). База develop16b476a2.