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

Start connection in Net::HTTP adapter for mocked requests #960

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ojab
Copy link
Contributor

@ojab ojab commented Dec 10, 2021

Instrumentation like Sentry only instruments requests when
Net::HTTP.started? because #request is called twice if connection
is not started yet. This change allows this instrumentation to work
with mocked requests.

See https://github.com/ruby/net-http/blob/master/lib/net/http.rb#L1529-L1534

Instrumentation like Sentry only instruments requests when
`Net::HTTP.started?` because `#request` is called twice if connection
is not started yet. This change allows this instrumentation to work
with mocked requests.

See https://github.com/ruby/net-http/blob/master/lib/net/http.rb#L1529-L1534
@ojab
Copy link
Contributor Author

ojab commented Dec 10, 2021

Ready to review because CI waits for approval, rspec spec/acceptance/net_http/ is green locally.

@ojab ojab marked this pull request as ready for review December 10, 2021 16:37
@bblimke
Copy link
Owner

bblimke commented Aug 2, 2022

@ojab I'm sorry for the delay.

Is that related specifically to Sentry which checks for Net::HTTP.started? . My concern is that in all other cases it will unnecessarily call request twice without any specific need.

@ojab
Copy link
Contributor Author

ojab commented Aug 2, 2022

Sentry prompted me to look into that and I didn't saw any issues about it in other places, but overall it should make mocking more correct and shouldn't bring much overhead.

@rzane
Copy link
Contributor

rzane commented Aug 8, 2022

#992 also fixes this, if I'm not mistaken.

@bblimke
Copy link
Owner

bblimke commented Aug 19, 2023

@ojab is this PR still required since #992 has already been released?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants