diff --git a/.changeset/olive-cats-take.md b/.changeset/olive-cats-take.md new file mode 100644 index 00000000..8537bc70 --- /dev/null +++ b/.changeset/olive-cats-take.md @@ -0,0 +1,10 @@ +--- +"posthog-rails": patch +"posthog-ruby": patch +--- + +Capture handled errors reported through `Rails.error` during web requests by replacing Rails-specific suppression with shared exception deduplication. + +For mutable exceptions, `capture_exception` now suppresses subsequent captures of the same exception object and its captured causes, including manual calls and calls through another client. The first successful capture wins; subsequent calls return `false` without merging context. Supply the required identity and context on the first report. Failed captures remain retryable. + +Remove `PostHog::Rails::IN_WEB_REQUEST_KEY` and the request-tracking and exception-marker helpers documented as private. Remove any application references to these integration internals. diff --git a/lib/posthog/client.rb b/lib/posthog/client.rb index 0707a48c..05a86233 100644 --- a/lib/posthog/client.rb +++ b/lib/posthog/client.rb @@ -54,6 +54,9 @@ class Client RELEASE_ID_ENV_VAR = 'POSTHOG_RELEASE_ID' private_constant :RELEASE_ID_ENV_VAR + CAPTURED_EXCEPTION_IVAR = :@posthog_exception_captured + private_constant :CAPTURED_EXCEPTION_IVAR + # Thread-safe tracking of client instances per API key for singleton warnings @instances_by_api_key = {} @instances_mutex = Mutex.new @@ -410,7 +413,7 @@ def capture(attrs) # Defaults to `{ 'type' => 'generic', 'handled' => true }` for manual captures. # @return [Boolean, nil] Whether the exception event was queued or sent, or nil if the input could not be parsed. def capture_exception(exception, distinct_id = nil, additional_properties = {}, flags: nil, mechanism: nil) - return false if @disabled + return false if @disabled || exception_captured?(exception) exception_list = ExceptionCapture.build_exception_list(exception, mechanism: mechanism) @@ -427,7 +430,9 @@ def capture_exception(exception, distinct_id = nil, additional_properties = {}, } event_data[:flags] = flags if flags - capture(event_data) + capture(event_data).tap do |captured| + mark_exception_captured(exception) if captured + end end # Identifies a user @@ -978,6 +983,25 @@ def shutdown(timeout: nil) private + def exception_captured?(exception) + exception.is_a?(Exception) && exception.instance_variable_get(CAPTURED_EXCEPTION_IVAR).equal?(exception) + rescue StandardError + false + end + + # Rails can unwrap template errors, so mark the causes included in the event too. + def mark_exception_captured(exception) + ExceptionCapture::MAX_CHAINED_EXCEPTIONS.times do + break unless exception.is_a?(Exception) + break if exception_captured?(exception) + + exception.instance_variable_set(CAPTURED_EXCEPTION_IVAR, exception) unless exception.frozen? + exception = exception.cause + end + rescue StandardError + nil + end + def enrich_capture_attrs_with_context(attrs) context = Internal::Context.current explicit_properties = attrs[:properties] @@ -1152,10 +1176,7 @@ def enqueue(action) action = process_before_send(action) return false if action.nil? || action.empty? - if @sync_mode - send_sync(action) - return true - end + return send_sync(action) if @sync_mode queued = @queue_mutex.synchronize do if @queue.length < @max_queue_size @@ -1223,13 +1244,15 @@ def send_sync(action) batch << action rescue MessageBatch::JSONGenerationError => e @on_error.call(-1, e.to_s) - return + return false end - return if batch.empty? + return false if batch.empty? @sync_lock.synchronize do res = @transport.send(@api_key, batch) - @on_error.call(res.status, res.error) unless res.status.between?(200, 299) + success = res.status.between?(200, 299) + @on_error.call(res.status, res.error) unless success + success end end diff --git a/posthog-rails/lib/posthog/rails.rb b/posthog-rails/lib/posthog/rails.rb index 7a70f32d..36240784 100644 --- a/posthog-rails/lib/posthog/rails.rb +++ b/posthog-rails/lib/posthog/rails.rb @@ -18,14 +18,6 @@ module PostHog module Rails - # Thread-local key for tracking web request context - IN_WEB_REQUEST_KEY = :posthog_in_web_request - ACTIVE_JOB_CAPTURED_EXCEPTION_IVAR = :@posthog_active_job_exception_captured - WEB_CAPTURED_EXCEPTION_IVAR = :@posthog_web_exception_captured - - private_constant :ACTIVE_JOB_CAPTURED_EXCEPTION_IVAR - private_constant :WEB_CAPTURED_EXCEPTION_IVAR - class << self # @return [PostHog::Rails::Configuration] Rails integration configuration. def config @@ -42,87 +34,6 @@ def config def configure yield config if block_given? end - - # Mark that we're in a web request context - # CaptureExceptions middleware will handle exception capture - # @api private - # @return [void] - def enter_web_request - Thread.current[IN_WEB_REQUEST_KEY] = true - end - - # Clear web request context (called at end of request) - # @api private - # @return [void] - def exit_web_request - Thread.current[IN_WEB_REQUEST_KEY] = false - end - - # Check if we're currently in a web request context - # Used by ErrorSubscriber to avoid duplicate captures - # @api private - # @return [Boolean] - def in_web_request? - Thread.current[IN_WEB_REQUEST_KEY] == true - end - - # Mark an exception as already captured by the ActiveJob integration. - # Used by ErrorSubscriber to avoid duplicate captures when Rails reports - # the same re-raised job exception via Rails.error. - # @api private - # @param exception [Exception] - # @return [void] - def mark_active_job_exception_captured(exception) - exception.instance_variable_set(ACTIVE_JOB_CAPTURED_EXCEPTION_IVAR, true) - rescue StandardError - nil - end - - # Check whether an exception was already captured by ActiveJobExtensions. - # @api private - # @param exception [Exception] - # @return [Boolean] - def active_job_exception_captured?(exception) - exception.instance_variable_get(ACTIVE_JOB_CAPTURED_EXCEPTION_IVAR) == true - rescue StandardError - false - end - - # Mark an exception as already captured by the CaptureExceptions middleware. - # - # ActionDispatch::Executor sits above our middleware and reports unhandled - # exceptions to Rails.error *after* the response has unwound back through - # CaptureExceptions, so `in_web_request?` is already false by then. The mark - # lets ErrorSubscriber recognize the report as a duplicate. - # - # The whole cause chain is marked because ActionDispatch::ExceptionWrapper - # unwraps ActionView::Template::Error to its cause, meaning Rails can report - # a different object than the one the middleware captured. - # @api private - # @param exception [Exception] - # @return [void] - def mark_web_exception_captured(exception) - current = exception - seen = {}.compare_by_identity - - while current.is_a?(Exception) && !seen[current] - seen[current] = true - current.instance_variable_set(WEB_CAPTURED_EXCEPTION_IVAR, true) - current = current.cause - end - rescue StandardError - nil - end - - # Check whether an exception was already captured by CaptureExceptions. - # @api private - # @param exception [Exception] - # @return [Boolean] - def web_exception_captured?(exception) - exception.instance_variable_get(WEB_CAPTURED_EXCEPTION_IVAR) == true - rescue StandardError - false - end end end end diff --git a/posthog-rails/lib/posthog/rails/active_job.rb b/posthog-rails/lib/posthog/rails/active_job.rb index b621ae8e..a7842f88 100644 --- a/posthog-rails/lib/posthog/rails/active_job.rb +++ b/posthog-rails/lib/posthog/rails/active_job.rb @@ -70,7 +70,6 @@ def capture_job_exception(exception) properties, mechanism: { 'type' => 'active_job', 'handled' => false } ) - PostHog::Rails.mark_active_job_exception_captured(exception) rescue StandardError => e # Don't let PostHog errors break job processing PostHog::Logging.logger.error("Failed to capture job exception: #{e.message}") diff --git a/posthog-rails/lib/posthog/rails/capture_exceptions.rb b/posthog-rails/lib/posthog/rails/capture_exceptions.rb index e762a5ad..6cbd8657 100644 --- a/posthog-rails/lib/posthog/rails/capture_exceptions.rb +++ b/posthog-rails/lib/posthog/rails/capture_exceptions.rb @@ -18,10 +18,6 @@ def initialize(app) # @param env [Hash] Rack environment. # @return [Array] Rack response. def call(env) - # Signal that we're in a web request context - # ErrorSubscriber will skip capture for web requests to avoid duplicates - PostHog::Rails.enter_web_request - response = @app.call(env) env['posthog.response_status_code'] = response_status(response) @@ -35,8 +31,6 @@ def call(env) # Capture unhandled exceptions capture_exception(e, env) if should_capture?(e) raise - ensure - PostHog::Rails.exit_web_request end private @@ -60,13 +54,12 @@ def capture_exception(exception, env) distinct_id = extract_distinct_id(env) additional_properties = build_properties(request, env) - captured = PostHog.capture_exception( + PostHog.capture_exception( exception, distinct_id, additional_properties, mechanism: { 'type' => 'rails', 'handled' => false } ) - PostHog::Rails.mark_web_exception_captured(exception) if captured rescue StandardError => e PostHog::Logging.logger.error("Failed to capture exception: #{e.message}") PostHog::Logging.logger.error("Backtrace: #{e.backtrace&.first(5)&.join("\n")}") diff --git a/posthog-rails/lib/posthog/rails/error_subscriber.rb b/posthog-rails/lib/posthog/rails/error_subscriber.rb index a1f0890a..207a8790 100644 --- a/posthog-rails/lib/posthog/rails/error_subscriber.rb +++ b/posthog-rails/lib/posthog/rails/error_subscriber.rb @@ -20,15 +20,6 @@ class ErrorSubscriber def report(error, handled:, severity:, context:, source: nil) return unless PostHog::Rails.config&.auto_capture_exceptions return unless PostHog::Rails.config&.should_capture_exception?(error) - # Skip if in a web request - CaptureExceptions middleware will handle it - # with richer context (URL, params, controller, etc.) - return if PostHog::Rails.in_web_request? - # ActionDispatch::Executor reports the exception once the response has - # unwound past our middleware, so the flag above is already cleared. - # CaptureExceptions marks what it captured to catch that late report. - return if PostHog::Rails.web_exception_captured?(error) - return if PostHog::Rails.config&.auto_instrument_active_job && - PostHog::Rails.active_job_exception_captured?(error) distinct_id = context[:user_id] || context[:distinct_id] diff --git a/public_api_snapshot.txt b/public_api_snapshot.txt index 4930d38e..16f70bd6 100644 --- a/public_api_snapshot.txt +++ b/public_api_snapshot.txt @@ -327,7 +327,6 @@ instance_method PostHog::Rails::Configuration#use_tracing_headers=(arg0) instance_method PostHog::Rails::Configuration#user_id_method() instance_method PostHog::Rails::Configuration#user_id_method=(arg0) constant PostHog::Rails::Configuration::DEFAULT_LOGS_MAX_RECORDS_PER_MINUTE: Integer -constant PostHog::Rails::IN_WEB_REQUEST_KEY: Symbol class PostHog::Rails::InitConfig instance_method PostHog::Rails::InitConfig#api_key=(value) instance_method PostHog::Rails::InitConfig#before_send=(value) diff --git a/spec/posthog/client_spec.rb b/spec/posthog/client_spec.rb index b992eb74..ab446e98 100644 --- a/spec/posthog/client_spec.rb +++ b/spec/posthog/client_spec.rb @@ -274,11 +274,36 @@ def shutdown ) sync_client = Client.new(api_key: API_KEY, sync_mode: true, on_error: on_error) - sync_client.capture(Queued::CAPTURE) + expect(sync_client.capture(Queued::CAPTURE)).to be(false) expect(error_status).to eq(400) end + it 'allows an exception to be retried after a failed synchronous request' do + PostHog::Transport.stub = false + request = stub_request(:post, 'https://us.i.posthog.com/batch/').to_return(status: 400).then + .to_return(status: 200) + on_error = proc {} + expect(on_error).to receive(:call).with(400, anything).once + sync_client = Client.new(api_key: API_KEY, sync_mode: true, on_error: on_error) + error = StandardError.new('boom') + + expect(sync_client.capture_exception(error)).to be(false) + expect(sync_client.capture_exception(error)).to be(true) + expect(sync_client.capture_exception(error)).to be(false) + expect(request).to have_been_requested.twice + end + + it 'allows an exception to be retried after an oversized event was dropped' do + sync_client = Client.new(api_key: API_KEY, sync_mode: true) + error = StandardError.new('boom') + properties = { details: 'x' * PostHog::Defaults::Message::MAX_BYTES } + + expect(sync_client.capture_exception(error, nil, properties)).to be(false) + expect(sync_client.capture_exception(error)).to be(true) + expect(sync_client.capture_exception(error)).to be(false) + end + [201, 202, 204].each do |status| it "does not call on_error for a #{status} response" do on_error = proc {} @@ -289,7 +314,7 @@ def shutdown ) sync_client = Client.new(api_key: API_KEY, sync_mode: true, on_error: on_error) - sync_client.capture(Queued::CAPTURE) + expect(sync_client.capture(Queued::CAPTURE)).to be(true) end end @@ -327,7 +352,7 @@ def shutdown ) sync_client = Client.new(api_key: API_KEY, sync_mode: true, on_error: on_error) - sync_client.capture(Queued::CAPTURE) + expect(sync_client.capture(Queued::CAPTURE)).to be(false) expect(error_status).to eq(-1) expect(error_message).to include('Serialization error') @@ -2139,6 +2164,7 @@ def release raise StandardError, 'Test exception' rescue StandardError => e client.capture_exception(e, 'user-123', { 'user_agent' => 'Test Agent', 'request_id' => 'req-123' }) + expect(client.capture_exception(e)).to be(false) end message = client.dequeue_last_message @@ -2162,6 +2188,54 @@ def release expect(client.dequeue_last_message[:timestamp]).to eq('2024-07-16T04:00:00.123Z') end + it 'does not mark an exception when it could not be queued' do + error = StandardError.new('boom') + allow(client).to receive(:capture).and_return(false, true) + + expect(client.capture_exception(error)).to be(false) + expect(client.capture_exception(error)).to be(true) + expect(client.capture_exception(error)).to be(false) + end + + %i[dup clone].each do |operation| + it "captures a distinct exception created with #{operation}" do + error = StandardError.new('boom') + expect(client.capture_exception(error)).to be(true) + + copy = error.public_send(operation) + expect(client.capture_exception(copy)).to be(true) + expect(client.capture_exception(copy)).to be(false) + expect(client.queued_messages).to eq(2) + end + end + + it 'captures a replacement exception raised with a different message' do + error = StandardError.new('original') + client.capture_exception(error) + + expect { raise error, 'different failure' }.to raise_error('different failure') do |replacement| + expect(client.capture_exception(replacement)).to be(true) + end + + expect(client.queued_messages).to eq(2) + expect(client.dequeue_last_message[:properties]['$exception_list'].first['value']).to eq('original') + expect(client.dequeue_last_message[:properties]['$exception_list'].first['value']).to eq('different failure') + end + + %i[instance_variable_get instance_variable_set].each do |operation| + it "still captures when the exception marker's #{operation} fails" do + error = StandardError.new('boom') + allow(error).to receive(operation).and_raise(StandardError, 'marker failed') + + expect(client.capture_exception(error)).to be(true) + expect(client.queued_messages).to eq(1) + end + end + + it 'can capture frozen exceptions' do + expect(client.capture_exception(StandardError.new('boom').freeze)).to be(true) + end + it 'captures the full cause chain outermost-first' do begin begin @@ -2171,6 +2245,7 @@ def release end rescue StandardError => e client.capture_exception(e, 'user-123') + expect(client.capture_exception(e.cause)).to be(false) end message = client.dequeue_last_message diff --git a/spec/posthog/rails/exception_mechanism_spec.rb b/spec/posthog/rails/exception_mechanism_spec.rb index c00ebb9c..556401e8 100644 --- a/spec/posthog/rails/exception_mechanism_spec.rb +++ b/spec/posthog/rails/exception_mechanism_spec.rb @@ -20,8 +20,39 @@ PostHog::Rails.config = previous_config end + let(:client) { PostHog::Client.new(api_key: API_KEY, test_mode: true) } + before do - allow(PostHog).to receive(:capture_exception) + allow(PostHog).to receive(:client).and_return(client) + allow(PostHog).to receive(:capture_exception).and_call_original + end + + describe 'Rails error reports during requests' do + let(:reporter) { ActiveSupport::ErrorReporter.new.tap { |r| r.subscribe(PostHog::Rails::ErrorSubscriber.new) } } + + it 'captures handled errors with the existing request and reporter context' do + app = lambda do |_env| + reporter.handle(context: { distinct_id: 'shop-123', shop_id: 123 }) { raise 'handled' } + [200, {}, []] + end + middleware = PostHog::Rails::RequestContext.new(PostHog::Rails::CaptureExceptions.new(app)) + middleware.call(Rack::MockRequest.env_for('/orders')) + + expect(client.queued_messages).to eq(1) + event = client.dequeue_last_message + expect(event[:distinct_id]).to eq('shop-123') + expect(event[:properties]).to include('$request_path' => '/orders', '$context_shop_id' => 123) + expect(event[:properties]['$exception_list'].first['mechanism']['handled']).to be(true) + end + + it 'does not duplicate an error reported before it escapes the request' do + app = ->(_env) { reporter.record { raise 'unhandled' } } + middleware = PostHog::Rails::CaptureExceptions.new(app) + + expect { middleware.call(Rack::MockRequest.env_for('/orders')) }.to raise_error('unhandled') + expect(client.queued_messages).to eq(1) + expect(client.dequeue_last_message[:properties]['$exception_list'].first['mechanism']['handled']).to be(false) + end end describe PostHog::Rails::CaptureExceptions do @@ -41,7 +72,6 @@ end it 'prevents the Rails error subscriber from capturing the same web exception again' do - allow(PostHog).to receive(:capture_exception).and_return(true) app = ->(_env) { raise StandardError, 'boom' } middleware = described_class.new(app) env = Rack::MockRequest.env_for('/api/test') @@ -53,11 +83,6 @@ error = e end - # ActionDispatch::Executor sits above this middleware and reports to - # Rails.error only after the response has unwound past it, so the - # in-web-request flag is already cleared by the time the report lands. - expect(PostHog::Rails.in_web_request?).to be(false) - PostHog::Rails::ErrorSubscriber.new.report( error, handled: false, @@ -66,7 +91,7 @@ source: 'application.action_dispatch' ) - expect(PostHog).to have_received(:capture_exception).once + expect(client.queued_messages).to eq(1) expect(PostHog).to have_received(:capture_exception).with( an_instance_of(StandardError), anything, @@ -131,7 +156,7 @@ it 'skips exceptions already captured by ActiveJobExtensions' do PostHog::Rails.config.auto_instrument_active_job = true error = StandardError.new('boom') - PostHog::Rails.mark_active_job_exception_captured(error) + client.capture_exception(error) described_class.new.report( error, @@ -141,12 +166,12 @@ source: 'application.active_support' ) - expect(PostHog).not_to have_received(:capture_exception) + expect(client.queued_messages).to eq(1) end it 'skips exceptions already captured by CaptureExceptions' do error = StandardError.new('boom') - PostHog::Rails.mark_web_exception_captured(error) + client.capture_exception(error) described_class.new.report( error, @@ -156,7 +181,7 @@ source: 'application.action_dispatch' ) - expect(PostHog).not_to have_received(:capture_exception) + expect(client.queued_messages).to eq(1) end it 'skips the unwrapped cause of an exception already captured by CaptureExceptions' do @@ -170,7 +195,7 @@ end end - PostHog::Rails.mark_web_exception_captured(wrapped) + client.capture_exception(wrapped) # ActionDispatch::ExceptionWrapper unwraps ActionView::Template::Error to # its cause, so Rails reports a different object than the one captured. @@ -182,7 +207,7 @@ source: 'application.action_dispatch' ) - expect(PostHog).not_to have_received(:capture_exception) + expect(client.queued_messages).to eq(1) end it 'still captures unmarked ActiveSupport reports when ActiveJob instrumentation is enabled' do @@ -256,7 +281,6 @@ def perform_now it 'prevents Rails error subscriber from capturing the same job exception again' do PostHog::Rails.config.auto_instrument_active_job = true - allow(PostHog).to receive(:capture_exception).and_return(true) error = nil begin @@ -275,7 +299,8 @@ def perform_now source: 'application.active_support' ) - expect(PostHog).to have_received(:capture_exception).once + expect(client.queued_messages).to eq(1) + expect(client.dequeue_last_message[:properties]).to include('$job_class' => 'FakeJob', '$job_id' => 'job-1') expect(PostHog).to have_received(:capture_exception).with( an_instance_of(StandardError), anything, @@ -283,5 +308,21 @@ def perform_now mechanism: { 'type' => 'active_job', 'handled' => false } ) end + + it 'allows a Rails report to retry when the job event was dropped' do + PostHog::Rails.config.auto_instrument_active_job = true + attempts = 0 + retry_client = PostHog::Client.new(api_key: API_KEY, test_mode: true, before_send: lambda { |event| + attempts += 1 + event if attempts > 1 + }) + allow(PostHog).to receive(:client).and_return(retry_client) + + expect { job_class.new.perform_now }.to raise_error(StandardError, 'job failed') do |error| + PostHog::Rails::ErrorSubscriber.new.report(error, handled: false, severity: :error, context: {}) + end + + expect(retry_client.queued_messages).to eq(1) + end end end