Skip to content

Gracefully handle "sentry-trace" headers that don't match the regexp#1265

Merged
st0012 merged 1 commit intogetsentry:masterfrom
lfittl:patch-1
Feb 5, 2021
Merged

Gracefully handle "sentry-trace" headers that don't match the regexp#1265
st0012 merged 1 commit intogetsentry:masterfrom
lfittl:patch-1

Conversation

@lfittl
Copy link
Copy Markdown
Contributor

@lfittl lfittl commented Feb 4, 2021 •
edited
Loading

Fixes the handling of invalid "sentry-trace" header values, such as "null", that otherwise lead to an application crash.

Example error backtrace:

Rack app error handling request { POST /action }
#<NoMethodError: undefined method `[]' for nil:NilClass>
.../sentry-ruby-core-4.1.6/lib/sentry/transaction.rb:32:in `from_sentry_trace'
.../sentry-ruby-core-4.1.6/lib/sentry/rack/capture_exceptions.rb:21:in `block in call'
.../sentry-ruby-core-4.1.6/lib/sentry/hub.rb:52:in `with_scope'
.../sentry-ruby-core-4.1.6/lib/sentry-ruby.rb:149:in `with_scope'
.../sentry-ruby-core-4.1.6/lib/sentry/rack/capture_exceptions.rb:14:in `call'

@codecov-io
Copy link
Copy Markdown

codecov-io commented Feb 4, 2021 •
edited
Loading

Codecov Report

Merging #1265 (23cb83e) into master (db7be87) will increase coverage by 0.60%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1265      +/-   ##
==========================================
+ Coverage   97.98%   98.59%   +0.60%     
==========================================
  Files         200      105      -95     
  Lines        8655     4825    -3830     
==========================================
- Hits         8481     4757    -3724     
+ Misses        174       68     -106
Impacted Files Coverage Δ
sentry-ruby/lib/sentry/transaction.rb 100.00% <100.00%> (ø)
sentry-raven/spec/raven/transports/stdout_spec.rb
sentry-raven/lib/raven/transports/dummy.rb
...try-raven/lib/raven/utils/exception_cause_chain.rb
sentry-raven/lib/raven/cli.rb
...-raven/spec/raven/integrations/delayed_job_spec.rb
sentry-raven/lib/raven/core_ext/object/deep_dup.rb
...s/rails/overrides/debug_exceptions_catcher_spec.rb
...entry-raven/lib/raven/integrations/rack-timeout.rb
sentry-raven/spec/raven/breadcrumbs_spec.rb
... and 86 more

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 db7be87...23cb83e. Read the comment docs.

st0012
st0012 approved these changes Feb 4, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

thanks for the fix! can you add a new changelog entry for this as well? thx

@st0012 st0012 added this to the 4.2.0 milestone Feb 4, 2021
st0012
st0012 suggested changes Feb 4, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

actually, can you add a test case to capture_exceptions_spec.rb?

@lfittl
Copy link
Copy Markdown
Contributor Author

lfittl commented Feb 4, 2021

@st0012 Done - I've also found an adjacent bug, added in a follow-up commit in this PR, since its in the same code area (happy to move to a separate PR if you prefer).

@st0012
Copy link
Copy Markdown
Contributor

st0012 commented Feb 4, 2021

@lfittl thanks for the update 👍 some thoughts:

  1. I still want to see a test in spec/sentry/rack/capture_exceptions_spec.rb for the invalid sentry case. I suspect that this change breaks the middleware because it assumes span won't be nil in any case.
  2. the issue you mentioned is indeed a problem, but I'm not sure if adding the sampling decision is the right answer. I think the correct solution is to change sentry-rails's condition on instrumentation. I'll open a PR for that.

@st0012 st0012 modified the milestones: 4.2.0, sentry-ruby-4.2.1 Feb 4, 2021
@lfittl
Copy link
Copy Markdown
Contributor Author

lfittl commented Feb 4, 2021 •
edited
Loading

  1. I still want to see a test in spec/sentry/rack/capture_exceptions_spec.rb for the invalid sentry case. I suspect that this change breaks the middleware because it assumes span won't be nil in any case.

Good catch - fixed now. I assume we'd want to run through the regular logic in this case (i.e. if its an invalid value, act as if the header wasn't set)

  1. the issue you mentioned is indeed a problem, but I'm not sure if adding the sampling decision is the right answer. I think the correct solution is to change sentry-rails's condition on instrumentation. I'll open a PR for that.

Ack, removed again from this PR. Thanks!

st0012
st0012 reviewed Feb 4, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

thanks for catching the issue and proposed the great fix 😄

because I just released 4.2.0 earlier today, can you rebase the branch and update the changelog again?

# Changelog

## Unreleased <- Add this

< and put the entry here >

## 4.2.0
.....

@lfittl
Gracefully handle "sentry-trace" headers that don't match the regexp
82d1940 
Fixes the handling of invalid "sentry-trace" header values, such as "null".
@lfittl
Copy link
Copy Markdown
Contributor Author

lfittl commented Feb 4, 2021

@st0012 Done - rebased & updated change log entry.

@st0012 st0012 merged commit 71fd7ca into getsentry:master Feb 5, 2021
@st0012 st0012 mentioned this pull request Feb 5, 2021
Merged
@st0012
Copy link
Copy Markdown
Contributor

st0012 commented Feb 5, 2021

@lfittl I've talked to other maintainers about the other issue you spotted in this PR. and the answer is that the child transactions initialized from .from_sentry_trace should just inherit the parent's sampling decision. I've added #1269 to update this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@st0012 st0012 st0012 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

sentry-ruby Traces

Projects

None yet

Milestone

sentry-ruby-4.2.1

Development

Successfully merging this pull request may close these issues.

3 participants

@lfittl @codecov-io @st0012