Skip to content

Add ETS table memory observability metrics - #3632

Merged
alco merged 21 commits into
mainfrom
alco/ets-observability
Apr 9, 2026
Merged

Add ETS table memory observability metrics#3632
alco merged 21 commits into
mainfrom
alco/ets-observability

Conversation

@alco

@alco alco commented Dec 21, 2025

Copy link
Copy Markdown
Member

Summary

  • Adds ElectricTelemetry.EtsTables module that collects memory usage statistics from ETS tables, grouping by "type" (extracted from table names using pattern matching on UUID suffixes)
  • Integrates ETS table metrics into the telemetry pipeline via a new ets_table_memory periodic measurement in ApplicationTelemetry
  • Exports ets_table.memory.total metric with table_type tag, following the same pattern as process.memory.total
  • Adds configurable top_ets_table_count option (default: 10)

Changes

  • New: ElectricTelemetry.EtsTables - functions for collecting top N ETS tables by memory (individually and grouped by type)
  • Modified: ElectricTelemetry.ApplicationTelemetry - added ets_table_memory/1 periodic measurement and metric definition
  • Modified: ElectricTelemetry.Opts - added top_ets_table_count configuration option

Test plan

  • Unit tests for EtsTables module (13 tests passing)
  • Tests cover type extraction from colon-separated and underscore-separated names
  • Tests verify grouping, sorting, and statistics calculations
  • Compilation with --warnings-as-errors passes

🤖 Generated with Claude Code

@alco alco self-assigned this Dec 22, 2025
@codecov

codecov Bot commented Dec 22, 2025

Copy link
Copy Markdown

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
2742 1 2741 1
View the top 2 failed test(s) by shortest run time
Elixir.Electric.Replication.PublicationManagerTest::test component restarts handles relation tracker restart
Stack Traces | 0.238s run time
10) test component restarts handles relation tracker restart (Electric.Replication.PublicationManagerTest)
     .../electric/replication/publication_manager_test.exs:503
     ** (exit) exited in: GenServer.call({:via, Registry, {:"Electric.ProcessRegistry:Electric.Replication.PublicationManagerTest test component restarts handles relation tracker restart", {Electric.Replication.PublicationManager.RelationTracker, nil}}}, {:remove_shape, "48312455-1775735905866142"}, 5000)
         ** (EXIT) no process: the process is not alive or there's no process currently associated with the given name, possibly because its application isn't started
     code: PublicationManager.remove_shape(ctx.stack_id, shape_handle)
     stacktrace:
       (elixir 1.19.5) lib/gen_server.ex:1135: GenServer.call/3
       (electric 1.5.0) .../replication/publication_manager/relation_tracker.ex:73: Electric.Replication.PublicationManager.RelationTracker.remove_shape/2
       .../electric/replication/publication_manager_test.exs:522: (test)
test/client.test.ts > should fall back to long polling after 3 consecutive short SSE connections
Stack Traces | 0.514s run time
AssertionError: expected 5 to be less than or equal to 4
 ❯ test/client.test.ts:2987:37

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@blacksmith-sh

blacksmith-sh Bot commented Dec 22, 2025

Copy link
Copy Markdown
Contributor

Found 2 test failures on Blacksmith runners:

Failures

Test View Logs
Elixir.ElectricTelemetry.CallHomeReporterTest/
test reports all expected info when started under ApplicationTelemetry
View Logs
Elixir.ElectricTelemetry.CallHomeReporterTest/
test reports all expected info when started under StackTelemetry
View Logs

Fix in Cursor

@balegas

balegas commented Jan 19, 2026

Copy link
Copy Markdown
Contributor

you can close this if you don't want to work on it anymore

@alco alco removed their assignment Mar 2, 2026
@alco alco changed the title Alco/ets observability Add ETS table memory observability metrics Mar 23, 2026
@alco alco added the claude label Mar 23, 2026
@claude

claude Bot commented Mar 23, 2026

Copy link
Copy Markdown

Claude Code Guidelines

See AGENTS.md for project conventions, architecture, and coding guidelines.

@alco
alco force-pushed the alco/ets-observability branch from 31e33f6 to c5e1099 Compare March 26, 2026 09:36
@claude

claude Bot commented Mar 26, 2026

Copy link
Copy Markdown

Claude Code Review

Summary

Iteration 3. Good incremental progress: the metric name was revised, test flakiness was fixed, and the utility router now emits the correct Content-Type. The one remaining Important item — @spec annotations on public functions — is still outstanding.

What's Working Well

  • Metric name revised: ets.memory.total is now fully dot-separated and consistent with process.memory.total. This resolves the naming inconsistency flagged in iterations 1 and 2.
  • Reduced list traversals (e2cc59f4): calculate_type_stats/1 is now shared between top_tables and top_by_type instead of being recomputed twice. Clean refactor.
  • Test flakiness fixed (50619b5d): The proc_type/1 dead-process test no longer races with the monitor, and assert_receive :labelled now has an explicit 150 ms timeout.
  • text_resp/3 helper (44f796aa): Proper text/plain Content-Type on the /metrics endpoint and 404 response. The empty-string fallback instead of "[]" is also correct.
  • Intentional word_size decision (7bcbc66b): The commit message documents that querying at runtime is deliberate — no concern here.

Issues Found

Critical (Must Fix)

None.

Important (Should Fix)

Missing @spec annotations on all three public functions (carried from iterations 1 and 2 — still not addressed)

File: packages/electric-telemetry/lib/electric/telemetry/ets_tables.ex

Project conventions require @spec on public functions. All three are still missing:

@spec top_tables(pos_integer()) :: [
  %{
    name: atom() | binary(),
    type: binary(),
    memory: non_neg_integer(),
    size: non_neg_integer(),
    type_table_count: pos_integer(),
    avg_size_per_type: float()
  }
]

@spec top_by_type(pos_integer()) :: [
  %{type: binary(), memory: non_neg_integer(), table_count: pos_integer(), avg_size: float()}
]

@spec top_memory_stats(pos_integer(), pos_integer()) :: %{
  top_tables: list(),
  top_by_type: list()
}

Suggestions (Nice to Have)

Redundant to_string/1 in ets_memory/1

packages/electric-telemetry/lib/electric/telemetry/application_telemetry.ex (new ets_memory/1 function)

table_type/1 always returns a binary (Atom.to_string/1, extract_type_from_name/1, or inspect/1), so to_string(type) in %{table_type: to_string(type)} is a no-op. Removing it makes the intent clearer:

:telemetry.execute([:ets, :memory], %{total: memory}, %{table_type: type})

top_memory_stats/2 — IEx-only intent not documented (from iteration 1 — still unaddressed)

packages/electric-telemetry/lib/electric/telemetry/ets_tables.ex (top_memory_stats/2)

The production integration only calls top_by_type/1. A single sentence in the @doc noting this is a debugging/IEx helper would help future readers.

Issue Conformance

No linked GitHub issue. PR description clearly explains the scope and a changeset is present; a linked issue would be nice for traceability but is not blocking.

Previous Review Status

Item Status
Docstring examples show string types Fixed (iter 2)
@word_size hoisted to compile-time Intentionally reverted to runtime call (iter 3)
Missing @spec on public functions Not addressed
Metric name inconsistency Fixed in iter 3 (ets.memory.total)
Redundant count > 0 guard Author dismissed
top_memory_stats/2 doc note Not addressed
Redundant to_string(type) New issue (iter 3)

Review iteration: 3 | 2026-04-09

alco and others added 18 commits April 9, 2026 12:37
… mem usage

Similar to how ElectricTelemetry.Processes shows top N types of
processes by memory use.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add ets_table_memory periodic measurement that reports top N ETS table
types by memory usage, similar to how process_memory reports top process
types. Adds the ets_table.memory.total metric with table_type tag and
a configurable top_ets_table_count option (default: 10).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Named ETS tables are owned by the test process and auto-deleted when it
exits. The on_exit callback runs in a separate process, so tables may
already be gone. Use try/rescue instead of :ets.info check.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Update docstring examples to show string types instead of atoms,
  matching the actual return values after the String.to_atom removal
- Hoist :erlang.system_info(:wordsize) to a module attribute since
  it's a VM constant

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ETS tables are owned by the creating process, so they are automatically
cleaned up when the test process exits.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@alco
alco force-pushed the alco/ets-observability branch from 405cdc7 to 44f796a Compare April 9, 2026 11:57
@alco
alco marked this pull request as ready for review April 9, 2026 11:57

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 44f796aa8f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

]},
default: {:count, 5}
],
top_ets_table_count: [type: :integer, default: 10],

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Validate top ETS table count as positive

top_ets_table_count is validated as any integer, but ApplicationTelemetry.ets_memory/1 passes it to EtsTables.top_by_type/1, which only has a clause for integers > 0. If a deployment sets this option to 0 or a negative number, periodic telemetry polling will raise a FunctionClauseError on every run, dropping ETS metrics and potentially causing repeated poller restarts/log noise. Constrain this option to a positive integer (or add a non-crashing fallback path).

Useful? React with 👍 / 👎.

@alco
alco merged commit cc623d7 into main Apr 9, 2026
31 of 33 checks passed
@alco
alco deleted the alco/ets-observability branch April 9, 2026 13:06
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