Skip to content

refactor(sync-service): Convert Logger calls to use static strings with metadata#3969

Draft
alco wants to merge 18 commits intomainfrom
refactor/static-logger-strings
Draft

refactor(sync-service): Convert Logger calls to use static strings with metadata#3969
alco wants to merge 18 commits intomainfrom
refactor/static-logger-strings

Conversation

@alco
Copy link
Copy Markdown
Member

@alco alco commented Mar 6, 2026

Summary

  • Refactor all Logger calls in electric-telemetry and sync-service packages to use static strings with metadata attributes instead of string interpolation
  • This improves log structuring for better observability and integrates with OTEL log handlers that can extract metadata fields
  • Update config/runtime.exs with extended metadata list for default formatter and OTEL metadata_map entries
  • Update Lux tests and unit tests to match the new log format

Test plan

  • Run mix compile --warnings-as-errors - passes
  • Run Lux integration tests to verify log patterns are correctly matched
  • Verify dynamic parameters appear in logs with metadata format

🤖 Generated with Claude Code

@alco alco added the claude label Mar 6, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 6, 2026

Codecov Report

❌ Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.47%. Comparing base (0af96e9) to head (3cd1f43).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ry/lib/electric/telemetry/application_telemetry.ex 0.00% 5 Missing ⚠️
...telemetry/lib/electric/telemetry/system_monitor.ex 0.00% 5 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (0af96e9) and HEAD (3cd1f43). Click for more details.

HEAD has 14 uploads less than BASE
Flag BASE (0af96e9) HEAD (3cd1f43)
unit-tests 5 1
packages/react-hooks 1 0
typescript 5 0
packages/start 1 0
packages/y-electric 1 0
packages/experimental 1 0
packages/typescript-client 1 0
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #3969       +/-   ##
===========================================
- Coverage   88.67%   64.47%   -24.21%     
===========================================
  Files          25       14       -11     
  Lines        2438      425     -2013     
  Branches      615        0      -615     
===========================================
- Hits         2162      274     -1888     
+ Misses        274      151      -123     
+ Partials        2        0        -2     
Flag Coverage Δ
electric-telemetry 64.47% <0.00%> (?)
elixir 64.47% <0.00%> (?)
packages/experimental ?
packages/react-hooks ?
packages/start ?
packages/typescript-client ?
packages/y-electric ?
typescript ?
unit-tests 64.47% <0.00%> (-24.21%) ⬇️

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.

@blacksmith-sh
Copy link
Copy Markdown
Contributor

blacksmith-sh bot commented Mar 6, 2026

Found 1 test failure on Blacksmith runners:

Failure

Test View Logs
Elixir.Electric.Connection.ConnectionManagerTest/
test status monitor backtracks the status when the canary goes down
View Logs

Fix in Cursor

@claude
Copy link
Copy Markdown

claude bot commented Mar 6, 2026

LINE1
LINE2
LINE3

@netlify
Copy link
Copy Markdown

netlify bot commented Mar 9, 2026

Deploy Preview for electric-next ready!

Name Link
🔨 Latest commit f86404d
🔍 Latest deploy log https://app.netlify.com/projects/electric-next/deploys/69aea9610e127000080c7b2b
😎 Deploy Preview https://deploy-preview-3969--electric-next.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@netlify
Copy link
Copy Markdown

netlify bot commented Mar 9, 2026

Deploy Preview for electric-next ready!

Name Link
🔨 Latest commit e200733
🔍 Latest deploy log https://app.netlify.com/projects/electric-next/deploys/69c41106f60f4700080eee8b
😎 Deploy Preview https://deploy-preview-3969--electric-next.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@claude
Copy link
Copy Markdown

claude bot commented Mar 9, 2026

Claude Code Review

Summary

The latest commit (3cd1f43) adds a Lux fail-pattern override to exclude benign OTEL log handler removal messages from causing false test failures in stack-telemetry.lux. The fix is correct and targeted. One previously-flagged issue remains unaddressed, and the system_monitor.ex refactor introduces the same silent-drop pattern.

Issues Found

Important (Should Fix)

system_monitor.ex: gc_info and locations metadata values silently dropped by Logger.Formatter

Files: packages/electric-telemetry/lib/electric/telemetry/system_monitor.ex (long GC handler ~line 41, long schedule handler ~line 74)

Issue: Two new metadata values are complex types that Logger.Formatter will silently skip. Logger.Formatter.format_metadata_value/1 returns :skip for any value that is not a binary, atom, number, PID, or port. Keyword lists (gc_info: info) and maps (locations:) hit the catch-all clause and are silently omitted from console output. The OTEL handler may serialize them differently, but for local log output these fields will be invisible. Apply the same inspect/1 fix used in consumer_registry.ex (commit f7abd5b).

application_telemetry.ex: data_points list values still not wrapped with inspect/1

File: packages/electric-telemetry/lib/electric/telemetry/application_telemetry.ex (~lines 337, 390, 424)

Issue: As noted in the previous review, three Logger.warning/2 calls still pass list values directly as data_points: metadata. missing_system_memory_keys, missing_keys, and missing_swap_keys are all Enum.reject/2 results (lists). Logger.Formatter will silently drop them. Flagged in iteration 10 but not addressed in any subsequent commit. Wrap each with inspect/1.

Previous Review Status

  • Flaky CI test assertions addressed in cb8f804
  • consumer_registry.ex silent-drop for crashed/shape_handles addressed in f7abd5b
  • OTEL log handler removal false positive in stack-telemetry.lux addressed in 3cd1f43 (latest commit)
  • application_telemetry.ex data_points list values without inspect/1still unresolved
  • system_monitor.ex gc_info (keyword list) and locations (map) without inspect/1new issue in this diff

Review iteration: 11 | 2026-03-25

alco and others added 15 commits March 25, 2026 17:39
…th metadata

Refactor all Logger calls in electric-telemetry and sync-service packages to
use static strings with metadata attributes instead of string interpolation.

This improves log structuring for better observability and integrates with
OTEL log handlers that can extract metadata fields.

Changes:
- Extract dynamic values from log messages into metadata attributes
- Update config/runtime.exs with extended metadata list for default formatter
- Add OTEL metadata_map entries for new attributes
- Update Lux tests to match new log format with metadata
- Update unit tests to check for static messages with metadata

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Fix test assertions that were checking for interpolated log messages.
Now check for static messages with metadata attributes.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Update Lux test patterns to match Logger calls that now use metadata
attributes instead of string interpolation.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Update manual-table-publishing.lux for Failed to create snapshot pattern
- Update postgres-disconnection.lux for Database connection failed pattern
- Update tests to use simpler "Starting replication client" pattern

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Update Snapshot started pattern to match new metadata-based format.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Rename `msg:` to `unexpected_msg:` to avoid reserved key conflict
- Remove `state:` from logger metadata to prevent credential leakage

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The Lux test fail pattern matches "error" case-insensitively, which
causes false failures when error_message= appears in log metadata output.
Renaming to err_msg avoids this issue.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove `opts: opts` metadata from SchemaReconciler (exposes internal config)
- Guard eager `length/1` calls in consumer.ex hot paths with `Logger.enabled?(:debug)`
- Rename `lock_name:` to `slot_name:` for consistency in acquire_lock log
- Remove redundant `client_pid: self()` from api.ex (`:pid` already in global metadata)
- Add missing metadata keys to formatter and OTEL metadata_map in runtime.exs
- Remove unused `lock_name` from formatter metadata list and OTEL map
- Update Lux tests for lock_name -> slot_name rename

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix test assertion: lock_name= -> slot_name= in replication_client_test
- Rename message: to pg_message: metadata key to avoid OTEL reserved field
- Add missing metadata keys to formatter list (version, instance_id, etc.)
- Add missing OTEL metadata_map entries (query_string, idle_time_s, etc.)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Logger.enabled?/1 takes a PID, not a log level. Use
Logger.compare_levels(Logger.level(), :debug) != :gt to correctly
check if debug logging is enabled before computing expensive metadata.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add ~26 missing metadata keys to formatter list and OTEL metadata_map
- Rename pid: to connection_pid:/materializer_pid:/monitored_pid: to
  avoid shadowing built-in :pid metadata
- Standardize dependency_handle -> dep_handle for consistency
- Add 9 missing OTEL metadata_map entries (interval_ms, trash_dir, etc.)
- Add patch changeset for sync-service and electric-telemetry

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…gment comment

- Convert 3 Logger calls introduced on main to use static strings with metadata
- Add comment explaining intentional omission of :txn and :txn_fragment from OTEL metadata_map

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@erik-the-implementer erik-the-implementer force-pushed the refactor/static-logger-strings branch from c613cc5 to e200733 Compare March 25, 2026 16:44
erik-the-implementer and others added 3 commits March 25, 2026 17:57
- Fix 3 test assertions that expected crash details inline in log message
  (now in metadata)
- Add :crashed to formatter metadata list for log output
- Restore "..." ellipsis in replication connection warning (changed during rebase)
- Convert 3 new Logger interpolation calls from main to static strings

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Logger.Formatter silently drops metadata values that don't implement
String.Chars. Use inspect() to convert map and list values to strings
so they render in log output.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The otel_log_handler may be removed during startup in test environments,
producing a "Logger - error: {removed_failing_handler,otel_log_handler}"
message that triggers the Lux fail pattern. Override the fail pattern in
the stack-telemetry test to exclude this specific benign message.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.

2 participants