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

Fix Instrumentation not inheriting Feature #506

Closed

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"}>

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.

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.

@ixti

This comment has been minimized.

@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
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
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.

Instrumentation feature not working
4 participants