Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Dec 24, 2021

Conversation

ojab
Copy link
Contributor

@ojab ojab commented Dec 8, 2021

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
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
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
Collaborator

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

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
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
Contributor Author

ojab commented Dec 10, 2021

Updated, CI is green.

@ojab
Copy link
Contributor Author

ojab commented Dec 10, 2021

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
Collaborator

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
Collaborator

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
Copy link
Contributor Author

ojab commented Dec 13, 2021

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

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
Copy link
Contributor Author

ojab commented Dec 22, 2021

Hey, any changes to be done here?

@st0012
Copy link
Collaborator

st0012 commented Dec 22, 2021

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

Copy link
Collaborator

@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 modified the milestones: 4.8.2, 4.9.0 Dec 25, 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