Skip to content

Fixup Net::HTTP breadcrumb url & span creation when Net::HTTP.start is used#1637

Merged
st0012 merged 2 commits intogetsentry:masterfrom
ojab:fixup_net_http_breadcrumbs
Dec 24, 2021
Merged

Fixup Net::HTTP breadcrumb url & span creation when Net::HTTP.start is used#1637
st0012 merged 2 commits intogetsentry:masterfrom
ojab:fixup_net_http_breadcrumbs

Conversation

@ojab
Copy link
Copy Markdown
Contributor

@ojab ojab commented Dec 8, 2021 •
edited
Loading

Please keep these instructions in mind so we can review it more efficiently:

Right now we're not recording URL and creating single span for all the requests.

Description

Not sure if body addition is a welcome change and if 100 is a good length, but it would be useful for us.

@ojab
Copy link
Copy Markdown
Contributor Author

ojab commented Dec 8, 2021

CI failure is unrelated and I will update changelog with PR number anyway, so sending to review.

@ojab ojab marked this pull request as ready for review December 8, 2021 15:36
@ojab
Copy link
Copy Markdown
Contributor Author

ojab commented Dec 9, 2021

OTOH breadcrumbs are already limiting max message size since https://github.com/getsentry/sentry-ruby/pull/1465/files, so limiting body size could be dropped

@st0012
Copy link
Copy Markdown
Contributor

st0012 commented Dec 9, 2021

@ojab thanks for the PR. I think they're both good additions but we can't add those information directly as it may contain pii data. we should only add them when Sentry.configuration.send_default_pii is true, like

sentry-ruby/sentry-ruby/lib/sentry/interfaces/request.rb

Lines 40 to 44 in efcf170

if Sentry.configuration.send_default_pii
self.data = read_data_from(request)
self.cookies = request.cookies
self.query_string = request.query_string
end

@ojab
Copy link
Copy Markdown
Contributor Author

ojab commented Dec 9, 2021

Yeah, I saw that and guessed that only incoming stuff is counted as PII.
I'll make a change to save query & body only with send_default_pii probably tomorrow then.

@ojab ojab force-pushed the fixup_net_http_breadcrumbs branch 2 times, most recently from 2647b75 to 93910fc Compare December 10, 2021 09:34
@ojab
Copy link
Copy Markdown
Contributor Author

ojab commented Dec 10, 2021

Updated, CI is green.

@ojab
Copy link
Copy Markdown
Contributor Author

ojab commented Dec 10, 2021 •
edited
Loading

Also pushed commit to fix tracing of multiple requests in a single connection.

@ojab ojab force-pushed the fixup_net_http_breadcrumbs branch 2 times, most recently from 6cff752 to 33de2cf Compare December 10, 2021 20:49
@st0012
Copy link
Copy Markdown
Contributor

st0012 commented Dec 12, 2021

@ojab it looks like you've made a few other changes to address different issues? since you're essentially rewriting the Net::HTTP patch, can you separate the changes for different issues/features? it'll help me review this more carefully, as it's an important feature of the SDK. thanks.

@st0012
Copy link
Copy Markdown
Contributor

st0012 commented Dec 12, 2021

also, the feature and fixes will likely go out separately (feature in 4.9.0 while bug fixes in 4.8.x, depending on the impact).

@ojab ojab force-pushed the fixup_net_http_breadcrumbs branch 2 times, most recently from 7d29d36 to 6a03061 Compare December 13, 2021 15:51
@ojab ojab changed the title Fixup Net::HTTP breadcrumb url and add query string & body there Fixup Net::HTTP breadcrumb url & span creation when Net::HTTP.start is used Dec 13, 2021
@ojab ojab force-pushed the fixup_net_http_breadcrumbs branch from 6a03061 to dacc310 Compare December 13, 2021 15:56
@ojab
Copy link
Copy Markdown
Contributor Author

ojab commented Dec 13, 2021 •
edited
Loading

Left only bugfixes in this PR and moved query string & body addition to the new PR #1642. Every change is split per-commit, so hope two PRs are ok.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Dec 13, 2021

Codecov Report

Merging #1637 (dacc310) into master (ce10521) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1637   +/-   ##
=======================================
  Coverage   98.45%   98.45%           
=======================================
  Files         135      135           
  Lines        7505     7511    +6     
=======================================
+ Hits         7389     7395    +6     
  Misses        116      116
Impacted Files Coverage Δ
sentry-ruby/lib/sentry/net/http.rb 100.00% <100.00%> (ø)
sentry-ruby/spec/sentry/net/http_spec.rb 100.00% <100.00%> (ø)

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 ce10521...dacc310. Read the comment docs.

@ojab ojab mentioned this pull request Dec 13, 2021
Merged
@ojab
Copy link
Copy Markdown
Contributor Author

ojab commented Dec 22, 2021

Hey, any changes to be done here?

@st0012
Copy link
Copy Markdown
Contributor

st0012 commented Dec 22, 2021

@ojab hey sorry I'm quite busy recently so haven't got time to check all the PRs yet.

st0012
st0012 approved these changes Dec 24, 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!

@st0012 st0012 added the bug fix label Dec 24, 2021
@st0012 st0012 added this to the 4.8.2 milestone Dec 24, 2021
@st0012 st0012 merged commit e9d291d into getsentry:master Dec 24, 2021
@st0012 st0012 removed this from the 4.8.2 milestone Dec 25, 2021
@st0012 st0012 added this to the 4.9.0 milestone Dec 25, 2021
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

Projects

None yet

Milestone

4.9.0

Development

Successfully merging this pull request may close these issues.

3 participants

@ojab @st0012 @codecov-commenter