Code Review

Overall: Clean, focused deduplication — the extracted helper is well-named, correctly placed, and the two call sites collapse as intended. A few small notes below.

batchSpanAttrs implementation

batchSpanAttrs n attrs =
  ("messaging.batch.message_count", OA.toAttribute n)
    : toList (("ce.type",) . OA.toAttribute @Text <$> HM.lookup "ce-type" attrs)

The toList on Maybe is correct (Foldable instance), and the PR description notes this is an intentional improvement over the foldMap-with-singleton-list pattern. One alternative worth considering since Relude exports it:

: maybeToList (("ce.type",) . OA.toAttribute @Text <$> HM.lookup "ce-type" attrs)

maybeToList is arguably more intention-revealing (toList on Maybe is a Foldable trick some readers won't immediately parse), but this is a minor style nit — both are correct.

Pre-existing: unused binding in lambda

In processMessages (unchanged by this PR, but visible in the diff):

spans <- forM rMsgs \(rmAckId, rawSize, msg) -> runMaybeT do

rmAckId is never used in the body — would emit a -Wunused-matches warning. Easy fix: rename to _ or _ackId. Not introduced by this PR, but it's right there.

Test coverage

batchSpanAttrs is a pure function with trivial, verifiable behavior. A doctest would add coverage at near-zero cost:

-- >>> length (batchSpanAttrs 3 HM.empty)
-- 1
-- >>> length (batchSpanAttrs 3 (HM.singleton "ce-type" "com.example.event"))
-- 2

Summary

LGTM with the nits above being optional.

Code Review

Summary: Clean deduplication PR. The extraction is well-scoped and the maybeToList rewrite is strictly better than the foldMap-with-singleton-list pattern it replaces.

Nits / possible improvements

1. @Text annotation is redundant

In batchSpanAttrs, HM.lookup "ce-type" attrs already has type Maybe Text, so GHC infers the ToAttribute Text instance without the explicit @Text:

-- current
: maybeToList (("ce.type",) . OA.toAttribute @Text <$> HM.lookup "ce-type" attrs)

-- @Text is redundant; can drop it
: maybeToList (("ce.type",) . OA.toAttribute <$> HM.lookup "ce-type" attrs)

If you want to keep it for documentation/resilience, that's fine — but it's noise given the inferred type.

2. CloudEvents concern in the abstraction boundary

The Haddock says "queue consumers, gRPC handlers", but ce.type is a CloudEvents-specific header that only makes sense for the Pub/Sub path. If a future gRPC handler calls batchSpanAttrs with an attrs map that happens to contain a "ce-type" key (unlikely but possible), it will silently emit a ce.type span attribute in a confusing context.

Consider splitting the signature to make the optional attribute explicit at the call site:

-- option A: separate the optional CE attr
batchSpanAttrs :: Int -> [(Text, Attribute)]
batchSpanAttrs n = [("messaging.batch.message_count", OA.toAttribute n)]

-- call sites pass the optional attrs themselves:
withSpan_ "pubsub.process_messages"
  (batchSpanAttrs (length msgs) <> ceTypeAttr attrs) ...

ceTypeAttr :: HM.HashMap Text Text -> [(Text, Attribute)]
ceTypeAttr attrs = maybeToList (("ce.type",) . OA.toAttribute <$> HM.lookup "ce-type" attrs)

Or keep the current signature but rename/document it as pubsubSpanAttrs to signal it's not general-purpose. This is a design nit, not a blocker.

3. _ackId rename (bonus cleanup) — good

Renaming rmAckId → _ackId in the forM rMsgs lambda is correct; the binding was already unused and this silences the warning properly.

Overall

LGTM modulo the nits above. The @Text drop is a one-character win. The abstraction boundary question is worth a quick thought before merge but doesn't block CI.

Code Review

Clean, well-scoped refactor. The extraction is correct and the call sites are noticeably more readable. A few notes:

Positives

• maybeToList (f <$> mx) is a clear improvement over foldMap (\v -> [v]) -- exactly the right idiom.
• Dropping the now-unused OA import from ProcessMessage.hs is good hygiene.
• Renaming rmAckId to _ackId correctly signals the intentionally-unused binding.
• Module placement and export-section grouping (-- * Common attribute builders) are on point.

Suggestion: catMaybes scales better than (:) + maybeToList

The current impl works, but mixing : (for the mandatory attr) with maybeToList (for the optional one) is slightly asymmetric. If a second optional attribute ever gets added, it devolves into a : maybeToList b ++ maybeToList c. The catMaybes form is uniform and extends naturally:

batchSpanAttrs :: Int -> HM.HashMap Text Text -> [(Text, Attribute)]
batchSpanAttrs n attrs = catMaybes
  [ Just ("messaging.batch.message_count", OA.toAttribute n)
  , ("ce.type",) . OA.toAttribute <$> HM.lookup "ce-type" attrs
  ]

Not a blocker -- current code is correct -- but worth considering given this helper is likely to grow.

Minor: doctest for the pure function

batchSpanAttrs is a pure function and easy to doctest. A quick example covering the absent-header case would make the semantics self-documenting and act as a regression guard:

-- >>> import Data.HashMap.Strict qualified as HM
-- >>> length (batchSpanAttrs 3 HM.empty)
-- 1
-- >>> length (batchSpanAttrs 3 (HM.singleton "ce-type" "io.monoscope.log"))
-- 2

Not required to merge, but cheap to add given the project already runs doctests (make test-doctests).

No concerns

• Type inference is unambiguous without the dropped @Text annotation -- HM.lookup already fixes the value type to Text.
• No performance or security concerns.
• Test plan in the PR description (CI + post-deploy span attribute check) is appropriate for this change.

Code Review

Overview: Clean deduplication — extracts the repeated (message_count, optional ce.type) attribute-building logic into System.Tracing.batchSpanAttrs, collapses both call sites to one-liners, and drops the now-unused OpenTelemetry.Attributes import from ProcessMessage.hs. Net: –5 lines, zero behaviour change.

batchSpanAttrs implementation

The catMaybes [Just x, f <$> mx] pattern is valid, but the standard idiom for "one required element followed by one optional element" is (:) + maybeToList, which avoids the spurious Just wrapper and is marginally more direct:

batchSpanAttrs :: Int -> HM.HashMap Text Text -> [(Text, Attribute)]
batchSpanAttrs n attrs =
  ("messaging.batch.message_count", OA.toAttribute n)
    : maybeToList (("ce.type",) . OA.toAttribute <$> HM.lookup "ce-type" attrs)

Both catMaybes and maybeToList are re-exported by Relude, so no new import is needed. The maybeToList form reads as "the count attr, consed with the optionally-present ce.type attr", which matches the intent more literally and avoids allocating a two-element list that catMaybes then filters.

Minor nits

• rmAckId → _ackId in the forM rMsgs lambda: good catch, but _ alone would be even more concise here since the variable is never referenced.
• The haddock comment is clear. One small precision: the header map key is "ce-type" (hyphen) while the emitted OTel attribute is "ce.type" (dot) — worth mentioning explicitly in the comment to avoid future confusion, since they look similar.

No issues found

• Correctness: both call sites pass identical arguments to batchSpanAttrs as they did inline; semantics are preserved.
• No new imports beyond what's already used.
• Exports section in the module header is properly updated with a section comment.
• CI / test plan looks appropriate given this is a pure refactor with no logic change.

Overall this is a well-scoped, correct refactor. The maybeToList suggestion above is the only substantive improvement worth considering.

Code Review

Clean, well-scoped refactor. No correctness issues.

What's good

• batchSpanAttrs properly centralises the two identical attribute-list constructions; call sites are now a single line each.
• Dropping the unused OpenTelemetry.Attributes import from ProcessMessage.hs and rmAckId → _ are good hygiene.
• Docstring explains the OTel-dot / CloudEvents-hyphen naming distinction, which is genuinely non-obvious.
• (\"ce.type\",) . OA.toAttribute correctly leverages TupleSections.

One suggestion: prefer toList over catMaybes [Just …, …]

The current implementation:

batchSpanAttrs n attrs =
  catMaybes
    [ Just ("messaging.batch.message_count", OA.toAttribute n)
    , ("ce.type",) . OA.toAttribute <$> HM.lookup "ce-type" attrs
    ]

Wrapping the unconditional element in Just to then immediately catMaybes it away is a small conceptual overhead. The PR description itself suggests toList (f <$> mx) over Maybe; applying that here gives:

batchSpanAttrs n attrs =
  ("messaging.batch.message_count", OA.toAttribute n)
    : toList (("ce.type",) . OA.toAttribute <$> HM.lookup "ce-type" attrs)

This makes the asymmetry explicit in the structure: one element is always present (the :), one is conditionally appended (toList over Maybe). Both catMaybes and toList are in scope via Relude, so it's just a readability trade-off — but the toList form aligns with what the PR description already says and avoids the gratuitous Just.

No other concerns

• Type inference for OA.toAttribute works without the dropped @Text annotation since HM.lookup already fixes the value type to Text.
• No performance, security, or test-coverage issues.

Overall: LGTM pending the optional toList nit.