Vets::SharedLogging example (#21130)

* Vets::SharedLogging example

* Add deprecation warning for SentryLogging

* Use SharedLogging in ExceptionHandling

Author: kayline

app/controllers/concerns/exception_handling.rb

@@ -4,9 +4,11 @@
 require 'common/client/errors'
 require 'json_schema/json_api_missing_attribute'
 require 'datadog'
+require 'vets/shared_logging'
 
 module ExceptionHandling
   extend ActiveSupport::Concern
+  include Vets::SharedLogging
 
   # In addition to Common::Exceptions::BackendServiceException that have sentry_type :none the following exceptions
   # will also be skipped.
@@ -76,6 +78,7 @@ def report_original_exception(exception)
     elsif exception.is_a?(Common::Exceptions::BackendServiceException) && exception.generic_error?
       # Warn about VA900 needing to be added to exception.en.yml
       log_message_to_sentry(exception.va900_warning, :warn, i18n_exception_hint: exception.va900_hint)
+      log_message_to_rails(exception.va900_warning, :warn, i18n_exception_hint: exception.va900_hint)
     end
   end
 
@@ -88,6 +91,7 @@ def report_mapped_exception(exception, va_exception)
     end
     va_exception_info = { va_exception_errors: va_exception.errors.map(&:to_hash) }
     log_exception_to_sentry(exception, extra.merge(va_exception_info))
+    log_exception_to_rails(exception)
 
     # Because we are handling exceptions here and not re-raising, we need to set the error on the
     # Datadog span for it to be reported correctly. We also need to set it on the top-level

app/models/form_attachment.rb

@@ -1,8 +1,10 @@
 # frozen_string_literal: true
 
+require 'vets/shared_logging'
+
 class FormAttachment < ApplicationRecord
   include SetGuid
-  include SentryLogging
+  include Vets::SharedLogging
 
   has_kms_key
   has_encrypted :file_data, key: :kms_key, **lockbox_options
@@ -18,6 +20,7 @@ def set_file_data!(file, file_password = nil)
     self.file_data = { filename: attachment_uploader.filename }.to_json
   rescue CarrierWave::IntegrityError => e
     log_exception_to_sentry(e, nil, nil, 'warn')
+    log_exception_to_rails(e, 'warn')
     raise Common::Exceptions::UnprocessableEntity.new(detail: e.message, source: 'FormAttachment.set_file_data')
   end
 
@@ -46,6 +49,7 @@ def unlock_pdf(file, file_password)
       password_regex = /(input_pw).*?(output)/
       sanitized_message = e.message.gsub(file_regex, '[FILTERED FILENAME]').gsub(password_regex, '\1 [FILTERED] \2')
       log_message_to_sentry(sanitized_message, 'warn')
+      log_message_to_rails(sanitized_message, 'warn')
       raise Common::Exceptions::UnprocessableEntity.new(
         detail: I18n.t('errors.messages.uploads.pdf.incorrect_password'),
         source: 'FormAttachment.unlock_pdf'

lib/sentry_logging.rb

@@ -3,6 +3,10 @@
 require 'sentry_logging'
 
 module SentryLogging
+  # WARNING: This module is deprecated, and will be removed in June 2025. Please use Vets::SharedLogging instead.
+  # If your team currently uses this module, please see documentation for migrating to Vets::SharedLogging: TODO
+  ActiveSupport::Deprecation
+    .new.warn('SentryLogging is deprecated and will be removed in June 2025. Use Vets::SharedLogging instead.')
   extend self
 
   def log_message_to_sentry(message, level, extra_context = {}, tags_context = {})

lib/vets/shared_logging.rb

@@ -0,0 +1,71 @@
+# frozen_string_literal: true
+
+module Vets
+  module SharedLogging
+    extend ActiveSupport::Concern
+
+    def log_message_to_sentry(message, level, extra_context = {}, tags_context = {})
+      level = normalize_shared_level(level, nil)
+      # https://docs.sentry.io/platforms/ruby/usage/set-level/
+      # valid sentry levels: log, debug, info, warning, error, fatal
+      level = 'warning' if level == 'warn'
+
+      if Settings.sentry.dsn.present?
+        set_sentry_metadata(extra_context, tags_context)
+        Sentry.capture_message(message, level:)
+      end
+    end
+
+    def log_exception_to_sentry(exception, extra_context = {}, tags_context = {}, level = 'error')
+      level = normalize_shared_level(level, exception)
+
+      if Settings.sentry.dsn.present?
+        set_sentry_metadata(extra_context, tags_context)
+        Sentry.capture_exception(exception.cause.presence || exception, level:)
+      end
+    end
+
+    def log_message_to_rails(message, level, extra_context = {})
+      # this can be a drop-in replacement for now, but maybe suggest teams
+      # handle extra context on their own and move to a direct Rails.logger call?
+      formatted_message = extra_context.empty? ? message : "#{message} : #{extra_context}"
+      Rails.logger.send(level, formatted_message)
+    end
+
+    def log_exception_to_rails(exception, level = 'error')
+      level = normalize_shared_level(level, exception)
+      if exception.is_a? Common::Exceptions::BackendServiceException
+        error_details = exception.errors.first.attributes.compact.reject { |_k, v| v.try(:empty?) }
+        log_message_to_rails(exception.message, level, error_details.merge(backtrace: exception.backtrace))
+      else
+        log_message_to_rails("#{exception.message}.", level)
+      end
+
+      log_message_to_rails(exception.backtrace.join("\n"), level) unless exception.backtrace.nil?
+    end
+
+    def normalize_shared_level(level, exception)
+      case exception
+      when Pundit::NotAuthorizedError
+        'info'
+      when Common::Exceptions::BaseError
+        # could change this attribute to log_level
+        # to make clear it is not just a Sentry concern
+        exception.sentry_type.to_s
+      else
+        level.to_s
+      end
+    end
+
+    def non_nil_hash?(h)
+      h.is_a?(Hash) && !h.empty?
+    end
+
+    private
+
+    def set_sentry_metadata(extra_context, tags_context)
+      Sentry.set_extras(extra_context) if non_nil_hash?(extra_context)
+      Sentry.set_tags(tags_context) if non_nil_hash?(tags_context)
+    end
+  end
+end

modules/decision_reviews/app/controllers/decision_reviews/application_controller.rb

@@ -24,46 +24,5 @@ def set_csrf_header
       token = form_authenticity_token
       response.set_header('X-CSRF-Token', token)
     end
-
-    def log_exception(exception, _extra_context = {}, _tags_context = {}, level = 'error')
-      level = normalize_level(level, exception)
-
-      if exception.is_a? Common::Exceptions::BackendServiceException
-        rails_logger(level, exception.message, exception.errors, exception.backtrace)
-      else
-        rails_logger(level, "#{exception.message}.")
-      end
-      rails_logger(level, exception.backtrace.join("\n")) unless exception.backtrace.nil?
-    end
-
-    alias log_exception_to_sentry log_exception # Method call in shared ExceptionHandling is :log_exception_to_sentry
-
-    def normalize_level(level, exception)
-      # https://docs.sentry.io/platforms/ruby/usage/set-level/
-      # valid sentry levels: log, debug, info, warning, error, fatal
-      level = case exception
-              when Pundit::NotAuthorizedError
-                'info'
-              when Common::Exceptions::BaseError
-                exception.sentry_type.to_s
-              else
-                level.to_s
-              end
-
-      return 'warning' if level == 'warn'
-
-      level
-    end
-
-    def rails_logger(level, message, errors = nil, backtrace = nil)
-      # rails logger uses 'warn' instead of 'warning'
-      level = 'warn' if level == 'warning'
-      if errors.present?
-        error_details = errors.first.attributes.compact.reject { |_k, v| v.try(:empty?) }
-        Rails.logger.send(level, message, error_details.merge(backtrace:))
-      else
-        Rails.logger.send(level, message)
-      end
-    end
   end
 end

modules/decision_reviews/spec/requests/v1/notice_of_disagreements_spec.rb

@@ -120,7 +120,9 @@ def personal_information_logs
           message: "Exception occurred while submitting Notice Of Disagreement: #{extra_error_log_message}",
           backtrace: anything
         )
-        expect(Rails.logger).to receive(:error).with(extra_error_log_message, anything)
+        expect(Rails.logger).to receive(:error) do |message|
+          expect(message).to include(extra_error_log_message)
+        end
         allow(StatsD).to receive(:increment)
         expect(StatsD).to receive(:increment).with('decision_review.form_10182.overall_claim_submission.failure')
         expect(personal_information_logs.count).to be 0

modules/decision_reviews/spec/requests/v1/supplemental_claims_spec.rb

@@ -106,7 +106,9 @@ def personal_information_logs
           message: "Exception occurred while submitting Supplemental Claim: #{extra_error_log_message}",
           backtrace: anything
         )
-        expect(Rails.logger).to receive(:error).with(extra_error_log_message, anything)
+        expect(Rails.logger).to receive(:error) do |message|
+          expect(message).to include(extra_error_log_message)
+        end
         allow(StatsD).to receive(:increment)
         expect(StatsD).to receive(:increment).with('decision_review.form_995.overall_claim_submission.failure')
 

modules/decision_reviews/spec/requests/v2/higher_level_reviews_spec.rb

@@ -107,7 +107,9 @@ def personal_information_logs
             message: "Exception occurred while submitting Higher Level Review: #{extra_error_log_message}",
             backtrace: anything
           )
-          expect(Rails.logger).to receive(:error).with(extra_error_log_message, anything)
+          expect(Rails.logger).to receive(:error) do |message|
+            expect(message).to include(extra_error_log_message)
+          end
           allow(StatsD).to receive(:increment)
           expect(StatsD).to receive(:increment).with('decision_review.form_996.overall_claim_submission.failure')
 

spec/controllers/application_controller_spec.rb

@@ -93,11 +93,6 @@ def append_info_to_payload(payload)
       before { allow(Settings.sentry).to receive(:dsn).and_return('asdf') }
 
       describe '#log_message_to_sentry' do
-        it 'error logs to Rails logger' do
-          expect(Rails.logger).to receive(:error).with(/blah/).with(/context/)
-          subject.log_message_to_sentry('blah', :error, { extra: 'context' }, tags: 'tagging')
-        end
-
         it 'logs to Sentry' do
           expect(Sentry).to receive(:set_tags)
           expect(Sentry).to receive(:set_extras)
@@ -106,17 +101,26 @@ def append_info_to_payload(payload)
         end
       end
 
-      describe '#log_exception_to_sentry' do
-        it 'warn logs to Rails logger' do
-          expect(Rails.logger).to receive(:error).with("#{exception.message}.")
-          subject.log_exception_to_sentry(exception)
+      describe '#log_message_to_rails' do
+        it 'error logs to Rails logger' do
+          expect(Rails.logger).to receive(:error).with(/blah/).with(/context/)
+          subject.log_message_to_rails('blah', :error, { extra: 'context' })
         end
+      end
 
+      describe '#log_exception_to_sentry' do
         it 'logs to Sentry' do
           expect(Sentry).to receive(:capture_exception).with(exception, level: 'error').once
           subject.log_exception_to_sentry(exception)
         end
       end
+
+      describe '#log_exception_to_rails' do
+        it 'warn logs to Rails logger' do
+          expect(Rails.logger).to receive(:error).with("#{exception.message}.")
+          subject.log_exception_to_rails(exception)
+        end
+      end
     end
 
     context 'without SENTRY_DSN set' do

spec/models/form_attachment_spec.rb

@@ -15,11 +15,12 @@
       let(:bad_password) { 'bad_pw' }
 
       context 'when provided password is incorrect' do
-        it 'logs a sanitized message to Sentry' do
+        it 'logs a sanitized message to Sentry and Rails' do
           error_message = nil
           allow_any_instance_of(FormAttachment).to receive(:log_message_to_sentry) do |_, message, _level|
             error_message = message
           end
+          allow(Rails.logger).to receive(:warn)
 
           tempfile = Tempfile.new(['', "-#{file_name}"])
           file = ActionDispatch::Http::UploadedFile.new(original_filename: file_name, type: 'application/pdf',
@@ -30,6 +31,7 @@
           end.to raise_error(Common::Exceptions::UnprocessableEntity)
           expect(error_message).not_to include(file_name)
           expect(error_message).not_to include(bad_password)
+          expect(Rails.logger).to have_received(:warn)
         end
       end
     end