code review and fixes

Author: ndbroadbent

.cspell/project-words.txt

@@ -53,3 +53,4 @@ TESTOPTS
 docspring
 shiki
 lightningcss
+klass

DEVELOPMENT.md

@@ -97,10 +97,10 @@ And you always need to check for any third-party gems that are not part of Rails
 
 - Use Sorbet type annotations for all methods
 - Ensure all files have the appropriate `# typed:` annotation
-- **NEVER use `T.unsafe` calls**. Instead, properly type your code or use appropriate type assertions with `T.let` or `T.cast` when absolutely necessary.
+- Prefer precise types and `T.let`/`T.cast` over `T.unsafe`. If a Sorbet limitation requires it (e.g., splat with callbacks, serializing unknown external objects), a minimal, localized `T.unsafe` may be used with a brief comment explaining why.
 - `T.untyped` is generally ok for Hash values when they come from unknown sources.
-- When dealing with external libraries, create proper type signatures or use extension methods rather than resorting to `T.unsafe`.
-- **NEVER use `class.name`** anywhere - this is a Sorbet quirk that hides the `name` method from all base classes. Prefer just using Classes themselves as the type. `"#{class}"` will automatically call `.to_s`. Similarly, `to_json` will automatically call `.to_s` - but you can call `.to_s` manually if you really need it.
+- When dealing with external libraries, create proper type signatures or use extension methods; keep `T.unsafe` usage small and justified.
+- Avoid `class.name` where possible due to Sorbet quirks; prefer string interpolation (`"#{klass}"`) or use the class as a value.
 
 ### Testing
 

Gemfile.lock

@@ -213,7 +213,7 @@ GEM
     public_suffix (6.0.1)
     racc (1.8.1)
     rack (3.1.11)
-    rack-session (2.1.0)
+    rack-session (2.1.1)
       base64 (>= 0.1.0)
       rack (>= 3.0.0)
     rack-test (2.2.0)

README.md

@@ -1,6 +1,6 @@
 # LogStruct
 
-Adds JSON structured logging to any Rails app. Simply add the gem to your Gemfile and add an initializer to configure it. Now your Rails app prints beautiful JSON logs to STDOUT. They're easy to search and filter, you can turn them into metrics and alerts, and they're great for building dashboards in CloudWatch, Grafana, or Datadog.
+Adds JSON structured logging to any Rails app. Simply add the gem to your Gemfile and add an initializer to configure it. By default, your Rails app prints JSON logs to STDOUT (or to the configured destination when `RAILS_LOG_TO_STDOUT` is set). They're easy to search and filter, you can turn them into metrics and alerts, and they're great for building dashboards in CloudWatch, Grafana, or Datadog.
 
 We support all your other favorite gems too, like Sidekiq, Sentry, and Shrine. (And if not, please open a PR!)
 
@@ -14,7 +14,7 @@ We support all your other favorite gems too, like Sidekiq, Sentry, and Shrine. (
 - Error handling and reporting
 - Metadata collection for rich context
 - Lograge integration for structured request logging
-- Sensitive data scrubbing with Logstop (vendored fork)
+- Sensitive data scrubbing for strings (inspired by the Logstop gem)
 - Host authorization logging for security violations
 - Rack middleware for enhanced error logging
 - ActionMailer delivery callbacks for Rails 7.0.x (backported from Rails 7.1)
@@ -46,7 +46,7 @@ LogStruct is designed to be highly opinionated and work out of the box with mini
 
 Please see the [documentation](https://logstruct.com/docs/configuration/) for configuration options.
 
-### Important Note on Integration
+### Important Notes on Integration
 
 Once initialized, the gem automatically includes its modules into the appropriate base classes:
 
@@ -55,6 +55,8 @@ Once initialized, the gem automatically includes its modules into the appropriat
 - We configure `Lograge` for request logging
 - A Rack middleware is inserted to catch and log errors, including security violations (IP spoofing, CSRF, blocked hosts, etc.)
 - Structured logging is set up for ActiveJob, Sidekiq, Shrine, etc.
+- Rails `config.filter_parameters` are merged into LogStruct's filters and then cleared (to avoid double filtering). Configure sensitive keys via `LogStruct.config.filters`.
+- When `RAILS_LOG_TO_STDOUT` is set, we log to STDOUT only. Otherwise, we log to STDOUT by default without adding a file appender to avoid duplicate logs.
 
 ## Documentation
 

lib/log_struct/concerns/configuration.rb

@@ -42,12 +42,14 @@ def set_enabled_from_rails_env!
           # Set enabled based on current Rails environment and the LOGSTRUCT_ENABLED env var.
           # Precedence:
           # 1. Check if LOGSTRUCT_ENABLED env var is defined
-          # 2. Check if current Rails environment is in enabled_environments
-          # 3. Default to whatever is set in config.enabled (which defaults to true)
+          #    - Sets enabled=true only when value is "true"
+          #    - Sets enabled=false when value is "false" (or any non-"true")
+          # 2. Otherwise, check if current Rails environment is in enabled_environments
+          # 3. Otherwise, leave as config.enabled (defaults to true)
 
           # Then check if LOGSTRUCT_ENABLED env var is set
           config.enabled = if ENV["LOGSTRUCT_ENABLED"]
-            # Only override if env var is "true"
+            # Override to true only if env var is "true"
             ENV["LOGSTRUCT_ENABLED"] == "true"
           else
             config.enabled_environments.include?(::Rails.env.to_sym)

lib/log_struct/formatter.rb

@@ -61,9 +61,8 @@ def process_values(arg, recursion_depth: 0)
 
         # Process each key-value pair
         arg.each do |key, value|
-          # Only filter keys in nested structures (recursion_depth >= 1)
-          # Check if this key should be filtered
-          result[key] = if recursion_depth >= 1 && ParamFilters.should_filter_key?(key)
+          # Check if this key should be filtered at any depth
+          result[key] = if ParamFilters.should_filter_key?(key)
             # Filter the value
             {_filtered: ParamFilters.summarize_json_attribute(key, value)}
           else

lib/log_struct/log/sql.rb

@@ -105,19 +105,19 @@ def serialize(strict = true)
         hash = serialize_common(strict)
         merge_additional_data_fields(hash)
 
-        # Add SQL-specific fields
-        hash[:message] = message
-        hash[:sql] = sql
-        hash[:name] = name
-        hash[:duration] = duration
-        hash[:row_count] = row_count
-        hash[:connection_adapter] = connection_adapter
-        hash[:bind_params] = bind_params
-        hash[:database_name] = database_name
-        hash[:connection_pool_size] = connection_pool_size
-        hash[:active_connections] = active_connections
-        hash[:operation_type] = operation_type
-        hash[:table_names] = table_names
+        # Add SQL-specific fields using LOG_KEYS mapping for consistency
+        hash[LOG_KEYS.fetch(:message)] = message
+        hash[LOG_KEYS.fetch(:sql)] = sql
+        hash[LOG_KEYS.fetch(:name)] = name
+        hash[LOG_KEYS.fetch(:duration)] = duration
+        hash[LOG_KEYS.fetch(:row_count)] = row_count
+        hash[LOG_KEYS.fetch(:connection_adapter)] = connection_adapter
+        hash[LOG_KEYS.fetch(:bind_params)] = bind_params
+        hash[LOG_KEYS.fetch(:database_name)] = database_name
+        hash[LOG_KEYS.fetch(:connection_pool_size)] = connection_pool_size
+        hash[LOG_KEYS.fetch(:active_connections)] = active_connections
+        hash[LOG_KEYS.fetch(:operation_type)] = operation_type
+        hash[LOG_KEYS.fetch(:table_names)] = table_names
 
         hash
       end

lib/log_struct/log_keys.rb

@@ -84,7 +84,19 @@ module LogStruct
 
     # CarrierWave-specific fields
     model: :model,
-    mount_point: :mount_point
+    mount_point: :mount_point,
+
+    # SQL-specific fields
+    sql: :sql,
+    name: :name,
+    row_count: :row_count,
+    connection_adapter: :connection_adapter,
+    bind_params: :bind_params,
+    database_name: :database_name,
+    connection_pool_size: :connection_pool_size,
+    active_connections: :active_connections,
+    operation_type: :operation_type,
+    table_names: :table_names
   }.freeze,
     T::Hash[Symbol, Symbol])
 end

lib/log_struct/semantic_logger/logger.rb

@@ -96,7 +96,7 @@ def tagged(*tags, &block)
         if tag_array.empty?
           super(&block)
         else
-          super(T.unsafe(tag_array), &block)
+          super(*T.unsafe(tag_array), &block)
         end
       end
 
@@ -113,9 +113,11 @@ def clear_tags!
         ::SemanticLogger.pop_tags(count) if count > 0
       end
 
-      sig { params(tags: T.untyped).void }
+      sig { params(tags: T.untyped).returns(T::Array[T.untyped]) }
       def push_tags(*tags)
-        tags.flatten.each { |tag| ::SemanticLogger.push_tags(tag) }
+        flat = tags.flatten.compact
+        flat.each { |tag| ::SemanticLogger.push_tags(tag) }
+        flat
       end
 
       sig { params(count: Integer).void }

lib/log_struct/semantic_logger/setup.rb

@@ -137,8 +137,8 @@ def self.add_appenders(app)
           )
         end
 
-        # Add file appender if configured
-        if app.config.paths["log"].first && !ENV["RAILS_LOG_TO_STDOUT"]
+        # Add file appender if configured and not already logging to STDOUT/StringIO
+        if app.config.paths["log"].first && io != $stdout && !io.is_a?(StringIO)
           ::SemanticLogger.add_appender(
             file_name: app.config.paths["log"].first,
             formatter: LogStruct::SemanticLogger::Formatter.new,
@@ -155,6 +155,7 @@ def self.determine_output(app)
           # Use StringIO for tests to avoid cluttering test output
           StringIO.new
         else
+          # Prefer file logging when not explicitly configured for STDOUT
           $stdout
         end
       end