Fix trace_id mismatch for logs emitted before Sentry::Rails::CaptureExceptions runs#3016
Open
runephilosof-abtion wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 096916b. Configure here.
…xceptions runs Rails::Rack::Logger's "Started ..." line (and anything else logged before CaptureExceptions runs) previously got a trace_id/span_id unrelated to the rest of the request, because CaptureExceptions is deliberately positioned after ActionDispatch::ShowExceptions (to skip transactions for static asset requests) - so it hadn't established any trace context yet when earlier middleware logged. This adds a new, minimal Sentry::Rails::CaptureContext middleware that is unshifted to the very front of the middleware stack and only establishes the propagation context (from incoming sentry-trace/baggage headers, if any) as early as possible, without touching where CaptureExceptions itself runs. To make CaptureExceptions actually reuse that early context instead of discarding/regenerating it: - Sentry::PropagationContext::ESTABLISHED_ENV_KEY is a new env flag that signals trace context was already established for this exact request. - Hub#continue_trace skips regenerating the propagation context when the flag is present. - Sentry::Rack::CaptureExceptions skips re-cloning the hub when the flag is present. - Hub#start_transaction, when not continuing a distributed trace and the flag is present (via custom_sampling_context[:env]), builds the new transaction with the already-established trace_id/sample_rand instead of generating an unrelated one. This was needed because Transaction.new otherwise always generates its own independent trace_id when not continuing an incoming trace, which would silently diverge from the propagation context even after the other two fixes. The trace_id reuse in start_transaction is intentionally scoped to only when the established flag is present, to avoid incorrectly linking together unrelated transactions that happen to share a thread (e.g. separate background jobs), which have their own propagation context by default but were never intended to share a trace_id with each other. Co-Authored-By: GitHub Copilot <noreply@example.com>
096916b to
080acef
Compare
Author
|
Should I cut down on the inline code comments? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Fixes #3015
Problem
Sentry::Rails::CaptureExceptionsis deliberately positioned right afterActionDispatch::ShowExceptions(to skip transaction creation for static asset requests). That means anything logged before it runs - most notably Rails' ownRails::Rack::Logger"Started ..." line - gets atrace_id/span_idunrelated to the rest of the request, because no trace context has been established yet.There's also a second, compounding issue: even once
CaptureExceptionsdoes run, if the request isn't continuing an incoming distributed trace,Hub#start_transaction's fallback (Transaction.new(**options)) generates its own independenttrace_id, ignoring whatever was already on the scope's propagation context.Fix
Sentry::Rails::CaptureContextmiddleware, unshifted to the very front of the Rails middleware stack. It only establishes the propagation context (from incomingsentry-trace/baggageheaders, if present) as early as possible - it doesn't start a transaction or capture exceptions, soCaptureExceptions's existing position/behavior is untouched.Sentry::PropagationContext::ESTABLISHED_ENV_KEY- a new env flag signaling that trace context was already established for this exact request.Hub#continue_traceskips regenerating the propagation context when the flag is present, instead of clobbering whatCaptureContextset up.Sentry::Rack::CaptureExceptionsskips re-cloning the hub when the flag is present, for the same reason.Hub#start_transaction, when not continuing a distributed trace and the flag is present (viacustom_sampling_context[:env]), builds the new transaction with the already-established trace_id/sample_rand instead of generating an unrelated one.The
start_transactionchange is intentionally scoped to only apply when the established flag is present (rather than always reusing whatever's on the scope), to avoid incorrectly linking together unrelated transactions that happen to share a thread (e.g. separate background jobs) - each of those still gets its own independent trace_id as before. This was verified against a regression insentry-rails' ActiveJob distributed-tracing specs during development.Testing
sentry-rails/spec/sentry/rails/capture_context_spec.rb, including an integration-style test that reproduces the original bug (probe middleware inserted before/afterCaptureExceptions) both with and without tracing enabled.sentry-ruby'shub/capture_exceptionsspecs for the new established-context guard behavior.sentry-rails' existing middleware-ordering spec to assertCaptureContextis the first middleware.sentry-rubyandsentry-railssuites locally; no regressions (Redis-dependent specs fail identically with/without this change due to no local Redis server, pre-existing/environment-only).