Skip to content

Move expiring-shapes log counts into Logger metadata#4161

Open
erik-the-implementer wants to merge 1 commit intomainfrom
fix-expiry-log-attrs
Open

Move expiring-shapes log counts into Logger metadata#4161
erik-the-implementer wants to merge 1 commit intomainfrom
fix-expiry-log-attrs

Conversation

@erik-the-implementer
Copy link
Copy Markdown
Contributor

@erik-the-implementer erik-the-implementer commented Apr 24, 2026

vetted by human ready for review

Summary

Replace the interpolated number_to_expire/max_shapes values in the Logger.notice(\"Expiring #{n} shapes as the number of shapes has exceeded the limit (#{max})\") call with a static message plus those values as structured Logger metadata. Extends the OTEL log handler's metadata_map so the counts surface as shape.expiry.number_to_expire and shape.expiry.max_shapes attributes.

Why

The expiring-shapes notice fires frequently and every entry currently contains different counts, which prevents log aggregators from grouping them together and makes filtering/aggregation in Honeycomb awkward. Following the pattern in #4145, keeping the message text static (with the dynamic values exported as attributes) makes these events easier to search, group, and alert on.

Refs electric-sql/alco-agent-tasks#33.

Test plan

  • mix compile --warnings-as-errors passes in packages/sync-service

Keep the "Expiring shapes as the number of shapes has exceeded the
limit" notice body static and move the dynamic counts into Logger
metadata. This allows log aggregators to group these events and lets
Honeycomb filter by the new shape.expiry.* OTEL attributes.

Refs electric-sql/alco-agent-tasks#33
@claude
Copy link
Copy Markdown

claude Bot commented Apr 24, 2026

Claude Code Review

Summary

This PR converts the expiring-shapes Logger.notice call from a string-interpolated message to a static message with number_to_expire and max_shapes as structured Logger metadata, and wires those keys into the OTEL log handler's metadata_map so they surface as shape.expiry.* attributes in Honeycomb. The change is minimal, correct, and follows the established pattern from #4145.

What's Working Well

  • Consistent pattern: Exactly mirrors the approach used in Move shape_handle into Logger metadata for No-consumer error #4145 for other log metadata — using Logger.notice/2 with a keyword-list second argument is idiomatic Elixir and correct.
  • Clean OTEL wiring: The two new entries in runtime.exs's metadata_map are correctly scoped (shape.expiry.number_to_expire, shape.expiry.max_shapes) and placed in the right block.
  • Changeset present: The .changeset/fix-expiry-manager-log-attributes.md is included, describes the change accurately, and is categorized correctly as a patch.
  • No behaviour change: The expiry logic itself is untouched; only the log formatting changes.

Issues Found

Critical (Must Fix)

None.

Important (Should Fix)

None.

Suggestions (Nice to Have)

  • Log message still readable without metadata: The static string "Expiring shapes as the number of shapes has exceeded the limit" is unambiguous even when viewed in a plain-text log stream, since the counts appear in the structured metadata. No action needed — just noting this tradeoff is already handled well.

Issue Conformance

The PR references electric-sql/alco-agent-tasks#33, which appears to be an internal/private task tracker rather than a public GitHub issue. The PR description compensates for this with a clear ## Why section explaining the aggregation/Honeycomb motivation, so reviewers have full context. No gaps in the implementation relative to what's described.


Review iteration: 1 | 2026-04-24

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.08%. Comparing base (3cef0ca) to head (a788273).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4161   +/-   ##
=======================================
  Coverage   68.08%   68.08%           
=======================================
  Files         130      130           
  Lines       16211    16211           
  Branches     3912     3911    -1     
=======================================
  Hits        11037    11037           
  Misses       5170     5170           
  Partials        4        4           
Flag Coverage Δ
packages/agents 38.39% <ø> (ø)
packages/agents-runtime 81.34% <ø> (ø)
packages/agents-server 65.55% <ø> (ø)
packages/agents-server-ui 0.00% <ø> (ø)
packages/electric-ax 30.46% <ø> (ø)
packages/experimental 87.73% <ø> (ø)
packages/react-hooks 86.48% <ø> (ø)
packages/start 82.83% <ø> (ø)
packages/typescript-client 94.30% <ø> (ø)
packages/y-electric 56.05% <ø> (ø)
typescript 68.08% <ø> (ø)
unit-tests 68.08% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@alco alco self-assigned this Apr 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants