Skip to content

Fix headers serialization for sentry-ruby#1197

Merged
st0012 merged 1 commit intogetsentry:masterfrom
moofkit:fix-headers-serailization
Jan 13, 2021
Merged

Fix headers serialization for sentry-ruby#1197
st0012 merged 1 commit intogetsentry:masterfrom
moofkit:fix-headers-serailization

Conversation

@moofkit
Copy link
Copy Markdown
Contributor

@moofkit moofkit commented Jan 12, 2021

Description

There is performance issue with sentry-raven and sentry-ruby gems.

In some cases different middlewares assign to env hash some large objects.
I.e. grape expose his routing in env['grape.routing_args'] https://github.com/ruby-grape/grape/blob/02d7113d09eb9fcb4264c841d1fdd305e3e8adb5/lib/grape/util/env.rb#L22

In large applications it may be havy hash object. So when sentry try to cast this with #to_s that causes to large CPU consumption. I.e. in our application it takes around 45 sec to perform and almost 100% CPU load).
And the result just throwed away because this is not headers that needs to be in the event.

There are some demo that emulates this issue https://gist.github.com/moofkit/e1bf1ca0d3186aa7f9b89d3c3ee7fc58
You can try it with

$ make start
$ make test
> time curl 'http://localhost:9292/exception'
{"error":"An error ocurred"}       10.11 real         0.00 user         0.00 sys

Why it takes 10 secs of 5 is covered in this issue #777

So, this little patch postpone Object#to_s after all headers checks in both sentry-raven and sentry-ruby gems

@moofkit moofkit changed the title Fix headers serailization Fix headers serialization Jan 12, 2021
@st0012 st0012 self-assigned this Jan 12, 2021
@st0012
Copy link
Copy Markdown
Contributor

st0012 commented Jan 12, 2021

@moofkit thanks for pointing this out & the PR! I'd love to merge it right away, but we're managing the release of these 2 gems separately. so can you split this PR into 2?

@moofkit
Copy link
Copy Markdown
Contributor Author

moofkit commented Jan 12, 2021

@st0012 sure

@moofkit moofkit force-pushed the fix-headers-serailization branch from efcec43 to 3826300 Compare January 12, 2021 17:43
@codecov-io
Copy link
Copy Markdown

codecov-io commented Jan 12, 2021

Codecov Report

Merging #1197 (3826300) into master (703beed) will increase coverage by 0.51%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1197      +/-   ##
==========================================
+ Coverage   97.99%   98.50%   +0.51%     
==========================================
  Files         192       97      -95     
  Lines        8181     4361    -3820     
==========================================
- Hits         8017     4296    -3721     
+ Misses        164       65      -99     
Impacted Files Coverage Δ
sentry-ruby/lib/sentry/interfaces/request.rb 94.73% <100.00%> (-3.54%) ⬇️
...y/spec/sentry/interfaces/request_interface_spec.rb 99.05% <100.00%> (-0.95%) ⬇️
sentry-ruby/lib/sentry-ruby.rb 98.75% <0.00%> (-1.25%) ⬇️
sentry-raven/spec/raven/interface_spec.rb
...raven/spec/raven/processors/utf8conversion_spec.rb
sentry-raven/spec/raven/event_spec.rb
sentry-raven/lib/raven/interfaces/http.rb
...en/spec/raven/integrations/rails/activejob_spec.rb
... and 90 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 703beed...3826300. Read the comment docs.

@moofkit moofkit changed the title Fix headers serialization Fix headers serialization for sentry-ruby Jan 12, 2021
@moofkit
Copy link
Copy Markdown
Contributor Author

moofkit commented Jan 12, 2021

@st0012 thanks for quick response!

I've split this in two PR.
This PR is for raven #1198

@st0012
Copy link
Copy Markdown
Contributor

st0012 commented Jan 12, 2021

@moofkit thanks 👍 I'll give them a look again tomorrow and merge them

@st0012 st0012 added the bug fix label Jan 13, 2021
@st0012 st0012 added this to the 4.1.4 milestone Jan 13, 2021
@st0012
Copy link
Copy Markdown
Contributor

st0012 commented Jan 13, 2021

@moofkit great work, thanks 👍

@st0012 st0012 merged commit 6673aeb into getsentry:master Jan 13, 2021
@smcgivern
Copy link
Copy Markdown

Thanks so much for this @moofkit, we also hit this issue and it was great to see such a clear PR description to help us understand it was the same problem: https://gitlab.com/gitlab-org/gitlab/-/issues/330729

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.

4 participants