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

added httpx opentelemetry adapter #681

Merged
merged 4 commits into from
Nov 5, 2023

Conversation

HoneyryderChuck
Copy link
Contributor

httpx is a ruby http client.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 3, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Collaborator

@arielvalentin arielvalentin left a comment

Choose a reason for hiding this comment

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

Thank you again for your contribution! We really appreciate your desire to add instrumentation to HTTPX.

This is the first time I am learning about the gem and I think it has a lot of essential features for modern Ruby applications. I most definitely would like to give it a try!
 
Since you are the inventor and maintainer of HTTPX, would you have any interest in being one of the first gem authors to provide First Party support and make OTel one of the official plugins available for your gem?

We could work with you to get the gem listed in the official registry :)

@HoneyryderChuck
Copy link
Contributor Author

Thx!

would you have any interest in being one of the first gem authors to provide First Party support and make OTel one of the official plugins available for your gem?

What does that entail? FWIW httpx already ships with adapters to other tools, including one for datadog. It seems that in OTel's case every integration ships as its own gem, so not sure if worth deviating frrom that path?

@arielvalentin
Copy link
Collaborator

Thx!

would you have any interest in being one of the first gem authors to provide First Party support and make OTel one of the official plugins available for your gem?

What does that entail? FWIW httpx already ships with adapters to other tools, including one for datadog. It seems that in OTel's case every integration ships as its own gem, so not sure if worth deviating frrom that path?

Longer term I think our goal is to have First Party Integration with other gems. We have hesitated doing this in the past because the instrumentation-wg is still immature and semantic conventions is still experimental in quite a few cases.

I am keen on giving first party support soon so that we are able to encourage gem authors to provide first-class support for OTel. We have limited bandwidth and lack a critical mass of experts to have a centralized repo for instrumentations.

This will require maintaining a close partnership with you and other httpx maintainers to ensure we are not inadvertently introducing unreasonable changes.

As for the mechanics of making this happen, here is what I do to create my own instrumentations for our internal libraries at GitHub:

  1. Move the instrumentation into the httpx repository. Your instrumentation should inherit from OpenTelemetry::Instrumentation::Base, which should already be done
  2. Namespace the gem in your repository under the httpx namespace (as opposed to using OpenTelemetry::Instrumentation)
  3. Provide instructions to include the gem in the application's Gemfile.
  4. Bundler should take it from there!

The Base gem should register your instrumentation in the registry so there is no need to manually active it if the end user is relying on use_all.

All that being said

I am by no means requiring you to do any of this!

AFAIK, this is the first time a gem author/maintainer is contributing an instrumentation to this repository so I was hoping you may be willing to give a try to First Party instrumentation.

We welcome this contribution and will add you as a CODEOWNER so that we are able to contact you for on-going maintenance in this repo.

@HoneyryderChuck
Copy link
Contributor Author

I've no problem with providing support for this adapter. I'm not sure what you mean as "first party" though. Does it mean moving the adapter into its own repo? I'm not sure if that would be beneficial at the stage OTel ruby is. But feel free to ping me in any issue and question related to it.

@HoneyryderChuck HoneyryderChuck force-pushed the httpx branch 2 times, most recently from 9f5f3e4 to 7490289 Compare October 17, 2023 14:52
@arielvalentin
Copy link
Collaborator

I'm not sure what you mean as "first party" though. Does it mean moving the adapter into its own repo?

Ideally OTel support would be a "first party" instrumentation, meaning maintained by gem authors; and not "third party" meaning maintained in this repository by us.

In this case it would mean residing with the other adapters and included in the httpx like the others are: https://gitlab.com/os85/httpx/-/tree/master/lib/httpx/adapters?ref_type=heads

Then in that ideal world, we would change the instrumentation portion of this repo to provide utilities and tools for gem maintainers.

The challenge we face with third party maintenance is that we have limited capacity and expertise and when upstream changes happen with gems being instrumented in this repo. We have had a few contributions and instrumentations that do not have active maintainers and at some point in the future we will not be able to keep up and have to sunset them.

Hope that clears up what I mean.

@arielvalentin
Copy link
Collaborator

@HoneyryderChuck There are a few more steps to ensure tests are passing on multiple versions of Ruby:

Please add the instrumentation to the CI builds:

Please add the gem to the release list:

Once all of the tests are passing I will merge.

@HoneyryderChuck
Copy link
Contributor Author

@arielvalentin updated

@arielvalentin
Copy link
Collaborator

@HoneyryderChuck
Copy link
Contributor Author

fixed.

instrumentation/httpx/CHANGELOG.md Outdated Show resolved Hide resolved
@HoneyryderChuck
Copy link
Contributor Author

@arielvalentin any updates on this one? do you think it's ready?

simi and others added 2 commits November 4, 2023 19:58
@arielvalentin arielvalentin merged commit 9fcf6d8 into open-telemetry:main Nov 5, 2023
46 checks passed
@HoneyryderChuck HoneyryderChuck deleted the httpx branch November 5, 2023 20:53
@HoneyryderChuck HoneyryderChuck restored the httpx branch November 5, 2023 20:53
@HoneyryderChuck HoneyryderChuck deleted the httpx branch November 5, 2023 20:53
@HoneyryderChuck HoneyryderChuck restored the httpx branch November 5, 2023 20:53
jdehaan pushed a commit to tools-aoeur/opentelemetry-ruby-contrib that referenced this pull request Nov 21, 2023
* added httpx opentelemetry adapter

* Update instrumentation/httpx/CHANGELOG.md

Co-authored-by: Ariel Valentin <[email protected]>

* Update instrumentation/httpx/README.md

Co-authored-by: Ariel Valentin <[email protected]>

* Update instrumentation/httpx/README.md

Co-authored-by: Ariel Valentin <[email protected]>

---------

Co-authored-by: Ariel Valentin <[email protected]>
Co-authored-by: Josef Šimánek <[email protected]>
jdehaan added a commit to tools-aoeur/opentelemetry-ruby-contrib that referenced this pull request Nov 21, 2023
* added httpx opentelemetry adapter (open-telemetry#681)

* added httpx opentelemetry adapter

* Update instrumentation/httpx/CHANGELOG.md

Co-authored-by: Ariel Valentin <[email protected]>

* Update instrumentation/httpx/README.md

Co-authored-by: Ariel Valentin <[email protected]>

* Update instrumentation/httpx/README.md

Co-authored-by: Ariel Valentin <[email protected]>

---------

Co-authored-by: Ariel Valentin <[email protected]>
Co-authored-by: Josef Šimánek <[email protected]>

* chore: Change release restrictions

The toys gem checks that all actions are passing before allowing
a release request to be opened, however only the CI builds are actually required.

This change limits the release request requirements to look specifically for CI builds

* chore: Fix httpx version

* chore: bump toys

* release: Release opentelemetry-instrumentation-httpx 0.1.0 (initial release) (open-telemetry#713)

* release: Release opentelemetry-instrumentation-httpx 0.1.0 (initial release)

* Update instrumentation/httpx/CHANGELOG.md

* feat!: Drop Rails 6.0 EOL (open-telemetry#680)

* feat!: Drop Rails 6.0 EOL

6.0 is no longer receiving maintenance, security, or feature updates as of 01 Jun 2023

Users who want to continue instrumentating Rails applications should pin to earlier versions of the instrumentation.

* squash: Fix test

* squash: gem version object instead of string

* Update README.md

Co-authored-by: Kayla Reopelle (she/her) <[email protected]>

* Update instrumentation/README.md

Co-authored-by: Kayla Reopelle (she/her) <[email protected]>

---------

Co-authored-by: Kayla Reopelle (she/her) <[email protected]>

* Inline gemspec dev constraints with Appraisals. (open-telemetry#716)

* Removal Sinatra 1.x Appraisal (open-telemetry#715)

fix: Removal Sinatra 1.x Appraisal

Rack 1.x is not directly supported anymore. Sinatra 1.x in turn
"is not". Removal appraisal and add a compatibility note to both
Sinatra and Rack for proper instrumentation version usage.

Co-authored-by: Ariel Valentin <[email protected]>

---------

Co-authored-by: Tiago <[email protected]>
Co-authored-by: Ariel Valentin <[email protected]>
Co-authored-by: Josef Šimánek <[email protected]>
Co-authored-by: Ariel Valentin <[email protected]>
Co-authored-by: Kayla Reopelle (she/her) <[email protected]>
Co-authored-by: Zachery Hostens <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed instrumentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants