Skip to content

Add GVL/allocation instrumentation and optional log fields#58

Merged
tycooon merged 8 commits into
umbrellio:mainfrom
Peralphix:gvl-allocations
Jul 21, 2026
Merged

Add GVL/allocation instrumentation and optional log fields#58
tycooon merged 8 commits into
umbrellio:mainfrom
Peralphix:gvl-allocations

Conversation

@Peralphix

Copy link
Copy Markdown
Contributor

Extracted GVL/memory instrumentation from our applications into the shared gem.

Added new files (loaded only via explicit require):

  • patches/active_support_event — adds #gvl_time and #malloc_increase_bytes to ActiveSupport::Notifications::Event;
  • patches/rails_semantic_logger — enriches the "Completed" action log entry with GC/GVL/allocation stats;
  • semantic_logger/sidekiq_job_metrics — same stats for Sidekiq jobs.

TinyJsonFormatter now supports optional duration and payload keys in custom_names_mapping. Previously such keys were silently ignored. Output is unchanged unless the keys are explicitly requested.

@tycooon

tycooon commented Jul 13, 2026

Copy link
Copy Markdown
Member

🤖 AI-assisted review (Claude Code), non-blocking.

Nice that the patches are opt-in (require-your-own) and covered by specs. Two small notes:

  1. GC.stat[:malloc_increase_bytes] allocates the whole stat hash. In lib/umbrellio_utils/patches/active_support_event.rb, start!/finish! call GC.stat[:malloc_increase_bytes], which builds the entire GC.stat hash (~30 entries) each time — twice per instrumented event. GC.stat(:malloc_increase_bytes) reads just that one key without allocating the hash. A little ironic for allocation instrumentation, and cheap to change.

  2. malloc_increase_bytes can be negative / cross-thread. It's a delta of a process-global GC counter, so it (a) can drop below zero if a GC runs inside the window, and (b) under threaded servers (Puma threads, Sidekiq concurrency) includes allocations from sibling threads. The consuming yabeda PRs guard with .positive?, but Event#malloc_increase_bytes surfacing negatives might deserve a doc caveat. gvl_time (GVLTools LocalTimer) is per-thread, so that one is fine.

@tycooon-review-bot tycooon-review-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.

Claude Code (AI review) — reviewed against origin/main.

Clean, well-scoped extraction. Verified the parts that are easy to get wrong: the three instrumentation files are genuinely opt-in (the gem's entrypoint uses explicit require_relative and doesn't load them, so require "gvltools" only fires for consumers who opt in), GVLTools::LocalTimer.monotonic_time is nanoseconds so / 1_000_000.0 → ms is correct, GC.stat(:malloc_increase_bytes) reads the single key without allocating the stat hash, and the base gem really does define extract_path (the spec stub mirrors it). The prior review's two notes (GC.stat allocation, negative/cross-thread malloc) are already addressed in the current code.

Two non-blocking notes:

  • P1add_stats (rails_semantic_logger) and metrics_payload (sidekiq_job_metrics) duplicate the same 6-stat extraction block plus a second PRECISION = 6; worth extracting a shared helper.
  • P2 — the controller patch leaves the raw :exception_object in the logged payload on failed actions, inconsistent with the Sidekiq path (which keeps only [class, message]).

No P0s. The TinyJsonFormatter optional-fields change is backward-compatible. CI is green across Ruby 3.3 / 3.4 / 4.0.

Comment thread lib/umbrellio_utils/patches/rails_semantic_logger.rb Outdated
Comment thread lib/umbrellio_utils/patches/rails_semantic_logger.rb

@tycooon-review-bot tycooon-review-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.

APPROVED — Claude Code (AI review). Both findings addressed in 25fcc21 with tests (P1: shared ActiveSupportEvent#stats; P2: :exception_object dropped + failed-action spec). CI green on Ruby 3.3/3.4/4.0.

@tycooon
tycooon merged commit cab07b9 into umbrellio:main Jul 21, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants