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

Tracing: don't record HTTP OPTIONS and HEAD transactions #2181

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

natikgadzhi
Copy link
Contributor

@natikgadzhi natikgadzhi commented Nov 26, 2023

Summary

This pull request makes it so HTTP OPTIONS and HEAD requests will not be sampled (traced).

Closes #1864. Based on the approach in getsentry/sentry-javascript#5485

Changes

  • Adds a check for Rack env http method in capture_exceptions rack app middleware. It felt like the right spot for this, instead of shoving another check in Transaction.set_initial_sample_decision, because it depends directly on an HTTP request, and thus is rather specific to Rack env.
  • Adds a changelog entry
  • This needs unit tests, but capture_exceptions_spec.rb is scary. I'll write them up tomorrow.

Open questions

  • @sl0thentr0py, @st0012, is this the right approach and the right place for this check?
  • Another note: this approach ignores the incoming trace. If I'm reading the Javascript implementation correctly, it does the same, since the HTTP method check is the first one in the tracing middleware.

Copy link

codecov bot commented Nov 26, 2023

Codecov Report

Merging #2181 (d8891f3) into master (49a628e) will decrease coverage by 30.97%.
The diff coverage is 100.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #2181       +/-   ##
===========================================
- Coverage   97.43%   66.47%   -30.97%     
===========================================
  Files         102      100        -2     
  Lines        3817     3749       -68     
===========================================
- Hits         3719     2492     -1227     
- Misses         98     1257     +1159     
Components Coverage Δ
sentry-ruby 55.75% <ø> (-42.40%) ⬇️
sentry-rails 95.05% <100.00%> (+0.03%) ⬆️
sentry-sidekiq 94.70% <ø> (ø)
sentry-resque 93.65% <ø> (+1.58%) ⬆️
sentry-delayed_job 94.44% <ø> (ø)
sentry-opentelemetry 100.00% <ø> (ø)
Files Coverage Δ
sentry-rails/lib/sentry/rails/action_cable.rb 100.00% <100.00%> (ø)
...entry-rails/lib/sentry/rails/capture_exceptions.rb 96.77% <100.00%> (+0.22%) ⬆️

... and 56 files with indirect coverage changes

@natikgadzhi
Copy link
Contributor Author

Added the unit tests, and moved the changelog entry to Unreleased.

@natikgadzhi natikgadzhi force-pushed the ng/feat/performance/no-traces-for-options branch from cb4cd77 to 923eb4e Compare November 27, 2023 20:32
@natikgadzhi natikgadzhi force-pushed the ng/feat/performance/no-traces-for-options branch from 923eb4e to 5d990bc Compare November 30, 2023 05:34
Copy link
Member

@sl0thentr0py sl0thentr0py left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you'll need the same logic in

def start_transaction(env, scope)

and
def start_transaction(env, scope)

sentry-ruby/lib/sentry/transaction.rb Outdated Show resolved Hide resolved
@natikgadzhi
Copy link
Contributor Author

Just passing by here — I'll clean this up today so we can get it merged! Sorry for the delay.

@natikgadzhi natikgadzhi force-pushed the ng/feat/performance/no-traces-for-options branch from 5d990bc to f670ad4 Compare January 4, 2024 04:56
@natikgadzhi natikgadzhi force-pushed the ng/feat/performance/no-traces-for-options branch from f670ad4 to 7afd73d Compare January 4, 2024 04:58
@natikgadzhi
Copy link
Contributor Author

@sl0thentr0py, thank you for the guidance <3

I've pushed up the code, with two gotchas where I'd love your input and preference:

  1. I've duplicated the IGNORED_HTTP_METHODS in the three classes that use it. In theory, we could just put it in module Sentry, and use it with Sentry::IGNORED_HTTP_METHODS, but I think having it in the same file is a bit more readable. Want me to DRY it out?
  2. I have not yet added the tests for the Rails and ActionCable pieces. I can get back to this, or we can ship this as is — up to you!

@natikgadzhi
Copy link
Contributor Author

Hmmmmmmmm

   1) Sentry::Client#event_from_transaction correct dynamic_sampling_context when head SDK
     Failure/Error:
       expect(event.dynamic_sampling_context).to eq({
         "environment" => "development",
         "public_key" => "12345",
         "sample_rate" => "1.0",
         "sampled" => "true",
         "transaction" => "test transaction",
         "trace_id" => transaction.trace_id
       })

       expected: {"environment"=>"development", "public_key"=>"12345", "sample_rate"=>"1.0", "sampled"=>"true", "trace_id"=>"f86126d5bfb14f5d8716c5459fe6495b", "transaction"=>"test transaction"}
            got: {"environment"=>"development", "public_key"=>"12345", "sample_rate"=>"0.5", "sampled"=>"true", "trace_id"=>"f86126d5bfb14f5d8716c5459fe6495b", "transaction"=>"test transaction"}

       (compared using ==)

       Diff:
       @@ -1,6 +1,6 @@
        "environment" => "development",
        "public_key" => "12345",
       -"sample_rate" => "1.0",
       +"sample_rate" => "0.5",
        "sampled" => "true",
        "trace_id" => "f86126d5bfb14f5d8716c5459fe6495b",
        "transaction" => "test transaction",
     # ./spec/sentry/client_spec.rb:183:in `block (3 levels) in <top (required)>'
     

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't trace OPTIONS requests by default
3 participants