Skip to content

Add query string & body to Net::HTTP breadcrumb#1642

Merged
st0012 merged 3 commits intogetsentry:masterfrom
ojab:add_query_string_and_body_to_net_http_breadcrumbs
Dec 26, 2021
Merged

Add query string & body to Net::HTTP breadcrumb#1642
st0012 merged 3 commits intogetsentry:masterfrom
ojab:add_query_string_and_body_to_net_http_breadcrumbs

Conversation

@ojab
Copy link
Copy Markdown
Contributor

@ojab ojab commented Dec 13, 2021

Description

They are useful, let's add them.
This is on top of #1637 because they're touching the same code and probably #1637 would be merged faster.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Dec 13, 2021

Codecov Report

Merging #1642 (c12195a) into master (ddda8fe) will decrease coverage by 0.02%.
The diff coverage is 98.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1642      +/-   ##
==========================================
- Coverage   98.50%   98.47%   -0.03%     
==========================================
  Files         135      135              
  Lines        7606     7663      +57     
==========================================
+ Hits         7492     7546      +54     
- Misses        114      117       +3     
Impacted Files Coverage Δ
sentry-ruby/lib/sentry/net/http.rb 98.00% <75.00%> (-2.00%) ⬇️
sentry-ruby/spec/sentry/net/http_spec.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/background_worker.rb 94.59% <0.00%> (-5.41%) ⬇️

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 ddda8fe...c12195a. Read the comment docs.

@ojab ojab marked this pull request as ready for review December 13, 2021 16:06
@ojab ojab force-pushed the add_query_string_and_body_to_net_http_breadcrumbs branch from 548e8bd to 0f29767 Compare December 13, 2021 16:06
@st0012
Copy link
Copy Markdown
Contributor

st0012 commented Dec 24, 2021

@ojab hey sorry for the trouble, but do you think you can rebase the PR? thx!

@ojab ojab force-pushed the add_query_string_and_body_to_net_http_breadcrumbs branch from 0f29767 to c6a000f Compare December 24, 2021 15:30
@ojab ojab force-pushed the add_query_string_and_body_to_net_http_breadcrumbs branch from c6a000f to 9eb3590 Compare December 24, 2021 15:33
@ojab ojab force-pushed the add_query_string_and_body_to_net_http_breadcrumbs branch from 9eb3590 to f8ed9f6 Compare December 24, 2021 15:34
@ojab
Copy link
Copy Markdown
Contributor Author

ojab commented Dec 24, 2021

Sure thing, rebased, CI is green.

@st0012 st0012 added this to the 4.9.0 milestone Dec 25, 2021
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.

Some small comments but nothing blocking. Thanks for the awesome feature 👍


{ method: req.method, url: url }
rescue
{ method: req.method, url: url.to_s }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should be uri.to_s. I'll change it after merging.

return if from_sentry_sdk?

request_info = extract_request_info(req)
request_info[:body] = req.body if Sentry.configuration.send_default_pii
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this extraction should happen inside extract_request_info.

@st0012 st0012 merged commit 1dc4bd8 into getsentry:master Dec 26, 2021
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.

3 participants