Add instrumentation and feedback collection infrastructure - #1174
Open
penso wants to merge 28 commits into
Open
Conversation
… profiles Moltis had no trace export of any kind. The workspace declared four opentelemetry crates that no code used, and nothing recorded what an agent run actually did beyond Prometheus counters. This adds moltis-observability: one instrumentation pass in the agent runtime, fanned out to any number of backends through an ObservationSink. The central design point is that backends do not receive the same payload. Langfuse is an LLM-native product whose cost, session and prompt-version views are built on conversation content, so it gets prompts, completions, tool arguments and results, the full observation taxonomy, cache-split token usage and managed-prompt linkage. Datadog and Grafana are operational tools: prompt bodies there mean unbounded span size, cardinality pressure, per-byte ingest billing, and user conversation content sitting in a system nobody scoped to hold it. They get latency, errors, model and token counts, with payload sizes instead of payloads and no per-user cardinality. An ExportProfile carries that policy and the mapping layer consults it before emitting any attribute; the default profile is the conservative one so a newly configured backend cannot silently receive conversation content. Transport is OTLP/JSON for every backend. That is Langfuse's own modern ingest path, used by its v3+ Python and v4+ JS SDKs, and the only one carrying the AGENT/TOOL/RETRIEVER observation types: the native ingestion API's observation-create event is marked deprecated upstream and its supported span-create/generation-create pair cannot express them. The same transport reaches Grafana Tempo, Honeycomb and Datadog's OTLP intake. Langfuse's REST ingestion API is still used for scores, which OTLP cannot express, and for the connection test. Telemetry never blocks a turn: record() is synchronous and try_send-based, dropping on a saturated queue and counting the drops rather than applying backpressure to the agent loop. Retries use exponential backoff with full jitter so instances that hit one rate limit do not retry in lockstep. Redaction runs before serialization on both key names and value shapes, since tool results are unstructured and a leaked credential rarely lands under a key called "password". Plaintext http is refused for non-loopback endpoints because these payloads carry content and a credential header. Config defaults to disabled, unlike every other Moltis feature flag: turning this on ships conversation content to a third party and should be a deliberate act. 135 tests cover the profile split, redaction, batching and backpressure, retry classification, and the OTLP wire encoding.
…tartup Connects the instrumentation core to the places that generate data and to the gateway that configures it. The agent runner now records a trace per turn, a GENERATION observation per LLM call carrying model, cache-split token usage and time-to-first-token, and a TOOL observation per tool call. Retrieval tools are recorded as RETRIEVER so backends that special-case RAG steps light up rather than showing every tool as an opaque TOOL. Instrumentation reaches the runner through a process-wide sink rather than a new parameter on run_agent_loop_streaming and its siblings. Those signatures already carry ten arguments and are called from many places; threading an eleventh through for telemetry would have touched far more code than the feature warrants, and the runner already reads global config directly. Two details worth calling out. The generation span is closed explicitly on the stream-error path before the retry return, because relying on the drop guard there would emit a span with no error recorded on it — a failed provider call would look successful in the backend. And the turn recorder is shared as an Arc because tool futures execute concurrently; finish() takes &self for the same reason. Trace scope derives from the session key and channel binding: the session key becomes the backend session id so a Langfuse session matches a Moltis conversation one-to-one, and the sender becomes a user id namespaced by channel so telegram:42 and slack:42 are never merged into one person. Gateway startup applies config to GatewayState's own instrumentation instance, so the RPC handlers and the shutdown flush observe the same backends that were installed. Backends that fail to start are reported with a reason through instrumentation.status rather than only logged — a silently disabled exporter is indistinguishable from a broken one. instrumentation.status never returns the secret key, only whether one is set. instrumentation.test probes Langfuse's health and credential endpoints; for OTLP backends it declines rather than POSTing a probe span, which would put fake data in the operator's traces. Adds the config template section and docs/src/instrumentation.md.
One page for Langfuse, OTLP and Datadog, since all three are fed from the same instrumentation pass and configuring them in separate places would misrepresent how they relate. The page leads with what each backend actually receives. That asymmetry is the design rather than an implementation detail, and an operator who does not know about it will either point Datadog at their prompts or wonder why their APM spans look empty compared to Langfuse. Backends that are enabled in config but failed to start are shown with the reason next to the backend they belong to, so a missing key or a rejected plaintext endpoint is visible where someone is already looking rather than only in the server log. instrumentation.status deliberately reports secret_key_set rather than the key: returning the value would place it in every browser devtools network log. An E2E test asserts that, alongside coverage for the backend cards, the disabled-by-default state, and the test-connection button being unavailable while Langfuse is off.
An end-to-end test over the real HTTP path caught this: the eager in-progress span emitted when a step opens was being sent to every backend. Langfuse upserts observations by id, so the start span is exactly what makes a long-running turn visible while it is still executing, with the closing update filling in model, usage and output. A plain OTLP collector — Tempo, Datadog — treats spans as immutable, so it would render the start and the completion as two separate spans sharing one id, duplicating every LLM call and tool call in the trace. Partial spans are now gated on the export profile alongside the other per-backend policy. Langfuse keeps the live view; APM backends receive only completed spans. Adds tests/end_to_end.rs, which drives a recorder through a live BatchSink into a mock collector and asserts on the bytes that actually leave the process: that prompts and completions reach Langfuse, that neither they nor the user id reach a generic OTLP backend, that operational signal still does, that a tool-argument credential is redacted even on the backend that receives everything else, that the span tree nests tool under generation under root, that cache tokens survive unsummed, and that recording 200 steps against a dead backend does not stall the caller.
Takes TokenUsage::to_usage_details by value, since the workspace denies wrong_self_convention and the type is Copy. Adds the standard expect_used/unwrap_used allow to the test modules and the integration test file, matching what the rest of the workspace does — the workspace denies both in production code, and test modules opt out explicitly rather than the lint being relaxed globally. Also corrects the README operations line: it claimed OpenTelemetry tracing while the four opentelemetry workspace dependencies were unused by any crate. It now names what is actually shipped.
The streaming loop was instrumented but the non-streaming one was not, and that is the path sub-agents take. spawn_agent calls run_agent_loop_with_context_and_limits, and silent_turn calls run_agent_loop, so every sub-agent run and every silent turn produced no trace at all — dropping exactly the nested-agent case the AGENT observation type exists to represent, and leaving a parent trace that showed a tool call with no visible work underneath it. Mirrors the streaming path: a trace per turn, a GENERATION observation per completion carrying model, cache-split usage and the message array, and the same explicit close-as-failed before the retry return so a failed provider call is not rendered as a successful span by the drop guard.
The workspace denies expect_used and unwrap_used in production code; test modules opt out explicitly, matching the rest of the workspace. The new instrumentation config tests were missing that opt-out and failed the clippy gate.
…2e suite Scores were recorded and then silently discarded. The Langfuse backend carries traces over OTLP, but the OTLP mapping has no representation for a score and drops those events by design, so every `TurnRecorder::score` call ended in a void — the REST client that can post scores was built and then only used by the settings "Test connection" button. Langfuse now contributes two sinks instead of one: traces over OTLP as before, plus a `ScoreSink` onto the ingestion API. The sink filters the fanout down to score events before they reach its queue, so a busy agent's trace volume cannot evict the comparatively rare scores. A rejected batch is classified retryable, because the usual causes (the trace has not landed yet, a 5xx) are transient and user feedback is not worth dropping on the first failure. Also splits the Langfuse exporter into a module directory ahead of the prompt, dataset and media APIs landing next to it. Separately, `TurnRecorder::begin` could only read the process-wide global sink, which made the end-to-end tests flaky at about 1 run in 12: they execute in parallel in one process, so one test's `set_global_sink` would overwrite another's and its spans would land in the wrong collector, leaving the original assertion with an empty body list. Adds `begin_with_sink` so a recorder can be pointed at an explicit destination, and moves the tests onto it. 25 consecutive runs green. The same entry point is what the experiment runner will need to route dataset traces separately from live traffic.
Adds the channel-agnostic half of reaction feedback: a vocabulary that turns a raw reaction token into a signal, and a score builder that makes the result idempotent. Normalization matters more than it looks. The three channels send three shapes for the same gesture — Telegram and Discord send the emoji, Slack sends a shortcode, and either may carry a skin-tone modifier or a presentation selector. Without folding those away, a thumbs up with a skin tone reads as no signal at all, so a user's feedback would be silently dropped based on how their client renders an emoji. Score ids are derived as a UUIDv5 of trace, score name and user rather than random. Langfuse upserts on score id, so this makes the natural reaction patterns behave correctly: switching from a thumb up to a thumb down overwrites that user's vote instead of recording two contradictory ones, a duplicate webhook delivery is a no-op, and removing a reaction can find the score to retract. Two users on the same trace still score independently. The default vocabulary is deliberately narrow. A default that swallowed every positive-looking emoji would turn release parties and acknowledgement reactions into quality data; teams that want that can set the lists explicitly, per-side, without restating the side they did not change. Also adds `delete_score` for retraction, treating a 404 as success since an already-absent score is the state the caller asked for.
…ced them A reaction names a message, not a turn. By the time someone reacts the agent run is long finished and nothing in the system remembers which trace wrote a given chat message, so attribution has to be recorded at send time. Keys on (channel_type, account_id, chat_id, message_id) rather than message id alone: channel message ids are only unique within a conversation — Telegram numbers them per chat — so a global lookup would confidently attribute a reaction in one chat to an unrelated turn in another. Stores identifiers only. The message log already owns message bodies, and a second copy here would quietly widen what the feedback feature retains. Upserts on relink so an edited or resent reply scores the newer turn, and prunes by age so the table does not grow for the life of the install.
Adds the service that records which trace produced a reply and converts a later reaction on it into a score, plus the plumbing each half needs. Channel outbound gains `send_text_reporting_ids`, an additive default method rather than a change to `send_text`. Every channel API returns the delivered message id and every one of them throws it away, but changing the existing signature would touch all fifteen channels for one feature. Telegram, Discord and Slack override it; the rest fall back to reporting nothing, so a channel that cannot report ids loses feedback attribution rather than losing the message. It returns a list because all three split long replies into several messages and a reader may react to any of them — reporting only the last would leave a thumb on an earlier chunk unattributable. The trace id itself comes from a small bounded per-session registry populated at turn start. A reply is dispatched far from the agent loop that produced it, and sessions process turns serially, so "the current trace for this session" is unambiguous at the moment its reply goes out. That avoids threading a trace id through every outbound signature. Eviction is by insertion order and costs an attribution, never a message. Reactions are classified before the link lookup, so the many non-feedback reactions in a busy chat do not each cost a query. Removing a reaction retracts the score rather than recording another one.
Completes the path from a thumb on a chat message to a score in Langfuse. The reply dispatcher lives in moltis-chat, which does not depend on the gateway, so the feedback service moved down into moltis-channels where both can reach it. `ChatRuntime` gains a defaulted `feedback()` accessor rather than a required method, so runtimes that do not collect feedback need no change. The service is constructed empty and installed during startup, mirroring how instrumentation itself is applied: reply/trace correlation needs the database pool, which does not exist at state construction. Everything is inert until installed, so a reaction arriving mid-startup is ignored rather than panicking, and re-applying settings takes effect without a restart. Reaction handling runs before the WebSocket broadcast so a slow client cannot delay scoring. Slack already emitted `ChannelEvent::ReactionChange` and nothing consumed it; that event is now the pipeline's entry point. Trace links are pruned on startup against the configured retention window.
Slack already reported reactions; Discord and Telegram did not, so two of the three channels the feedback pipeline targets were silent. Discord already requested the GUILD_MESSAGE_REACTIONS intent but implemented no handler for it. The bot's own acknowledgement reactions are filtered out — it marks incoming messages with an eyes/check/cross emoji, and treating those as opinions would score every turn the bot itself handled. Telegram needs `message_reaction` in `allowed_updates` or it never delivers reaction updates at all, and it reports them as a whole-set diff rather than an add/remove event: each update carries the reaction lists before and after. Scoring needs the difference, and in particular a swap from thumbs up to thumbs down arrives as a single update that must produce both a retraction and a new vote rather than only the latter. Anonymous channel reactions are skipped on Telegram and custom emoji on both: without a user id two people's votes collapse onto one score, and a custom emoji is an opaque id with no name a vocabulary could match.
Adds the score UI: a thumbs pair in the message action bar, and the `feedback.*` RPC behind it. The web is treated as one more channel rather than a special case, so a thumb in the browser resolves through the same correlation table as a Telegram reaction. Links are keyed on the run id, not the message index — an index shifts when older history is compacted away, which would silently re-attribute old feedback to the wrong turn. The RPC takes a named signal rather than a numeric score so the wire format cannot smuggle an arbitrary value into the metric, and clicking an already active thumb clears it instead of re-asserting it. The control only renders when instrumentation is active and feedback is enabled; a thumb that goes nowhere is worse than no thumb. When a submission cannot land, the reason is surfaced: a turn older than the retention window reports that it is too old to rate rather than showing a generic failure the user would retry forever. `on_reaction` now takes the channel as a string. The alternative was adding a `Web` variant to `ChannelType`, which enumerates real chat channels — the web UI is not one, and it would have leaked into every match over that enum.
Cost was never computed anywhere in the process; `cost_details` existed on the model but was always empty, so every backend had to derive spend from a private price table of its own, and nothing could show cost without one configured. Adds a built-in USD-per-million-tokens table and prices usage at the point it is recorded. Cache buckets are priced separately rather than folded into input: a cache read can be an order of magnitude cheaper, and summing them would overstate spend on exactly the workloads caching exists to make cheaper. Lookup is longest-prefix over a normalized model id, so a dated snapshot like `claude-opus-4-5-20260101` inherits its family's price without an entry per release, `gpt-4o-mini` is not priced as `gpt-4o`, and the same model routed through OpenRouter prices identically to one called directly. An unknown model reports no cost rather than falling back to an average. A confident wrong number that looks authoritative is worse than a visible gap. `set_model` re-prices any usage already on the step, because callers are free to record usage before the model is known and the step would otherwise keep a cost of zero forever.
The feedback and pricing work pushed four files that were sitting just under the limit over it. Each split follows the domain boundary rather than cutting at an arbitrary line: - chat: reply/trace correlation and the compaction notice both move out of the channel delivery module into siblings. - discord: inbound reaction handling moves next to the message handler. - gateway: the feedback startup wiring moves out of post_state. - channels: ChannelType and its parsing leave the plugin trait file. No behaviour change.
Contributor
Greptile SummaryAdds backend-neutral agent instrumentation and reaction feedback infrastructure.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| crates/gateway/src/methods/dispatch.rs | Classifies feedback submission as a write-scoped RPC and tests rejection of read-scoped clients. |
| crates/gateway/src/methods/services/feedback.rs | Validates web feedback requests and submits them using the repository's single-owner identity model. |
| crates/chat/src/channel_feedback.rs | Records reply trace links using bare channel chat IDs so Telegram topic reactions resolve consistently. |
| crates/chat/src/agent_loop.rs | Collects delivered streaming message IDs and records trace links after stream workers finish. |
| crates/chat/src/channels.rs | Records trace links for separately delivered streamed logbook follow-ups. |
| crates/observability/src/runtime.rs | Provides runtime-configurable instrumentation lifecycle and bounded delivery flushing. |
| crates/observability/src/exporters/langfuse/mod.rs | Maps completed traces and observations into Langfuse-compatible OTLP exports. |
| crates/web/ui/src/pages/sections/InstrumentationSection.tsx | Adds instrumentation configuration and connection-testing controls to the settings UI. |
Sequence Diagram
sequenceDiagram
participant User
participant Channel as Web/Channel
participant Gateway
participant Agent
participant Provider
participant Telemetry
User->>Channel: Send message
Channel->>Gateway: Dispatch request
Gateway->>Agent: Run instrumented turn
Agent->>Provider: Completion request
Provider-->>Agent: Stream/result and usage
Agent->>Telemetry: Record completed trace and observations
Agent-->>Channel: Deliver reply and message IDs
Channel->>Gateway: Submit reaction
Gateway->>Telemetry: Upsert or delete feedback score
Reviews (6): Last reviewed commit: "fix(observability): harden telemetry edg..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Merging this PR will not alter performance
Comparing 
Footnotes
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Four defects, all of which made the feedback path either forgeable or inert on the delivery routes people actually use. `feedback.submit` was registered in READ_METHODS while writing a score, so a client holding only `operator.read` could record or retract votes. It also took the scoring identity from the request, and since a score id is UUIDv5 over (trace, name, user), a caller-chosen `userId` could overwrite or retract someone else's vote. It is now a write method and attributes to the authenticated web operator; the UI never sent the field, so nothing on the wire changes. `RegistryOutboundRouter` never overrode `send_text_reporting_ids`, and every channel reaches the outbound trait through that router. It inherited the trait default, which reports no ids — so no channel ever produced a trace link and every reaction resolved as `UnknownMessage`. The router now delegates all three id-reporting sends. Trace links were stored under `outbound_to()`, which carries a `:thread` suffix for a Telegram forum topic, while a `message_reaction` update reports only the bare chat id. Links are now keyed on `chat_id`, which is what both sides can agree on. Two delivery paths never recorded a link at all: edit-in-place streaming (which delivers the reply itself, so the normal send path is skipped) and any reply carrying an activity logbook. Both now go through id-reporting sends — `send_stream_reporting_ids` and `send_text_with_suffix_reporting_ids`, additive with defaults so the channels that cannot report ids are untouched — and record the link. The reply fan-out moved into `channel_reply_delivery.rs`, both to stay under the file-size limit and so the "every path records a link" rule is checkable by reading one file. Also drops two imports left dead by the earlier file split, which the workspace `-D warnings` gate would have rejected.
Collaborator
Author
|
@greptile review |
Telegram already linked the logbook message it emits when the suffix does not fit the last chunk, but Discord dropped the equivalent embed. A reader rating "the bot's answer" may land on either, and both map back to the same turn, so linking them recovers a score rather than losing it to an UnknownMessage lookup.
The scope fix was only asserted at authorize_method. Dispatching the method with operator.read exercises the path a forging client would actually take, and pins the behavior against a future edit that moves the method back into READ_METHODS.
…lies Two delivery paths still produced replies nobody could rate. A Telegram voice reply whose transcript is too long for a caption sends the text as a follow-up message. That branch called `send_text` directly and discarded the ids, so a reaction on the readable form of the answer found no trace link. It now goes through `deliver_text_reply` like every other text reply. Slack native streaming returned an empty id list. The timestamp was there all along: `chat.startStream` reports the `ts` of the message it creates, and the code read only `stream_id` from the response. `chat.stopStream` carries it too, used as a fallback. All three Slack stream modes now report ids, so a reaction resolves the same way however the account is configured.
Collaborator
Author
|
@greptile review |
When a reply is streamed edit-in-place, the activity logbook is sent afterwards via send_html — the one message that path creates. It went out with no trace link, so a reaction on it resolved as UnknownMessage. Adds send_html_reporting_ids alongside the other id-reporting sends, implemented for Telegram (chunk ids), Discord (the embed id) and Slack (which renders no HTML and posts the markup as text), and records the link for the streamed-target logbook follow-up.
…e identity Review flagged the constant web identity as letting operators overwrite each other's scores. Moltis authenticates one account - a single password, no user table, passkeys registered as "owner" - and AuthIdentity carries only a method and scopes, so no per-person id exists to attribute to. Records that where the constant is chosen, and names the line that must change if Moltis ever grows real accounts.
Collaborator
Author
|
@greptile review |
Four branches still sent the logbook follow-up through send_html, which reports no ids: the Telegram and generic voice paths, and both already-streamed paths. On each of them the answer went out as audio or was already streamed, so the logbook is the only text message of the turn and the most likely thing a reader reacts to. All four now share deliver_logbook_follow_up, which uses the id-reporting send and records the link. This is the fourth round of the same defect appearing in a different branch, so it is now pinned rather than fixed case by case: a test scans this module and fails if any delivery code calls send_text, send_text_with_suffix or send_html directly instead of the *_reporting_ids variant. Verified the guard fails on an injected violation rather than passing vacuously.
Collaborator
Author
|
@greptile review |
Langfuse observations were exported with mutable or incomplete semantics, feedback attribution could reuse the wrong run, and provider usage/failover paths produced inconsistent telemetry. Shutdown and exporter failures could also silently lose buffered data. Export completed immutable v4 observations with canonical IDs, use the dedicated ordered Scores API, correlate feedback by run, normalize usage and fallback identity, strengthen redaction, and expose delivery health. Add cancellation-safe flushing, semantic validation, UI status, documentation, and coverage across streaming and non-streaming paths.
Update the feedback selector and settings navigation expectation for the current UI, and wait for the initial session fetch before mutating the date-bucket test store.
The targeted validator treated any Rust file below a tests directory as a Cargo integration-test target. In-source test modules such as config/src/validate/tests/instrumentation.rs therefore produced nonexistent --test targets. Match only direct crates/<crate>/tests/<target>.rs paths so nested source modules use filename filters instead.
Prevent operational exporters from leaking channel session identifiers and omit inline image bytes from generation payloads. Restore logical tool failure propagation, make provider failover deterministic, sanitize exporter configuration and errors, and reserve shutdown time for queued telemetry. Keep feedback retention and reaction authorization consistent across runtime changes, add focused regression coverage, and correct nested Rust test selection in local validation.
Collaborator
Author
|
@greptileai review |
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.
Summary
Adds backend-neutral agent instrumentation, Langfuse v4 export, operational OTLP backends, and end-user reaction feedback.
AGENTroot semantics, lowercase observation taxonomy, session/user/environment/release metadata, and structured truncation/redaction.BOOLEANreaction scores through the Scores API, including stable upsert IDs and score deletion when reactions are removed.Static local pricing remains available to Moltis, but cost is not exported to Langfuse; Langfuse infers cost from model and normalized usage metadata.
Not included
These are tracked as follow-up work rather than advertised as implemented.
Validation
Completed
./scripts/local-validate.sh 1174- all local contexts passed onab251236a, including format, Biome, TypeScript, i18n, file-size, lockfile, clippy, targeted Rust tests, workspace builds, macOS, and iOS.cargo test -p moltis-observability -p moltis-agents -p moltis-channels -p moltis-config -p moltis-gateway -p moltis-telegram -p moltis-discord -p moltis-slackcd crates/web/ui && npx playwright test e2e/specs/settings-nav.spec.js- 42 passed.cd crates/web/ui && npx playwright test e2e/specs/instrumentation.spec.js- 9 passed.cd crates/web/ui && npm run build:allcd crates/web/ui && npx tsc --noEmitcd docs && mdbook buildjust lintjust release-preflightThe full local Rust and Playwright suites were not rerun after merging main. Current repository policy reserves them for explicit
just local-validate-full; standard PR validation targets tests changed or added by the branch.Remaining
Manual QA
AGENTroot with generation/tool/retriever observations, normalized usage, model/provider attribution, and no duplicate mutable spans.user-feedbackBoolean score is attached to the matching trace. Switch to thumbs down, then clear it; confirm replacement and deletion rather than contradictory duplicate scores.