Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-position CaptureExceptions middleware to reduce tracing noise #1405

Merged
merged 3 commits into from
Apr 19, 2021

Conversation

st0012
Copy link
Collaborator

@st0012 st0012 commented Apr 18, 2021

Right now, sentry-rails is smart enough to un-sample transactions of static file/assets requests.

But users may still see tracing logs in those requests like:

[Tracing] Starting <rails.request> transaction </favicon.ico>

This is because the current implementation un-samples those transactions after the initial sampling decision has been made.
The approach's downsides are:

  1. It requires hacking the FileHandler class.
  2. It generates annoying and sometimes confusing tracing logs, like the above one, for requests that are always un-sampled.

The reason behind it is the order of middlewares:

request 
  -> Sentry::CaptureExceptions (starts the transaction) 
  -> ActionDispatch::Static (checks if the request is for static files) 
      -> A patch from the SDK then manually un-sample the transaction if the request is for static files

As you can see, when starting the transaction, the SDK can't possibly know if the request is for static files unless Sentry::CaptureExceptions also knows how to check static file requests.

Solution - Re-position the middleware

When CaptureExceptions is the first middleware, the benefits are:

  1. It captures as many exceptions as possible.
  2. Transactions can record almost the full length of the request's events.

But there are also some downsides:

  1. Exceptions of the outter middlewares may mostly be noise to users, as they usually are the Rails default.
  2. It makes skipping some requests' transactions very hard. For example: Un-sample asset/static file requests in a less-hacky way #1404

By re-positioning the middleware after ActionDispatch::Executor, we can avoid the downsides while still keeping most of the benefits. It also allows us to remove the patch on FileHandler.

@st0012 st0012 added this to the 4.4.0 milestone Apr 18, 2021
@st0012 st0012 self-assigned this Apr 18, 2021
@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@f82a2c3). Click here to learn what that means.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1405   +/-   ##
=========================================
  Coverage          ?   98.47%           
=========================================
  Files             ?       33           
  Lines             ?     1113           
  Branches          ?        0           
=========================================
  Hits              ?     1096           
  Misses            ?       17           
  Partials          ?        0           
Impacted Files Coverage Δ
...entry-rails/lib/sentry/rails/capture_exceptions.rb 96.00% <100.00%> (ø)
sentry-rails/lib/sentry/rails/railtie.rb 100.00% <100.00%> (ø)
sentry-rails/spec/sentry/rails/tracing_spec.rb 99.13% <100.00%> (ø)
sentry-rails/spec/sentry/rails_spec.rb 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f82a2c3...5ffa912. Read the comment docs.

@st0012 st0012 force-pushed the improve-sentry-rails-tracing-sampling-2 branch from 4a62c22 to 953e68c Compare April 18, 2021 09:06
When `CaptureExceptions` is the first middleware, the benefits are:

1. It captures as many exceptions as possible.
2. Transactions can record almost the full length of the request's
   events.

But there are also some downsides:

1. Exceptions of the outter middlewares may mostly be noise to users, as
   they usually are the Rails default.
2. It makes skipping some requests' transactions very hard. For example:
  #1404

By re-positioning the middleware after `ActionDispatch::Executor`, we
can automatically avoid the downsides while still keeping most of the
benefits. It also allows us to remove the patch on `FileHandler`.
@st0012 st0012 force-pushed the improve-sentry-rails-tracing-sampling-2 branch from 953e68c to bab9daa Compare April 18, 2021 09:14
@st0012 st0012 merged commit 73abda3 into master Apr 19, 2021
@st0012 st0012 deleted the improve-sentry-rails-tracing-sampling-2 branch April 19, 2021 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants