Code Review

Overview

This PR does three things: bumps the CLI HTTP timeout to 5 min for long-running KQL searches, fixes two divide-by-zero bugs in renderSpanRecordRow, and adds output-field alias resolution + exception-attribute COALESCE rewriting in the KQL parser + a synthesised stack-trace fallback on the GET /issues/:id API endpoint.

cli/CLI/Core.hs — Timeout bump

Clean. No concerns.

src/Pages/Telemetry.hs — Divide-by-zero fixes

Correct fix. The guard if listLen == 0 then 0 and if totalDuration == 0 then 0 are exactly right.

src/Pkg/Parser/Expr.hs — Parser changes

Redundant entries in flattenedOtelAttributes
attributes.exception.{type,message,stacktrace,escaped} are now in both flattenedOtelAttributes and exceptionFlattenedFields. The first guard in transformFlattenedAttribute catches them before the S.member flattenedOtelAttributes fallthrough, so their presence in flattenedOtelAttributes is required for Display Subject to dispatch into transformFlattenedAttribute at all — but this dual-registration is non-obvious. A brief comment (or a combined set) would clarify the intent.

outputFieldAliases doctests
The >>> examples look like runnable doctests, but there's no doctest runner configured. Either wire them up or drop them to stay lean.

Comment verbosity
The multi-line -- block above transformFlattenedAttribute (6 lines) and the outputFieldAliases block (5 lines) are longer than the "one line when the WHY is non-obvious" convention. The git commit message / PR description is the right home for the historical explanation.

src/Web/ApiHandlers.hs — enrichIssue / synthStackFromSpans

synthStackFromSpans reinvents Telemetry.spanServiceName
The nested 6-line case s.resource of … inside formatOne is a verbatim reimplementation of the already-exported spanServiceName :: OtelLogsAndSpans -> Maybe Text in Models.Telemetry.Telemetry. Replace with:

svc = Telemetry.spanServiceName s

spanId extraction is more verbose than needed

-- current
spanId = maybe "?" (fromMaybe "?" . (.span_id)) s.context
-- simpler
spanId = fromMaybe "?" (s.context >>= (.span_id))

storedStack/patchStack bypass the typed RuntimeExceptionData
issueData stores RuntimeExceptionData via Aeson RuntimeExceptionData. Manual JSON-key surgery is fragile; the typed round-trip is already there:

storedStack d = case AE.fromJSON (getAeson d) of
  AE.Success (r :: Issues.RuntimeExceptionData) -> r.stackTrace
  _ -> ""

patchStack s d = case AE.fromJSON (getAeson d) of
  AE.Success (r :: Issues.RuntimeExceptionData) ->
    Aeson $ AE.toJSON r{Issues.stackTrace = s}
  _ -> d

This removes the AEKM imports added solely for this purpose and makes the derivation-driven field name (stack_trace ↔ stackTrace) explicit rather than implicit.

Performance: uncached DB calls on every GET /issues/:id
enrichIssue fires up to two extra queries (getErrorPatternByHash + getSpanRecordsByTraceId) on every single issue fetch, including repeated reads of the same issue. Consider caching the synthesised stack on the issues row (e.g. update issueData in place after synthesis, behind a T.null storedStack check that short-circuits on subsequent reads) rather than re-deriving it on every request.

Summary of actionable items

Code Review

Overview

This PR adds: (1) O(1) point-in-time event lookup via a new GET /events/:id/time/:ts route, (2) client-side KQL query validation in the CLI, (3) a 5-minute HTTP timeout for long-range KQL searches, (4) divide-by-zero guards in the trace tree view, (5) exception field COALESCE rewrites in the query parser, and (6) synthetic stack trace enrichment on GET /issues/:id for RuntimeException issues.

Bug: synthStackFromSpans looks up service name incorrectly

formatOne navigates resource["service"]["name"] (nested), but the resource map stores a flat "service.name" key — as proven by the SQL resource->>'service.name' and the existing spanServiceName function:

-- Telemetry.hs:170
spanServiceName s = atMapText "service.name" (unAesonTextMaybe s.resource)

The nested lookup will always return Nothing, so every span in the synthesised trace will show no service label. Use the exported helper instead:

-- current: wrong key navigation, duplicates spanServiceName
svc = case s.resource of
  Just (AesonText m) ->
    case Map.lookup "service" m of
      Just (AE.Object o) -> case AEKM.lookup "name" o of ...

-- fix: one line, already exported from Models.Telemetry.Telemetry
svc = spanServiceName s

Performance: enrichIssue adds eager DB queries to every issue-detail call

Every GET /issues/:id for a RuntimeException with an empty stored stack now hits two extra queries (getErrorPatternByHash + getSpanRecordsByTraceId). For a busy project this could add hundreds of ms of latency to what was previously a single-row lookup. Consider:

• Computing and storing the synthetic stack at ingestion/upsert time rather than on every read.
• Or at minimum, short-circuiting earlier — getErrorPatternByHash is called unconditionally for every qualifying issue even before checking recentTraceId.

Inconsistency: validateQueryOrDie only wired to search, not tail/context

Applied in validateEventsOpts (used by events search) but EventsTailOpts and EventsContextOpts also carry a query field with their own validation paths. A bad KQL expression in events tail would still reach the server and produce an opaque HTTP error rather than a clear client-side message.

Minor issues

outputFieldAliases in SELECT vs WHERE context — Display Subject rewrites span_name -> name unconditionally. In a SELECT this emits the raw column name without the alias, so query results return a column named name rather than span_name. If the map is only intended for WHERE rewrites, the SELECT rendering path needs a carve-out.

Orphaned comment in ApiHandlers.hs — the existing block comment for apiFacets now sits detached above apiEventGet with no blank line separating it from the new -- | haddock. Move the apiFacets comment to sit immediately above apiFacets.

exceptionFlattenedFields vs flattenedOtelAttributes duplication — the four exception field names appear in both sets. A comment clarifying the invariant ("these must overlap") would prevent silent drift.

What's good

• The COALESCE rewrite for exception fields is well-motivated and correctly ordered before the plain-attribute guard.
• The 5-minute timeout with a clear rationale comment is a good ops improvement.
• The divide-by-zero guards in renderSpanRecordRow are a clean, minimal fix.
• iso8601ParseM . toString for --at parsing is idiomatic.
• The encodeUtf8 / error message fix in Charts.hs corrects a latent show-on-Text bug and improves the copy.

Code Review

Overview

This PR adds: (1) O(1) point-in-time event lookup via a new GET /api/v1/events/{id}/time/{ts} route + --at CLI flag, (2) client-side KQL validation in the CLI, (3) a 5-minute HTTP timeout for slow searches, (4) a synthesised stack-trace enrichment for RuntimeException issues, (5) OTel exception-field COALESCE rewriting to cover span-event-based SDKs, (6) divide-by-zero guards in the trace table renderer, and (7) an output-field alias map for span_name/service/trace_id.

Bug: silent fallback on invalid --at timestamp

-- cli/CLI/Commands.hs:402
case opts.at >>= iso8601ParseM . toString of
  Just (t :: UTCTime) -> ...
  Nothing -> ...   -- also taken when --at is present but unparseable

If the user passes --at not-a-date, iso8601ParseM returns Nothing and the code silently falls through to a 90-day range scan. The user gets a slow result with no error. The fix is to validate eagerly:

case opts.at of
  Nothing -> -- fallback path
  Just raw -> case iso8601ParseM (toString raw) of
    Nothing -> printError "--at: invalid ISO-8601 timestamp" >> liftIO exitFailure
    Just (t :: UTCTime) -> -- fast path

Broken Haddock on validateEventsOpts

Two consecutive -- | markers are stacked — Haddock treats the second as a new, orphaned doc block rather than continuing the first:

-- | Validate every flag …
-- 'normalizeKind' … (D2). Failures … (D5).
-- | Validate search options …   ← should be plain --

Change the second -- | to --.

enrichIssue: ad-hoc JSON surgery instead of the typed record

storedStack/patchStack pattern-match on raw AE.Object while RuntimeExceptionData already has a typed stack_trace field with FromJSON/ToJSON. If the schema grows or a field is renamed, these helpers silently return "" and no compiler warns. Prefer decoding via the existing type:

case AE.fromJSON (getAeson issue.issueData) of
  AE.Success (rd :: RuntimeExceptionData)
    | T.null rd.stackTrace -> ... patch via rd{stackTrace = synth} ...
  _ -> pure issue

Minor: exceptionFlattenedFields is manually mirrored from flattenedOtelAttributes

The comment says "every member here must also appear under attributes.exception.* in flattenedOtelAttributes" — but this invariant is invisible to the compiler. Derive it instead:

exceptionFlattenedFields :: Set T.Text
exceptionFlattenedFields =
  S.fromList
    . mapMaybe (T.stripPrefix "attributes.exception.")
    . S.toList
    $ flattenedOtelAttributes

Zero duplication, invariant enforced automatically.

Minor: redundant null formatted check in synthStackFromSpans

formatted = map formatOne ordered is non-empty iff spans is non-empty. The guard if null formatted can be moved earlier to avoid the sortOn/map allocation entirely:

synthStackFromSpans _ [] = ""
synthStackFromSpans trId spans = ...

Minor: foldMap over Maybe for svcTxt

svcTxt = maybe "" (\v -> " [" <> v <> "]") svc
-- cleaner:
svcTxt = foldMap (\v -> " [" <> v <> "]") svc

Maybe has Foldable, so foldMap reads more uniformly with the rest of the codebase's style.

No new tests

The KQL alias rewriting (outputFieldAliases), exception COALESCE rewriting, and synthStackFromSpans are all pure functions. Unit tests for the first two in test/unit/Pkg/ParserSpec.hs and a standalone test for synthStackFromSpans would cover the key invariants cheaply.

Good things

• The 90d/24H lookback change is the right default for trace IDs that may be hours old.
• The COALESCE rewrite for hs-opentelemetry span events is correct and nicely scoped.
• The divide-by-zero guards in renderSpanRecordRow are well-placed.
• The encodeUtf8 fix in Charts.hs (replacing show err which double-quoted the error message) is a solid correctness fix.
• The 5-minute timeout rationale is clear and the implementation is clean.

Code Review

Overview

This PR adds: (1) a fast O(1) point-in-time event lookup (GET /events/{id}/time/{ts} + --at CLI flag), (2) client-side KQL validation before sending to the server, (3) exception-attribute COALESCE rewriting to catch OTel SDK span-event exceptions, (4) output-field alias rewriting (span_name→name, service→resource___service___name, etc.), (5) a synthesised stack-trace fallback for empty-stacktrace RuntimeException issues, and (6) a divide-by-zero guard in the span table renderer.

Bugs / Correctness

Non-UUID event IDs rejected silently on the --at path (Web/Routes.hs, CLI/Commands.hs)

The new route uses Capture "event_id" UUID.UUID, but the old code explicitly noted that event IDs are not always UUIDs ("synthetic-… spans, integer log_pattern ids, etc."). When a user passes a non-UUID id with --at, Servant returns a 400 at the routing layer with no helpful message. Either change the capture type to Text and parse/validate in the handler, or document the restriction and have the CLI reject it early with a clear message before building the URL.

-- Routes.hs — current
:> Capture "event_id" UUID.UUID   -- rejects non-UUID IDs silently

-- CLI/Commands.hs — current (no pre-flight check)
let path = "/api/v1/events/" <> opts.eventId <> "/time/" <> ...

Performance / Risk

since = "90d" fallback range scan (CLI/Commands.hs:83)

The fallback was "24H" and is now "90d" — a 90× expansion. The comment justifies it for bare trace IDs, but this is the path taken every time --at is not supplied (i.e., almost all current usage). For busy projects this will be a significant regression. Worth either keeping the default small and adding a dedicated --since override, or gating the 90d window on opts.showTree only (where the trace-id match is the whole point).

Two extra queries per apiIssueGet call (ApiHandlers.hs)

The acknowledged TODO is fine, but worth noting: enrichIssue runs getErrorPatternByHash + getSpanRecordsByTraceId on every GET /issues/{id} for any RuntimeException issue without a stored stack trace. At scale this will be felt. The suggested fix (compute at ingestion) is the right direction.

5-minute timeout applied globally (CLI/Core.hs:123)

reqOpts is used for every CLI request — login, project list, short gets, etc. A 5-minute wall-clock wait on a failed auth request will be confusing. Consider threading a per-call timeout, or at minimum only bumping it for search/tail operations.

Code Succinctness / Package Usage

exceptionFlattenedFields — use S.mapMaybe (Pkg/Parser/Expr.hs:656)

Data.Set.mapMaybe (containers ≥ 0.6.3) avoids the toList/fromList round-trip:

-- current (4 lines, extra toList/fromList)
exceptionFlattenedFields =
  S.fromList
    . mapMaybe (T.stripPrefix "attributes.exception.")
    . S.toList
    $ flattenedOtelAttributes

-- simpler
exceptionFlattenedFields = S.mapMaybe (T.stripPrefix "attributes.exception.") flattenedOtelAttributes

Defensive / Error Handling

enrichIssue silently swallows JSON decode failure (ApiHandlers.hs:456)

_ -> pure issue   -- AE.Error case: no log, no signal

If issueData fails to decode as RuntimeExceptionData for a RuntimeException issue, the caller gets the issue back unchanged with no indication anything went wrong. A printDebug/logWarn here would surface schema drift early.

Minor Style Notes

• The multi-paragraph block comment on outputFieldAliases (the EXTEND/summarize edge-case analysis, ~8 lines) and the enrichIssue comment are longer than the project norm; the essential point fits in one line each.
• The -- Note: EventsTailOpts and EventsContextOpts do not carry… comment in validateEventsOpts explains why code doesn't exist — useful to some readers, but borderline given the project's minimal-comment preference.
• flattenedOtelAttributes uses bare fromList (OverloadedLists) while exceptionFlattenedFields uses qualified S.fromList. Consistent either way is fine, but pick one.

What's Good

• The COALESCE rewriting for attributes.exception.* is a solid fix for the hs-opentelemetry span-event path; deriving exceptionFlattenedFields from flattenedOtelAttributes keeps them in sync automatically.
• validateQueryOrDie gives users immediate, readable feedback before a slow round-trip.
• The synthStackFromSpans tests are thorough and cover the edge cases (empty list, missing name/span_id/service, ordering).
• The divide-by-zero guard in renderSpanRecordRow is a correct fix.
• The encodeUtf8 fix in Charts.hs (removing the stray show that was quoting the error string) is a clean correction.

Code Review

Overview: This PR adds a fast O(1) event point-in-time lookup via --at flag, client-side KQL validation, a synthesized stack-trace fallback for Haskell exceptions, exception-field COALESCE rewriting in the KQL→SQL transpiler, field alias rewrites (span_name, service, trace_id), a divide-by-zero fix in renderSpanRecordRow, and a bumped HTTP timeout in the CLI.

Bugs fixed correctly

• Divide-by-zero guards in renderSpanRecordRow (avgDuration, pctOfTotal) — good fix.
• encodeUtf8 wrapping of the Charts error body was missing — good catch.

Reuse existing helpers

apiEventGet reimplements notFoundOr — there's already a helper in this file that does exactly this:

-- current
apiEventGet pid eid ts = do
  mItem <- Telemetry.logRecordByProjectAndId pid ts eid
  case mItem of
    Nothing -> throwError err404{errBody = encodeUtf8 ("event not found" :: Text)}
    Just item -> pure (AE.toJSON item)

-- simpler, uses existing notFoundOr
apiEventGet pid eid ts =
  AE.toJSON <$> (notFoundOr "event not found" =<< Telemetry.logRecordByProjectAndId pid ts eid)

validateQueryOrDie ignores validateOrDie — the module already exports validateOrDie :: IOE :> es => Either Text a -> Eff es a. The new function can leverage it and shed the explicit case:

validateQueryOrDie q
  | T.null (T.strip q) = pure ()
  | otherwise =
      validateOrDie $ first ("invalid query: " <>) $ void (parseQueryToAST q)

Succinctness: exceptionFlattenedFields

The S.toList → mapMaybe → S.fromList round-trip allocates an intermediate list. foldMap over the Set avoids it:

-- current
exceptionFlattenedFields =
  S.fromList
    . mapMaybe (T.stripPrefix "attributes.exception.")
    . S.toList
    $ flattenedOtelAttributes

-- shorter, no intermediate list
exceptionFlattenedFields =
  foldMap (\t -> maybe mempty S.singleton (T.stripPrefix "attributes.exception." t))
    flattenedOtelAttributes

Potential bug: opts.eventId in --at path is unescaped

In the range-scan path the event id is carefully escaped:

let eid = T.replace "\"" "\\\"" (T.replace "\\" "\\\\" opts.eventId)

But in the --at direct path it goes raw into the URL:

let path = "/api/v1/events/" <> opts.eventId <> "/time/" <> ...

A / or ? in eventId would silently corrupt the request. Apply the same escaping (or percent-encode it) before building path.

Minor style notes

• at :: Maybe Text vs Maybe UTCTime — parsing is deferred to runEventsGet, which is inconsistent with other flags that validate at the parser level. An eitherReader in eventsGetParser would catch typos earlier and give optparse-applicative's standard error formatting. Low priority but worth noting for consistency.

• Comment on what the code doesn't do — the block explaining why EventsTailOpts and EventsContextOpts don't call validateQueryOrDie adds noise. The absence of a call needs no justification in a comment.

• synthStackFromSpans export for testing — consider whether the function is truly part of the public API or just exposed for unit tests. If the latter, a -- | Exposed for testing note is conventional; some codebases prefer an internal test module instead.

Test coverage

The new Web.ApiHandlersSpec and the parser alias/COALESCE tests are well-structured and cover the expected edge cases. The ordering test's "longer remaining tail" approach is a little fragile (breaks if the header happens to contain "earlier"/"later"); a T.isPrefixOf check on each line or lines-based indexing would be more robust.

Summary

Good PR overall — the feature set is cohesive and the divide-by-zero fix is important. The three most actionable items are: (1) use notFoundOr in apiEventGet, (2) use validateOrDie inside validateQueryOrDie, and (3) URL-escape opts.eventId in the --at path.

Code Review

Overview

This PR adds three main things: (1) an O(1) GET /events/{id}/time/{ts} route with a matching --at CLI flag, (2) exception COALESCE rewriting in the KQL parser so OTel SDK-style span-event exceptions aren't missed, and (3) a synthStackFromSpans fallback for Haskell issues with empty stack traces. Also includes two correctness fixes (divide-by-zero in trace view, error body encoding in charts) and an MCP server doc.

Bug: test fixture uses wrong resource structure

In test/unit/Web/ApiHandlersSpec.hs, mkSpan builds the resource field as:

resource = (\svc -> AesonText (Map.singleton "service" (AE.Object (AEKM.singleton "name" (AE.String svc))))) <$> service

But spanServiceName (and atMapText) does a flat key lookup for "service.name" in the map. The nested structure {"service": {"name": "…"}} will never match — service names are silently Nothing in every test. There's no test asserting that [api] actually appears in the output when service is provided, so the bug is hidden.

Fix: use Map.singleton "service.name" (AE.String svc) in the fixture, then add a test case that asserts [api] appears in the formatted line.

Conciseness opportunities

synthStackFromSpans — nested maybe can be flattened

-- current
spanId = maybe "?" (fromMaybe "?" . (.span_id)) s.context

-- shorter, uses Monad Maybe
spanId = fromMaybe "?" (s.context >>= (.span_id))

enrichIssue — long TODO comment

The 4-line TODO block is the kind of thing that belongs in a ticket/PR description, not inline. A single-line -- TODO(perf): store synth stack at ingestion to avoid these 2 extra queries is sufficient given the project's preference for minimal comments.

formatOne service suffix

svcTxt = foldMap (\v -> " [" <> v <> "]") svc

foldMap on Maybe here means "if Nothing → mempty, if Just v → f v". That's idiomatic — no issue, just noting it's intentional.

Performance

enrichIssue runs two extra queries (getErrorPatternByHash + getSpanRecordsByTraceId) on every apiIssueGet call for RuntimeException issues with empty stacks. The TODO comment acknowledges this. Worth confirming that no list endpoint calls enrichIssue in a loop — if apiIssuesList ever hydrates full issues, this becomes an N+1.

Potential path injection (low risk)

In runEventsGet:

let path = "/api/v1/events/" <> opts.eventId <> "/time/" <> toText (iso8601Show t)

opts.eventId is raw user-supplied Text. A value containing / would silently alter the path. Risk is minimal (self-injection in a CLI tool), but URL-encoding or rejecting / in the event ID would be safer.

Minor

• docs/mcp.md is missing a trailing newline.
• The outputFieldAliases doctest uses M.! — worth checking the $setup block covers that import, though it's likely fine since it's a module-level let binding.
• validateQueryOrDie is not directly unit-tested; it's implicitly exercised through validateEventsOpts. A small property test (valid KQL → no exit, invalid KQL → error) would close that gap.

What's good

• The COALESCE rewriting is derived from flattenedOtelAttributes rather than maintained separately — that's the right call.
• The Display Subject instance correctly checks outputFieldAliases before flattenedOtelAttributes, so aliases take priority.
• Parser tests for both alias rewriting and COALESCE paths are solid.
• The divide-by-zero and error-body encoding fixes are correct.
• Bumping the HTTP timeout to 5 min (from 30 s) in reqOpts is well-justified and the comment explains why.

Code Review

Overview

This PR adds MCP server documentation, a new GET /events/{id}/time/{ts} O(1) lookup endpoint, client-side KQL validation in the CLI, stack-trace synthesis for GHC runtime exceptions, output-field alias rewriting (span_name/service/trace_id), COALESCE rewriting for attributes.exception.*, and divide-by-zero guards in the span table renderer. Good scope overall.

Issues

1. Global 5-minute timeout (CLI/Core.hs)
reqOpts sets the timeout for all HTTP calls, including the new O(1) --at path. A genuine server hang on that fast path now takes 5 minutes to surface instead of 30s. Consider a separate reqOptsLong for KQL searches, or pass the timeout as a parameter.

2. exceptionFlattenedFields is redundant (Pkg/Parser/Expr.hs)
exceptionFlattenedFields derives from flattenedOtelAttributes by stripping "attributes.exception.". The guard field \S.member` exceptionFlattenedFieldsintransformFlattenedAttributeis logically equivalent toentire `S.member` flattenedOtelAttributes— since every entry was derived from there. The wholeexceptionFlattenedFields` definition (~7 lines) can be dropped, and the guard simplified to:

transformFlattenedAttribute entire
  | entire `S.member` flattenedOtelAttributes
  , Just field <- T.stripPrefix "attributes.exception." entire =
      "COALESCE(..." <> field <> ...
  | entire `S.member` flattenedOtelAttributes = T.replace "." "___" entire
  ...

3. synthStackFromSpans mixes spans and log records (Web/ApiHandlers.hs)
Telemetry.getSpanRecordsByTraceId returns [OtelLogsAndSpans], which may include log records that don't carry status_code. Those get rendered without !! even when they're error-level logs, and they'll appear in the synthesized "stacktrace" as pseudo-frames, which is misleading. Consider filtering to spans only (kind /= Nothing or explicit kind check), or document this behavior.

4. enrichIssue adds 2 extra queries on every apiIssueGet call
The TODO acknowledges this, but it runs on the hot issue-get path for every RuntimeException with an empty stack trace. Please open a ticket — the comment implies it's a known cost, but without a tracker item it'll stay forever.

5. Path concatenation safety (CLI/Commands.hs)

| T.any (== '/') opts.eventId -> printError "event id must not contain '/'" >> liftIO exitFailure

/ is guarded, but ?, #, and % are not. The server will reject malformed paths, but a user typo like abc?foo=1 produces a confusing error rather than a helpful client-side message. Worth expanding the validation or percent-encoding the id instead.

Minor

• Ordering test in ApiHandlersSpec (line ~836): checking T.length (snd (T.breakOn ...)) to infer position is fragile. T.lines out and elemIndex would be clearer and more robust.

• reqOpts uses record update on tlsManagerSettings (tlsManagerSettings{HC.managerResponseTimeout = ...}): this is correct but the named-field update on a record imported from another package is opaque. A short comment pointing at managerResponseTimeout docs or the default value would help future readers.

• "24H" → "90d" change in runEventsGet: the old value was "24H" (uppercase H), the new is "90d". Make sure the backend duration parser is case-insensitive or that H is no longer used anywhere — if it is, the format change could silently produce a 400.

• Charts.hs error message change from "Invalid signature; " <> show err (where show would double-quote the text) to encodeUtf8 $ "Invalid query: " <> err — good cleanup.

What's good

• Client-side KQL parse-before-send (validateQueryOrDie) is the right UX move.
• exceptionFlattenedFields deriving from flattenedOtelAttributes keeps the two lists in sync automatically — good DRY principle (even if the set itself is redundant).
• synthStackFromSpans is well-tested with 7 targeted cases.
• Divide-by-zero guards in renderSpanRecordRow are correct and the extracted let bindings make the render expressions cleaner.
• MCP docs are clear and the curl examples are immediately runnable.

Code Review

Overview

This PR adds MCP server documentation, a fast O(1) events get --at CLI lookup, client-side KQL validation, synthesised stack traces for runtime-exception issues, divide-by-zero guards in the spans view, and the outputFieldAliases + COALESCE exception-field rewriting in the query parser. Good scope of work with solid test coverage.

Issues & Suggestions

1. transformFlattenedAttribute — double set-membership check

-- current
transformFlattenedAttribute entire
  | entire `S.member` flattenedOtelAttributes
  , Just field <- T.stripPrefix "attributes.exception." entire = …coalesce…
  | entire `S.member` flattenedOtelAttributes = T.replace "." "___" entire

The S.member call is repeated. When the first guard matches the set but the pattern guard fails (a non-exception attribute), it re-checks. Collapse to one guard + case:

transformFlattenedAttribute entire
  | entire `S.member` flattenedOtelAttributes =
      case T.stripPrefix "attributes.exception." entire of
        Just field -> "COALESCE(attributes___exception___" <> field <> …
        Nothing    -> T.replace "." "___" entire
  | entire == "url_path" = "attributes___url___path"
  | otherwise = entire

2. CLI/Core.hs — redundant inline comment in reqOpts

The block comment immediately above reqOpts already explains the 5-minute timeout. The inline -- Override http-client's 30s default… repeats it verbatim. Drop one.

3. apiEventGet route captures UUID.UUID — inconsistent with CLI

The server route is:

"events" :> Capture "event_id" UUID.UUID :> "time" :> Capture "timestamp" UTCTime

But the existing code (and the CLI docs) note that event IDs are not always UUIDs (synthetic spans, integer log-pattern IDs, etc.). The CLI builds the path from opts.eventId :: Text and validates against /?#%, so a non-UUID eventId will produce a 404 or a Servant parse error rather than a useful message. Either narrow the CLI --at help text to "UUID events only" or widen the capture to Text (with server-side UUID parsing/validation).

4. enrichIssue — silent AE.Error case

| otherwise = case AE.fromJSON (getAeson issue.issueData) of
    AE.Success (rd :: Issues.RuntimeExceptionData) | T.null rd.stackTrace -> …
    _ -> pure issue   -- decode failure or non-empty stack: silently pass through

A decode failure is swallowed with no log line. A single printDebug/logDebug on the AE.Error msg branch would make diagnosing "why isn't the synth stack appearing?" much easier without adding any complexity.

5. getNestedValue — semantics changed but name/doc unchanged

getNestedValue ks@(k : rest) m =
  Map.lookup (T.intercalate "." ks) m <|> do …

The function now tries the whole dotted key as a flat lookup first, then falls back to recursive descent. The existing callers split on . before calling (T.split (== '.') key), so a key like "service.name" is passed as ["service","name"] and now tries the flat key "service.name" first — which is the intended fix. But the function name/docs don't reflect this new contract. A one-line note (-- tries flat dotted key first, then descends) would clarify the intent for future readers.

6. synthStackFromSpans — exported purely for testing

synthStackFromSpans is exported from Web.ApiHandlers only to make it testable. Consider moving it (and enrichIssue) to a small internal helper module (e.g. Web.ApiHandlers.Enrich) so the main module's export surface stays tight and the function doesn't need to carry an explicit "Internals exposed for testing" comment.

Smaller nits

• validateQueryOrDie comment (CLI/Validate.hs:197): "Empty queries pass through — the server accepts them." is fine but T.null (T.strip q) also passes through whitespace-only queries. Worth matching the comment: "blank or whitespace-only queries pass through".

• reqOpts / timeout value: 5 * 60 * 1_000_000 is clear. Binding it to a named constant (fiveMinutesMicros) would avoid the unlabelled arithmetic if reused, but at a single call site this is fine as-is.

• at field type in EventsGetOpts: Keeping it as Maybe Text and parsing later (rather than Maybe UTCTime via a custom ReadM) is pragmatic — fine.

• Test coverage: The new Web/ApiHandlersSpec covers the key edge cases of synthStackFromSpans well. The ParserSpec additions for alias rewriting and COALESCE are thorough.

Summary

The core logic is sound. The main items worth addressing before merge: (3) the UUID/non-UUID mismatch between the CLI flag and the route, (1) the double set-membership check in transformFlattenedAttribute, and (4) the silent decode failure in enrichIssue. The rest are low-priority nits.

Code Review

Overview

This PR adds an MCP server documentation layer, a --at flag for fast point-in-time event lookups, client-side KQL validation, exception COALESCE rewriting, output field aliasing (span_name, service, trace_id), a synthesised stacktrace fallback for runtime-exception issues, and a divide-by-zero guard in the span stats renderer. Good set of fixes overall, with a few actionable items below.

Bug: false-positive test in Web/ApiHandlersSpec.hs

-- line ~847
idx "earlier" `shouldSatisfy` (\e -> e < idx "later")

idx returns Maybe Int, and Nothing < Just n is True in Haskell's Ord (Maybe a) instance. If "earlier" is absent from the output lines, the assertion still passes. Fix with an explicit unwrap or liftA2 (<):

(liftA2 (<) (idx "earlier") (idx "later")) `shouldBe` Just True

Subtle semantic change in getNestedValue (Telemetry.hs)

getNestedValue ks@(k : rest) m =
  Map.lookup (T.intercalate "." ks) m <|> do

This now gives priority to a flat dotted key (e.g. "a.b.c") over recursive object descent. Any JSON document that has both a top-level key "a.b" and a nested {"a": {"b": …}} will silently return the flat key's value rather than the nested value — opposite of the old behaviour. Is this intentional for all call-sites? Worth a targeted test or an explicit note in the comment.

enrichIssue — hot-path extra queries

apiIssueGet pid iid = issueToFull <$> (enrichIssue pid =<< fetchIssue pid iid)

enrichIssue fires up to two extra DB queries on every RuntimeException issue fetch with an empty stack trace. The TODO is noted but this is now a regression on the latency of every such GET /issues/{id} call. Consider gating behind a query-param flag (?enrich_stack=true) until the ingest-time path is ready.

Conciseness: validateQueryOrDie pattern guard

With MultiWayIf / PatternGuards already enabled, the nested case can be flattened:

-- current
validateQueryOrDie q
  | T.null (T.strip q) = pure ()
  | otherwise = case parseQueryToAST q of
      Right _ -> pure ()
      Left err -> printError ("invalid query: " <> err) >> liftIO exitFailure

-- more concise
validateQueryOrDie q
  | T.null (T.strip q) = pure ()
  | Left err <- parseQueryToAST q = printError ("invalid query: " <> err) >> liftIO exitFailure
  | otherwise = pure ()

Redundant inline comment in reqOpts (Core.hs)

The Haddock above reqOpts already explains the timeout rationale. The inline comment on the & line repeats it verbatim — one should go.

Minor nits

• "/?#%" :: [Char] — :: String is the conventional spelling; [Char] is unusual enough to read as accidental.
• outputFieldAliases comment says | summarize … by span_name is an "acceptable trade-off" — a -- TODO would make it easier to track rather than leaving it in a block comment that future readers will skip.
• synthStackFromSpans header string is 110+ chars inline; a where headerLine = … would keep line lengths reasonable.
• fromRight' in test assertions panics with no context on failure; shouldBe (Right …) or shouldSatisfy isRight before unwrapping gives a better failure message.

What's good

• The divide-by-zero guard in renderSpanRecordRow is clean and the where bindings are a nice readability improvement.
• The COALESCE rewrite for attributes.exception.* over span events is well-motivated and the pattern guard chain in transformFlattenedAttribute is idiomatic.
• synthStackFromSpans is a good use of sortOn, foldMap on Maybe, and field-dot syntax throughout.
• Test coverage for the alias rewriting and COALESCE paths is solid.

Code Review — PR #368 (Improve MCP and CLI)

Overview

This PR adds: (1) an O(1) GET /api/v1/events/{id}/time/{ts} endpoint + CLI --at flag; (2) client-side KQL validation in the CLI; (3) output-field alias rewriting (span_name→name, service→resource___service___name, trace_id→context___trace_id); (4) COALESCE-based exception attribute lookup over span events; (5) synthesised stack traces for RuntimeException issues; (6) MCP server docs. Good breadth; several things worth tightening.

Bugs / Correctness

enrichIssue uses the wrong hash field
ApiHandlers.hs calls ErrorPatterns.getErrorPatternByHash pid issue.targetHash, but the analogous code in Pages/Anomalies.hs:968 uses issue.endpointHash. These are different fields; one of them is wrong, or the comment explaining why they differ is missing.

synthStackFromSpans ordering test can silently pass

-- test/unit/Web/ApiHandlersSpec.hs
idx needle = viaNonEmpty head [i | (i, l) <- zip [0 :: Int ..] ls, needle `T.isInfixOf` l]
shouldSatisfy (\e -> e < idx "later")

idx returns Maybe Int. Haskell's Ord (Maybe a) has Nothing < Just _, so if "earlier" is absent from the output idx "earlier" = Nothing and the assertion trivially passes. Use fromJust (with a clear failure message) or shouldBe on the concrete indices.

enrichIssue passes Nothing for the time hint
Telemetry.getSpanRecordsByTraceId pid trId Nothing now does a full-range scan. The analogous Anomalies page code passes Just traceStartTime to bound the query. The error pattern's recentTraceId doesn't carry a start time, but the span we're enriching from is a RuntimeException issue, so the trace timestamp is likely available via the issue itself. Without the hint this can be slow on large projects.

Performance

enrichIssue adds 2 DB round-trips to every GET /issues/{id}
The TODO(perf) comment acknowledges this. Given it only fires for RuntimeException issues with an empty stack trace it is tolerable short-term, but it should be tracked. Worth opening a follow-up issue if one doesn't exist.

getNestedValue now calls T.intercalate "." ks at every recursive level
For a path of depth n this is O(n²) string allocations. Fine for typical 3–4-level OTel paths, but worth noting: the flat-key probe could be moved to the call site (or cached as an argument) so it's only done once, not at each tail-recursive step.

Succinctness / Style

reqOpts has a duplicated comment
The Haddock block says "Bumps response timeout to 5 minutes" and the inline comment says "Override http-client's 30s default; long-running aggregates legitimately exceed it." — these say the same thing. Drop the inline one.

enrichIssue deeply nests case/guard inside case

| otherwise = case AE.fromJSON (getAeson issue.issueData) of
    AE.Success (rd :: Issues.RuntimeExceptionData)
      | T.null rd.stackTrace -> do ...
    _ -> pure issue

The outer guard + inner pattern match + inner guard can be flattened with a local do-let or by using maybe/guard from Control.Monad. Not a blocker, just a readability note given succinctness is a priority.

synthStackFromSpans — foldMap on Maybe is non-obvious

svcTxt = foldMap (\v -> " [" <> v <> "]") svc

foldMap over Maybe is idiomatic Haskell but maybe "" (\v -> " [" <> v <> "]") svc makes the default explicit and is more legible for readers less familiar with the Foldable Maybe instance. Minor preference.

Long --at option help text in Main.hs

optional (strOption (long "at" <> metavar "TIMESTAMP" <> help "ISO-8601 timestamp for a fast point-in-time lookup (avoids 90d range scan)"))

The parenthetical implementation detail ("avoids 90d range scan") is user-visible in --help. Users don't care about the scan strategy; the help text could just say "ISO-8601 timestamp; skips the default 90-day range scan".

Minor / Nits

• cli/CLI/Core.hs: reqOpts now imports both Network.HTTP.Client and Network.HTTP.Client.TLS. The comment in the Haddock block ("long-running aggregates") doesn't match the change context (it's also affecting searches, not just metrics). Update the doc.
• docs/mcp.md:252: protocolVersion: "2025-06-18" — if this is a real MCP spec date it's fine; just confirm it's not a placeholder.
• The isnotnull(trace_id) test update (ParserSpec) is correct — trace_id now rewrites to context___trace_id via outputFieldAliases — good catch updating the expected SQL.
• Exception COALESCE tests are comprehensive and well-structured. ✓
• synthStackFromSpans unit tests cover all the edge cases (empty list, no name, no context, sort order). ✓

Summary

The core logic is sound. The two actionable bugs are the targetHash vs endpointHash discrepancy (potential wrong-data bug) and the ordering test false-pass via Nothing < Just (test correctness). The missing time hint in enrichIssue is a performance concern worth fixing before it bites on a large dataset.