getsentry / sentry-ruby

Sentry SDK for Ruby
https://sentry.io/for/ruby
MIT License
926 stars 493 forks source link

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

Open natikgadzhi opened 9 months ago

natikgadzhi commented 9 months ago

Summary

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

Closes #1864. Based on the approach in https://github.com/getsentry/sentry-javascript/pull/5485

Changes

Open questions

codecov[bot] commented 9 months ago

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 ```diff @@ 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](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2181/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | Coverage Δ | | |---|---|---| | [sentry-ruby](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2181/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | `55.75% <ø> (-42.40%)` | :arrow_down: | | [sentry-rails](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2181/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | `95.05% <100.00%> (+0.03%)` | :arrow_up: | | [sentry-sidekiq](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2181/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | `94.70% <ø> (ø)` | | | [sentry-resque](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2181/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | `93.65% <ø> (+1.58%)` | :arrow_up: | | [sentry-delayed_job](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2181/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | `94.44% <ø> (ø)` | | | [sentry-opentelemetry](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2181/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | `100.00% <ø> (ø)` | | | [Files](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2181?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | Coverage Δ | | |---|---|---| | [sentry-rails/lib/sentry/rails/action\_cable.rb](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2181?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry#diff-c2VudHJ5LXJhaWxzL2xpYi9zZW50cnkvcmFpbHMvYWN0aW9uX2NhYmxlLnJi) | `100.00% <100.00%> (ø)` | | | [...entry-rails/lib/sentry/rails/capture\_exceptions.rb](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2181?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry#diff-c2VudHJ5LXJhaWxzL2xpYi9zZW50cnkvcmFpbHMvY2FwdHVyZV9leGNlcHRpb25zLnJi) | `96.77% <100.00%> (+0.22%)` | :arrow_up: | ... and [56 files with indirect coverage changes](https://app.codecov.io/gh/getsentry/sentry-ruby/pull/2181/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry)
natikgadzhi commented 9 months ago

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

natikgadzhi commented 8 months ago

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

natikgadzhi commented 8 months ago

@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 commented 8 months ago

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)>'