Skip to content

Fix Instrumentation not inheriting Feature #506

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fix Instrumentation not inheriting Feature #506

wants to merge 1 commit into from

Conversation

paul
Copy link
Contributor

@paul paul commented Oct 29, 2018

Fixes #505. There was a bug because the Instrumenter was getting initialized twice because the shortcut on https://github.com/httprb/http/blob/master/lib/http/options.rb#L111 wasn't hit without Instrumenter inheriting Feature.

I also fixed the docs a mentioned in the issue.

@ixti @tarcieri WDYT about adding activesupport as a dev dependency, so I can write a real test that exercises it being called correctly?

@paul
Copy link
Contributor Author

paul commented Oct 29, 2018

@joshuaflanagan Can you check if this works for you?

require "pp"
require "active_support/notifications"
ActiveSupport::Notifications.subscribe(/.*/) do |name, start, finish, id, payload|
  pp :name => name, :start => start.to_f, :finish => finish.to_f, :id => id, :payload => payload
end

HTTP.use(instrumentation: {instrumenter: ActiveSupport::Notifications.instrumenter}).get("https://httpbin.org/get")

{:name=>"start_request.http",
 :start=>1540825224.790576,
 :finish=>1540825224.7905908,
 :id=>"a1ecae467ff9e7c58964",
 :payload=>
  {:request=>
    #<HTTP::Request:0x000055e547d9bbb0
     @body=#<HTTP::Request::Body:0x000055e547d98758 @source=nil>,
     @headers=
      #<HTTP::Headers {"Connection"=>"close", "Host"=>"httpbin.org", "User-Agent"=>"http.rb/4.0.0"}>,
     @proxy={},
     @scheme=:https,
     @uri=#<HTTP::URI:0x0055e547d98de8 URI:https://httpbin.org/get>,
     @verb=:get,
     @version="1.1">}}

{:name=>"request.http",
 :start=>1540825224.792291,
 :finish=>1540825225.0600104,
 :id=>"a1ecae467ff9e7c58964",
 :payload=>
  {:response=>
    #<HTTP::Response/1.1 200 OK {"Connection"=>"close", "Server"=>"gunicorn/19.9.0", "Date"=>"Mon, 29 Oct 2018 15:00:25 GMT", "Content-Type"=>"application/json", "Content-Length"=>"195", "Access-Control-Allow-Origin"=>"*", "Access-Control-Allow-Credentials"=>"true", "Via"=>"1.1 vegur"}>}}

# => #<HTTP::Response/1.1 200 OK {"Connection"=>"close", "Server"=>"gunicorn/19.9.0", "Date"=>"Mon, 29 Oct 2018 15:00:25 GMT", "Content-Type"=>"application/json", "Content-Length"=>"195", "Access-Control-Allow-Origin"=>"*", "Access-Control-Allow-Credentials"=>"true", "Via"=>"1.1 vegur"}>

@paul paul mentioned this pull request Oct 29, 2018
Closed
ixti
ixti requested changes Oct 30, 2018
View reviewed changes
ixti
ixti reviewed Oct 30, 2018
View reviewed changes
lib/http/features/instrumentation.rb
instrumenter.instrument("start_#{name}", :request => request)
# Emit a separate "start" event, so a logger can print the request
# being run without waiting for a response
instrumenter.instrument("start_#{name}", :request => request) {}
Copy link
Member

Choose a reason for hiding this comment

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

I dislike this part. I see reason behind this but it feels like trying to use instrumenter for something it's not supposed to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a common pattern in Rails itself: https://github.com/rails/rails/blob/b2eb1d1c55a59fee1e6c4cba7030d8ceb524267c/actionpack/lib/action_controller/metal/instrumentation.rb#L30-L32

I'm not a big fan of it either (I wish the instrument call would just automatically emit events at both the start and end of the block), but I assume this is how it was intended to be used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't it? https://github.com/rails/rails/blob/405aef3dede8938c8eb9789f6d6722864faddb09/activesupport/lib/active_support/notifications/instrumenter.rb#L21-L29
line 21 - start
line 29 - finish

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshuaflanagan I dug through the overly-complicated code for a few minutes, and I can't figure out why, but the subscribers don't receive any calls at the start of the block, just the end.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, you're right. I misinterpreted that code. Line 21 is marking the start of the event, but nothing is published until the finish on line 29.

Confirmed in the docs (https://guides.rubyonrails.org/active_support_instrumentation.html#creating-custom-events):

Simply call instrument with a name, payload and a block. The notification will be sent after the block returns.

@ixti

This comment has been minimized.

Sign in to view
@ixti

This comment has been minimized.

Sign in to view
@ixti
Copy link
Member

ixti commented Oct 30, 2018

We have activemodel dev dependency due to certificate of authority but we were thinking about getting rid of this. I would say that it's OK to add AS dependency for now, but I have a feeling that probably we should extract this feature into a separate gem, probably, e.g. http-rails or such. Have no strong feelings one way or the other thogh. @tarcieri would really like to know your thoughts.

@tarcieri
Copy link
Member

I don't mind activesupport as a dev dependency. I agree extracting the functionality into a separate crate would probably be best.

@ixti
Copy link
Member

ixti commented Jan 15, 2019

Merged into 4-x-stable, will be released as part of v4.0.2

@ixti ixti closed this Jan 15, 2019
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Jun 7, 2020
www/ruby-http: update to 4.4.1
0e84a93 
Update ruby-http to 4.4.1.


## 4.4.1 (2020-03-29)

* Backport [#590](httprb/http#590)
  Fix parser failing on some edge cases.
  ([@ixti])

## 4.4.0 (2020-03-25)

* Backport [#587](httprb/http#587)
  Fix redirections when server responds with multiple Location headers.
  ([@ixti])

* Backport [#599](httprb/http#599)
  Allow passing HTTP::FormData::{Multipart,UrlEncoded} object directly.
  ([@ixti])

## 4.3.0 (2020-01-09)

* Backport [#581](httprb/http#581)
  Add Ruby-2.7 compatibility.
  ([@ixti], [@janko])


## 4.2.0 (2019-10-22)

* Backport [#489](httprb/http#489)
  Fix HTTP parser.
  ([@ixti], [@fxposter])


## 4.1.1 (2019-03-12)

* Add `HTTP::Headers::ACCEPT_ENCODING` constant.
  ([@ixti])


## 4.1.0 (2019-03-11)

* [#533](httprb/http#533)
  Add URI normalizer feature that allows to swap default URI normalizer.
  ([@mamoonraja])


## 4.0.5 (2019-02-15)

* Backport [#532](httprb/http#532) from master.
  Fix pipes support in request bodies.
  ([@ixti])


## 4.0.4 (2019-02-12)

* Backport [#506](httprb/http#506) from master.
  Skip auto-deflate when there is no body.
  ([@Bonias])


## 4.0.3 (2019-01-18)

* Fix missing URL in response wrapped by auto inflate.
  ([@ixti])

* Provide `HTTP::Request#inspect` method for debugging purposes.
  ([@ixti])


## 4.0.2 (2019-01-15)

* [#506](httprb/http#506)
  Fix instrumentation feature.
  ([@paul])


## 4.0.1 (2019-01-14)

* [#515](httprb/http#515)
  Fix `#build_request` and `#request` to respect default options.
  ([@RickCSong])


## 4.0.0 (2018-10-15)

* [#482](httprb/http#482)
  [#499](httprb/http#499)
  Introduce new features injection API with 2 new feaures: instrumentation
  (compatible with ActiveSupport::Notification) and logging.
  ([@paul])

* [#473](httprb/http#473)
  Handle early responses.
  ([@janko-m])

* [#468](httprb/http#468)
  Rewind `HTTP::Request::Body#source` once `#each` is complete.
  ([@ixti])

* [#467](httprb/http#467)
  Drop Ruby 2.2 support.
  ([@ixti])

* [#436](httprb/http#436)
  Raise ConnectionError when writing to socket fails.
  ([@janko-m])

* [#438](httprb/http#438)
  Expose `HTTP::Request::Body#source`.
  ([@janko-m])

* [#446](httprb/http#446)
  Simplify setting a timeout.
  ([@mikegee])

* [#451](httprb/http#451)
  Reduce memory usage when reading response body.
  ([@janko-m])

* [#458](httprb/http#458)
  Extract HTTP::Client#build_request method.
  ([@tycoon])

* [#462](httprb/http#462)
  Fix HTTP::Request#headline to allow two leading slashes in path.
  ([@scarfacedeb])

* [#454](httprb/http#454)
  [#464](httprb/http#464)
  [#384](httprb/http#384)
  Fix #readpartial not respecting max length argument.
  ([@janko-m], [@marshall-lee])
@tarcieri tarcieri mentioned this pull request May 13, 2021
Merged
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Aug 11, 2025
@ryoon
www/ruby-http: Update to 5.3.1
1478a6c 
Changelog:
## [5.3.1] - 2025-06-09

### Changed

- Revert switch to the native llhttp on MRI, as it's not compatible with
  standalone bundles
  ([#802](httprb/http#802))


## [5.3.0] - 2025-06-09

### Added

- (backported) Add .retriable feature to Http
- (backported) Add more specific ConnectionError classes
- (backported) New feature: RaiseError

### Changed

- (backported) Drop depenency on base64
- (backported) Cache header normalization to reduce object allocation
- (backported) Use native llhttp on MRI


## [5.2.0] - 2024-02-05

### Added

- Add `Connection#finished_request?`
  ([#743](httprb/http#743))
- Add `Instrumentation#on_error`
  ([#746](httprb/http#746))
- Add `base64` dependency (suppresses warnings on Ruby 3.0)
  ([#759](httprb/http#759))
- Add `PURGE` HTTP verb
  ([#757](httprb/http#757))
- Add Ruby-3.3 support

### Changed

- **BREAKING** Process features in reverse order
  ([#766](httprb/http#766))
- **BREAKING** Downcase Content-Type charset name
  ([#753](httprb/http#753))
- **BREAKING** Make URI normalization more conservative
  ([#758](httprb/http#758))

### Fixed

- Close sockets on initialize failure
  ([#762](httprb/http#762))
- Prevent CRLF injection due to broken URL normalizer
  ([#765](httprb/http#765))

[unreleased]: httprb/http@v5.3.0...5-x-stable
[5.3.0]: httprb/http@v5.2.0...v5.3.0
[5.2.0]: httprb/http@v5.1.1...v5.2.0

## 5.1.1 (2022-12-17)

* [#731](httprb/http#731)
  Strip brackets from IPv6 addresses in `HTTP::URI`.
  ([@jeraki])

* [#722](httprb/http#722)
  Add `on_redirect` callback.
  ([@benubois])

## 5.1.0 (2022-06-17)

* Drop ruby-2.5 support.

* [#715](httprb/http#715)
  Set default encoding to UTF-8 for `application/json`.
  ([@drwl])

* [#712](httprb/http#712)
  Recognize cookies set by redirect.
  ([@tkellogg])

* [#707](httprb/http#707)
  Distinguish connection timeouts.
  ([@YuLeven])

## 5.0.4 (2021-10-07)

* [#698](httprb/http#698)
  Fix `HTTP::Timeout::Global#connect_ssl`.
  ([@tarcieri])

## 5.0.3 (2021-10-06)

* [#695](httprb/http#695)
  Revert DNS resolving feature.
  ([@PhilCoggins])

* [#694](httprb/http#694)
  Fix cookies extraction.
  ([@flosacca])

## 5.0.2 (2021-09-10)

* [#686](httprb/http#686)
  Correctly reset the parser.
  ([@bryanp])

* [#684](httprb/http#684)
  Don't set Content-Length for GET, HEAD, DELETE, or CONNECT requests without a BODY.
  ([@jyn514])

* [#679](httprb/http#679)
  Use features on redirected requests.
  ([@nomis])

* [#678](httprb/http#678)
  Restore `HTTP::Response` `:uri` option for backwards compatibility.
  ([@schwern])

* [#676](httprb/http#676)
  Update addressable because of CVE-2021-32740.
  ([@matheussilvasantos])

* [#653](httprb/http#653)
  Avoid force encodings on frozen strings.
  ([@bvicenzo])

* [#638](httprb/http#638)
  DNS failover handling.
  ([@midnight-wonderer])


## 5.0.1 (2021-06-26)

* [#670](httprb/http#670)
  Revert `Response#parse` behavior introduced in [#540].
  ([@DannyBen])

* [#669](httprb/http#669)
  Prevent bodies from being resubmitted when following unsafe redirects.
  ([@odinhb])

* [#664](httprb/http#664)
  Bump llhttp-ffi to 0.3.0.
  ([@bryanp])


## 5.0.0 (2021-05-12)

* [#656](httprb/http#656)
  Handle connection timeouts in `Features`
  ([@semenyukdmitry])

* [#651](httprb/http#651)
  Replace `http-parser` with `llhttp`
  ([@bryanp])

* [#647](httprb/http#647)
  Add support for `MKCALENDAR` HTTP verb
  ([@meanphil])

* [#632](httprb/http#632)
  Respect the SSL context's `verify_hostname` value
  ([@colemannugent])

* [#625](httprb/http#625)
  Fix inflator with empty responses
  ([@LukaszMaslej])

* [#599](httprb/http#599)
  Allow passing `HTTP::FormData::{Multipart,UrlEncoded}` object directly.
  ([@ixti])

* [#593](httprb/http#593)
  [#592](httprb/http#592)
  Support informational (1XX) responses.
  ([@ixti])

* [#590](httprb/http#590)
  [#589](httprb/http#589)
  Fix response headers paring.
  ([@Bonias])

* [#587](httprb/http#587)
  [#585](httprb/http#585)
  Fix redirections when server responds with multiple Location headers.
  ([@ixti])

* [#581](httprb/http#581)
  [#582](httprb/http#582)
  Add Ruby 2.7.x support.
  ([@janko])

* [#577](httprb/http#577)
  Fix `Chainable#timeout` with frozen Hash.
  ([@antonvolkoff])

* [#576](httprb/http#576)
  [#524](httprb/http#524)
  **BREAKING CHANGE**
  Preserve header names casing.
  ([@joshuaflanagan])

* [#540](httprb/http#540)
  [#538](httprb/http#538)
  **BREAKING CHANGE**
  Require explicit MIME type for Response#parse
  ([@ixti])

* [#532](httprb/http#532)
  Fix pipes support in request bodies.
  ([@ixti])

* [#530](httprb/http#530)
  Improve header fields name/value validation.
  ([@Bonias])

* [#506](httprb/http#506)
  [#521](httprb/http#521)
  Skip auto-deflate when there is no body.
  ([@Bonias])

* [#489](httprb/http#489)
  Fix HTTP parser.
  ([@ixti], [@fxposter])

* [#546](httprb/http#546)
  **BREAKING CHANGE**
  Provide initiating `HTTP::Request` object on `HTTP::Response`.
  ([@joshuaflanagan])

* [#571](httprb/http#571)
  Drop Ruby 2.3.x support.
  ([@ixti])

* [3ed0c31](httprb/http@3ed0c31)
  Drop Ruby 2.4.x support.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ixti ixti ixti requested changes

+1 more reviewer

@joshuaflanagan joshuaflanagan joshuaflanagan left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Instrumentation feature not working
4 participants
@paul @ixti @tarcieri @joshuaflanagan