Code Review

This PR replaces random UUID v4 row IDs with deterministic UUID v5 IDs derived from OTel content (span path: project_id + trace_id + span_id; log path: project_id + body + name + severity + attributes + resource). The UUIDEff effect is correctly dropped from the entire ingest stack since mintOtelLogIds is now pure. Test coverage for the new ID scheme is solid — 7 cases covering determinism, span collapse despite attribute drift, timestamp exclusion, and resource-based distinction.

Findings

1. otelIdNamespace — use the [uuid|...|] quasi-quoter already in the project (Telemetry.hs:834)

-- current
otelIdNamespace = fromMaybe UUID.nil (UUID.fromString "6f1a7c30-9b2d-5e84-8a3f-0c1d2e3f4a5b")

fromMaybe UUID.nil is permanently dead code (the string is a valid UUID), but it creates a dangerous silent fallback: if someone ever edits the string to an invalid value, all derived IDs silently migrate to the nil-namespaced hash with no compile error, no log, no test failure. The project already ships uuid-quasi in the library's build-depends and uses it in integration tests:

import Data.UUID.Quasi (uuid)

otelIdNamespace :: UUID.UUID
otelIdNamespace = [uuid|6f1a7c30-9b2d-5e84-8a3f-0c1d2e3f4a5b|]

This removes fromMaybe/UUID.nil entirely and makes a malformed namespace a compile-time error.

2. Redundant Text/ByteString roundtrip in deterministicOtelId (Telemetry.hs:853–856)

The current pipeline is:

AE.encode → lazy ByteString
  → decodeUtf8 (enc helper) → Text
  → T.intercalate "\US" → Text (joined)
  → encodeUtf8 → ByteString
  → BS.unpack → [Word8]

enc decodes to Text only so T.intercalate can join everything, then encodeUtf8 immediately re-encodes. The Text step is pure waste. Work in ByteString throughout — Data.ByteString is already imported:

deterministicOtelId r =
  UUID.toText $ UUIDv5.generateNamed otelIdNamespace $ BS.unpack $
  BS.intercalate "\US" keyParts
  where
    enc :: AE.ToJSON a => Maybe a -> BS.ByteString
    enc = maybe "" (BSL.toStrict . AE.encode)
    keyParts = case r.context >>= (.span_id) of
      Just sid | not (T.null sid) ->
        [encodeUtf8 r.project_id, encodeUtf8 $ fromMaybe "" (r.context >>= (.trace_id)), encodeUtf8 sid]
      _ ->
        [ encodeUtf8 r.project_id, enc (unAesonTextMaybe r.body)
        , encodeUtf8 $ fromMaybe "" r.name, enc r.severity
        , enc (unAesonTextMaybe r.attributes), enc (unAesonTextMaybe r.resource) ]

Requires adding import Data.ByteString.Lazy qualified as BSL. The impact grows with payload size — large body/attributes/resource blobs currently allocate a joined Text string on every row, only to immediately re-encode it.

3. Scope note — ProcessMessage.hs legacy Pub/Sub path is not covered (ProcessMessage.hs:180–191)

processMessages still uses UUID.genUUID to mint the row id (and span_id) for legacy Pub/Sub spans, then calls insertAndHandOff directly without going through mintOtelLogIds. On DLQ replay of that path, new random UUIDs would be generated and TF would append fresh rows — the same dedup failure this PR fixes for the Kafka/OtlpServer path. The PR description's scope note alludes to this, but worth tracking explicitly so it's not forgotten.

Overall the fix is correct and well-reasoned. The three points above are independent improvements — #1 and #2 can be applied inline, #3 is a follow-up ticket.

Code Review

This PR replaces random UUIDv4 generation with deterministic UUIDv5 content-derived IDs, so DLQ replays of the same payload re-derive the same row ID and TimeFusion's `(id, timestamp)` dedup collapses retries instead of appending fresh rows. Removing `UUIDEff` from the ingest path and making `mintOtelLogIds` pure is the right structural simplification.

Bug: separator injection in key derivation

src/Models/Telemetry/Telemetry.hs lines 861–862

`project_id` and `name` are raw `encodeUtf8` without escaping the `\x1F` (Unit Separator) byte used as the delimiter. Two distinct records can hash to the same id:

project_id = "x\x1f",  body = Nothing,  name = "y"
 → intercalated bytes: "x\x1f" ++ "\x1f" ++ "" ++ "\x1f" ++ "y" ... = "x\x1f\x1f\x1fy..."

project_id = "x",      body = Nothing,  name = "\x1fy"
 → intercalated bytes: "x" ++ "\x1f" ++ "" ++ "\x1f" ++ "\x1fy" ... = "x\x1f\x1f\x1fy..."

Same bytes → same UUID → cross-project log dedup — the exact class of bug this PR is meant to eliminate.

The `enc` helper already solves this for `body`/`severity`/`attributes`/`resource` (Aeson escapes `\x1F` as `\u001f` inside JSON strings). Apply the same treatment to `project_id` and `name`:

-- log path:
[ enc (Just r.project_id)
, enc (unAesonTextMaybe r.body)
, enc r.name
, enc r.severity
, enc (unAesonTextMaybe r.attributes)
, enc (unAesonTextMaybe r.resource)
]
-- span path:
[ enc (Just r.project_id)
, enc (r.context >>= (.trace_id))
, enc (Just sid)
]

(OTel trace/span IDs are hex strings in practice, so they're safe today, but encoding them too makes the invariant unconditional.)

Simplification: avoid intermediate ByteString in generateNamed call

Telemetry.hs line 856

-- current
BS.unpack $ BS.intercalate "\US" keyParts

-- avoids the intermediate ByteString allocation
L.intercalate [0x1f] (map BS.unpack keyParts)

uuid's `generateNamed` requires `[Word8]`, so the list is unavoidable, but there's no need to first materialise a joined `ByteString` only to immediately unpack it again. The Issues.hs precedent (`BS.unpack . encodeUtf8`) is fine for a single field but a concat-then-unpack on several parts pays twice.

Readability: the log-path arm is a 120-char single line

Telemetry.hs line 862

-- current — one dense line
_ -> [encodeUtf8 r.project_id, enc (unAesonTextMaybe r.body), encodeUtf8 $ fromMaybe "" r.name, enc r.severity, enc (unAesonTextMaybe r.attributes), enc (unAesonTextMaybe r.resource)]

-- easier to read and diff
_ ->
  [ enc (Just r.project_id)
  , enc (unAesonTextMaybe r.body)
  , enc r.name
  , enc r.severity
  , enc (unAesonTextMaybe r.attributes)
  , enc (unAesonTextMaybe r.resource)
  ]

Test gap: empty span_id guard is untested

DeterministicIdSpec.hs

The guard `not (T.null sid)` on line 860 makes a span with `span_id = ""` fall back to content-based keying, but there's no test for it. A one-liner would cover it:

it "span with empty span_id falls back to content keying (treated as log)" do
  idOf base{context = ctx "tr" ""}  `shouldNotBe`  idOf base{context = ctx "tr" "sp"}

The determinism logic and the effect-simplification are solid; the separator-injection issue is the only correctness concern.

Found 3 test failures on Blacksmith runners:

Failures

Code Review

This PR fixes the 06-19 duplicate pile-up by switching from random UUID v4 to content-derived UUID v5 for OTel row IDs, making the DLQ replay path idempotent against TF's (id, timestamp) dedup. The design (spans key on OTel-unique (project_id, trace_id, span_id), logs on stable content fields, timestamp excluded) is correct and the test suite covers the important invariants well.

1. L.intercalate [0x1f] (map BS.unpack keyParts) → BS.unpack $ BS.intercalate "\x1f" keyParts

src/Models/Telemetry/Telemetry.hs:856

The current code unpacks every ByteString to [Word8] first, then intercalates at the list level. The existing pattern in the codebase (Issues.hs:814: UUID5.generateNamed UUID5.namespaceOID . BS.unpack . encodeUtf8) does the delimiter work in ByteString space and unpacks once at the end — same idiom should apply here:

-- before
deterministicOtelId r = UUID.toText $ UUIDv5.generateNamed otelIdNamespace $
  L.intercalate [0x1f] (map BS.unpack keyParts)

-- after
deterministicOtelId r = UUID.toText $ UUIDv5.generateNamed otelIdNamespace $
  BS.unpack $ BS.intercalate "\x1f" keyParts

Side-effect: removes the intercalate addition from the Data.List import (line 96), leaving just groupBy.

2. enc Nothing → "" (empty bytes) is semantically load-bearing but undocumented

src/Models/Telemetry/Telemetry.hs:862

enc = maybe "" (BSL.toStrict . AE.encode)

Nothing maps to empty bytes, not JSON "null". This is correct and non-colliding (JSON-encoded Just x always starts with a " byte, so no empty-vs-null ambiguity). But the existing comment only explains separator injection; it doesn't say that Nothing → "" is intentional and stable — a future refactor to BSL.toStrict . AE.encode (dropping the maybe "") would silently re-derive every existing ID. Worth one sentence:

-- Nothing → "" (not "null") so the delimiter structure is unchanged if a
-- field goes from absent to present; do not change without re-id-ing stored rows.
enc :: AE.ToJSON a => Maybe a -> BS.ByteString
enc = maybe "" (BSL.toStrict . AE.encode)

3. Test gap: empty span_id routing

test/unit/Models/Telemetry/DeterministicIdSpec.hs

The not (T.null sid) guard at line 864 routes span_id = Just "" to the log path. No test covers this. OTel emitters sometimes send span_id: "" for orphaned logs; if that happens, those records fall into content-based dedup (good!) but this is currently invisible in the spec. Suggest:

it "span with empty span_id falls back to log (content) path" do
  let spanRecord = base{context = Just (Context (Just "tr") (Just "") Nothing Nothing Nothing), body = jsonBody "hello"}
      logRecord  = base{body = jsonBody "hello"}
  idOf spanRecord `shouldBe` idOf logRecord

Minor

• UUIDv5 vs UUID5: Issues.hs imports Data.UUID.V5 qualified as UUID5; this file uses UUIDv5. Pick one across the codebase.
• The idOf helper in the spec (V.head (mintOtelLogIds (V.singleton r))).id) could be (.id) . V.head . mintOtelLogIds . V.singleton — no functional change, just point-free for consistency.

Overall this is clean work. The separator-injection rationale is well-commented, the test suite is thorough, and dropping UUIDEff from the effect row is a nice cleanup. The main actionable item is the intercalate simplification (#1).