honeycombio / libhoney-rb

Ruby library for sending data to Honeycomb
Apache License 2.0
11 stars 30 forks source link

Namespace conflict with http.rb gem #75

Closed bernerdschaefer closed 3 years ago

bernerdschaefer commented 3 years ago

We are attempting to set up honeycomb on one of our projects, but have run into an issue with the http gem. libhoney depends on http, which brings in http-cookie, which brings in domain_name, which introduces a DomainName class into the global namespace.

Unfortunately, this DomainName class conflicts with our own DomainName class (a database model) in the codebase, which is hard blocker for experimenting with honeycomb.

I see that this project has a bit of a history with HTTP libraries, having switched back and forth between http and faraday to end up with http again.

Would you be open to considering another HTTP library with fewer dependencies?

For discussion, here's a first pass at what the client would look like using excon: https://github.com/honeycombio/libhoney-rb/compare/main...bernerdschaefer:bs-excon. Excon has no runtime dependencies and supports similar features (persistent connections, proxies).

robbkidd commented 3 years ago

Thank you for raising this issue, @bernerdschaefer! Indeed there was an attempt at replacing http with faraday a while back. After consulting some of the other maintainers, I'll be experimenting with beefing up the Transmission tests and trying out the excon direct option and the faraday "be flexible with which HTTP library might already be present" option. Your first-pass at that is super helpful, thank you.

bernerdschaefer commented 3 years ago

@robbkidd Sounds great, thanks!

robbkidd commented 3 years ago

After a fair amount of experimentation, I have a plan to propose.

As you mention in a comment in your first pass, http.rb has a proxy configuration interface that is trickier in comparison to other HTTP client libraries who pull proxy connection info from environment variables. Changing out the underlying HTTP client library forces the issue of how to accept proxy configuration.

Proposal:

  1. In a new libhoney v1.x.y release, add a deprecation warning about no longer accepting a proxy_config array.
  2. Bump libhoney's major version to 2 with the following backwards breaking changes:
    • Forward web proxy configuration will now be via no_proxy, http_proxy, and https_proxy environment variables with all the variety of ways those can be set for a system or process.
    • Underlying HTTP client library changed with probable new behavior characteristics. Current candidates are excon or faraday.

A new release of the Ruby Beeline would follow to update its upper-bound version constraint on libhoney from ~> 1.14 to ~> 2.0. If faraday is chosen for the libhoney HTTP library, the beeline release will include an update to the faraday integration to ignore the libhoney's own generated HTTP requests to prevent a multiplication of sent events from beeline'd applications.

bernerdschaefer commented 3 years ago

@robbkidd Sounds great, thanks for your thorough research into this!

robbkidd commented 3 years ago

HTTP client library dependency comparisons

The Incumbent: http.rb

For the visually inclined, here is libhoney's current production gem dependency tree. http.rb brings with it 9 other library dependencies. Looking at this, maybe your first thought is "well, yea, it makes sense that an HTTP library would need things like that."

A dependency graph of libhoney's direct and transitive Ruby library dependencies when using the http.rb library. The problem described in this issue is shown here as a chain that leads from libhoney to http to http-cookie to domain_name.

The Lightweight Challenger: excon

Here's the dependency tree when using excon. Yea, that's smaller.

A dependency graph of libhoney's direct and transitive Ruby library dependencies when using the excon library. The graph shows that excon does not add any other gems to libhoney's transitive dependencies.

The Keep-Your-Options-Open Option: Faraday

Faraday is a super popular HTTP client library because it is an HTTP client library abstraction interface with adapters for many of the other Ruby HTTP client libraries. (I've said "HTTP client library" too many times. The phrase has lost all it's meaning now.) By default, Faraday will use the Ruby standard library's Net::HTTP. Sadly, stdlib doesn't support persistent connections, so this option also brings in net-http-persistent as the apparent Faraday-recommended solution.

An interesting wrinkle in this option is that the current Ruby beeline auto-instruments requests made by Faraday. The tests for the Faraday-flavored libhoney reveal that this would roughly triple the number of HTTP-related events sent as the beeline would enqueue events about sending batches of events. 😊 Thankfully, they aren't 1-to-1, so an infinite number of events are not generated. This option requires that the beeline's Faraday integration learn how to ignore libhoney requests when the beeline moves its dependency on libhoney to a Faraday'd version.

A dependency graph of libhoney's direct and transitive Ruby library dependencies when using the faraday and net-http-persistent libraries. The graph shows that together the libraries add three other transitive dependencies: multipart-post, ruby2_keywords, and connection_pool.

The Why-Not-Both: Faraday + excon

Faraday has an adapter for excon! I'll note, though, that I have not yet figured out how to configure persistent connections for excon through Faraday.

A dependency graph of libhoney's direct and transitive Ruby library dependencies when using the faraday and net-http-persistent libraries. The graph shows that together the libraries add two other transitive dependencies: multipart-post and ruby2_keywords.

robbkidd commented 3 years ago

Hello, everybody! After the brain vacation of the holidays, I have a new plan. The previous proposal to make this change backwards incompatible generated a gravity well that starting pulling in other backwards incompatible work that has not yet been done or even planned. In an effort to get this fix out and be gentler on consumers of this library, here is my new proposal.

Minor Version Bump