Skip to content

Modernize instrumenter#1

Merged
tagliala merged 8 commits into
masterfrom
chore/modernize-instrumenter
May 19, 2026
Merged

Modernize instrumenter#1
tagliala merged 8 commits into
masterfrom
chore/modernize-instrumenter

Conversation

@tagliala

Copy link
Copy Markdown
Member

No description provided.

@tagliala tagliala force-pushed the chore/modernize-instrumenter branch from 93b7902 to d0021e7 Compare May 19, 2026 10:13
@tagliala tagliala force-pushed the chore/modernize-instrumenter branch from d0021e7 to 25f91e4 Compare May 19, 2026 10:15
@tagliala tagliala requested a review from Copilot May 19, 2026 10:17

Copilot AI 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.

Pull request overview

This PR modernizes the instrumenter gem by adding CI/linting, introducing an Appraisal-based Rails version test matrix, and refactoring the core instrumentation implementation with a new RSpec suite.

Changes:

  • Refactors Instrumenter internals (new SubscriberBase, updated Maker, updated logging/runtime behavior).
  • Adds RSpec test suite and test helpers, plus a default rake test task.
  • Adds RuboCop configuration and GitHub Actions workflows, plus Appraisal gemfiles/matrix documentation.

Reviewed changes

Copilot reviewed 24 out of 25 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
lib/instrumenter.rb Major refactor of instrumentation/log subscriber + Railtie/controller runtime generation.
lib/version.rb Removes old version constant location.
lib/instrumenter/version.rb Adds new version file and bumps version to 0.0.2.
instrumenter.gemspec Modernizes gemspec, adds activesupport dependency and Ruby requirement, changes packaged file list.
spec/spec_helper.rb Adds RSpec + optional SimpleCov setup and loads Rails/instrumenter for specs.
spec/helpers.rb Adds shared spec helpers and thread-local runtime cleanup.
spec/instrumenter_spec.rb Adds specs for Instrumenter::VERSION and .instrument behavior.
spec/instrumenter/maker_spec.rb Adds specs covering generated subscriber/controller runtime/railtie behavior.
Rakefile Adds RSpec rake task and sets default task to run tests.
README.md Adds CI badges and documents Appraisal/testing matrix usage.
Gemfile Adds dev dependencies (rspec, rubocop, appraisal2, rails, etc.).
Appraisals Declares Rails version appraisal matrix (7.0–8.1 + edge).
gemfiles/rails_7.0.gemfile Appraisal-generated Rails 7.0 gemfile.
gemfiles/rails_7.1.gemfile Appraisal-generated Rails 7.1 gemfile.
gemfiles/rails_7.2.gemfile Appraisal-generated Rails 7.2 gemfile.
gemfiles/rails_8.0.gemfile Appraisal-generated Rails 8.0 gemfile.
gemfiles/rails_8.1.gemfile Appraisal-generated Rails 8.1 gemfile.
gemfiles/rails_edge.gemfile Appraisal-generated Rails edge gemfile.
.rubocop.yml Enables RuboCop plugins and sets project linting rules.
.rubocop_todo.yml Adds auto-generated RuboCop todo.
.rspec Configures RSpec to always require spec_helper.
.gitignore Updates ignored files (coverage/tmp/pkg, appraisal locks, etc.).
.gitattributes Marks .rubocop_todo.yml as linguist-generated.
.github/workflows/ruby.yml Adds GitHub Actions job to run specs across Ruby/Rails matrix.
.github/workflows/rubocop.yml Adds GitHub Actions job to run RuboCop.
Comments suppressed due to low confidence (2)

lib/instrumenter.rb:60

  • Default argument prefix.to_s.camelize.constantize relies on ActiveSupport inflections (String#camelize/#constantize), but this file only requires active_support/notifications/log_subscriber, which may not load those core extensions in non-Rails usage. Consider requiring the needed inflector/core_ext explicitly (or using ActiveSupport::Inflector.camelize/constantize) to avoid NoMethodError when Rails isn't loaded.
  def self.instrument(target, prefix, klass = prefix.to_s.camelize.constantize)
    target.instance_eval do
      define_method :instrument do |action, payload, &block|
        ActiveSupport::Notifications.instrument("#{action}.#{prefix}", payload, &block)
      end

lib/instrumenter.rb:80

  • define! can call define_railtie! whenever Rails is defined, even if ActionController::Base isn't (so @runtime stays nil). In that scenario (e.g., only railties loaded), the initializer will later execute include runtime with runtime == nil, raising a TypeError. Consider gating railtie generation on defined?(::Rails::Railtie) and on @runtime being present (or make the initializer conditional) so partial-Rails environments don't break.
    def define!
      define_log_subscriber!
      define_controller_runtime! if defined?(ActionController::Base)
      define_railtie! if defined?(Rails)
    end

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/instrumenter.rb
Comment thread lib/instrumenter.rb Outdated
Comment thread instrumenter.gemspec Outdated
@tagliala tagliala marked this pull request as draft May 19, 2026 10:23
@tagliala tagliala force-pushed the chore/modernize-instrumenter branch from f659d41 to b23e0e9 Compare May 19, 2026 12:25
@tagliala tagliala marked this pull request as ready for review May 19, 2026 12:25
@tagliala tagliala requested a review from Copilot May 19, 2026 12:26

Copilot AI 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.

Pull request overview

Copilot reviewed 24 out of 25 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

lib/instrumenter.rb:16

  • instrument defaults klass via prefix.to_s.camelize.constantize, but this file only requires active_support. In a non-Rails environment, String#camelize/#constantize are not guaranteed to be loaded, which can cause NoMethodError at call time. Consider requiring active_support/core_ext/string/inflections (or switching to ActiveSupport::Inflector.camelize/constantize to avoid relying on core extensions).
require 'active_support'
require 'active_support/notifications'
require 'active_support/log_subscriber'

require_relative 'instrumenter/version'

# Provides ActiveSupport-based instrumentation helpers for target classes.
module Instrumenter
  LOG_MESSAGE_FORMAT = '  %<name>s: %<method>s %<url>s (%<duration>.1fms) - cache %<cache>s'

  def self.instrument(target, prefix, klass = prefix.to_s.camelize.constantize)
    target.instance_eval do
      define_method :instrument do |action, payload, &block|
        ActiveSupport::Notifications.instrument("#{action}.#{prefix}", payload, &block)

Comment thread lib/instrumenter.rb
Comment thread README.md Outdated
Comment thread .github/workflows/ruby.yml
Comment thread .github/workflows/rubocop.yml
@tagliala tagliala force-pushed the chore/modernize-instrumenter branch 2 times, most recently from d35bcb4 to 616178e Compare May 19, 2026 12:41
@tagliala tagliala force-pushed the chore/modernize-instrumenter branch from 616178e to f8e82fb Compare May 19, 2026 12:43
@tagliala tagliala merged commit 3f8b70a into master May 19, 2026
30 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.

2 participants