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

Remove support for surf HTTP client #1537

Merged
merged 2 commits into from
Feb 16, 2024
Merged

Conversation

djc
Copy link
Contributor

@djc djc commented Feb 16, 2024

Changes

In a similar vein as #1534 / #1524, surf has seen its last release over 2 years ago and by the admission of its maintainer is pretty much unmaintained. On crates.io, it got 166k downloads in the last three months, vs reqwest's 11.7M.

#1534

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@djc djc requested a review from a team as a code owner February 16, 2024 08:40
Copy link

codecov bot commented Feb 16, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (d96d59f) 65.5% compared to head (2ac28da) 65.8%.

Files Patch % Lines
opentelemetry-otlp/src/exporter/http/mod.rs 0.0% 1 Missing ⚠️
opentelemetry-zipkin/src/exporter/mod.rs 0.0% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1537     +/-   ##
=======================================
+ Coverage   65.5%   65.8%   +0.2%     
=======================================
  Files        140     140             
  Lines      19569   19488     -81     
=======================================
  Hits       12824   12824             
+ Misses      6745    6664     -81     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@hdost hdost left a comment

Choose a reason for hiding this comment

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

Sounds good! Small ask on the Changelog, call it out as breaking.

opentelemetry-http/CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@shaun-cox shaun-cox left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup!

@cijothomas
Copy link
Member

Also towards #1427 (comment)

@cijothomas
Copy link
Member

Good to merge once merge conflicts resolved, and the changelog calls out breaking change! Thanks @djc

@djc djc force-pushed the no-surf branch 2 times, most recently from e752d7d to c519c11 Compare February 16, 2024 15:58
@djc
Copy link
Contributor Author

djc commented Feb 16, 2024

Rebased. How do you all feel about pushing out a release once this is merged? IMO it would be quite helpful to get the bumped tonic/prost dependencies out there given that they were severely lagging at this point.

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

Thanks for all these cleanups

@cijothomas cijothomas merged commit 93f206a into open-telemetry:main Feb 16, 2024
13 of 14 checks passed
@cijothomas
Copy link
Member

Rebased. How do you all feel about pushing out a release once this is merged? IMO it would be quite helpful to get the bumped tonic/prost dependencies out there given that they were severely lagging at this point.

Agree.
@jtescher @TommyCpp Can you help do a release with current state?

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.

None yet

5 participants