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

Don't impl HttpClient for reqwest::blocking::Client on wasm32 #2073

Merged
merged 3 commits into from
Sep 13, 2024

Conversation

OliverEvans96
Copy link
Contributor

Fixes #1472

Changes

reqwest::blocking is not implemented on wasm32. Currently, the opentelemetry-http crate contains impl HttpClient for reqwest::blocking::Client, which prevents it from compiling on WASM. This PR conditionally removes that impl.

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)

@OliverEvans96 OliverEvans96 requested a review from a team September 2, 2024 15:11
Copy link

linux-foundation-easycla bot commented Sep 2, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: OliverEvans96 / name: Oliver Evans (5efe944)
  • ✅ login: lalitb / name: Lalit Kumar Bhasin (70bd531, b049138)

@TommyCpp
Copy link
Contributor

TommyCpp commented Sep 3, 2024

reqwest::blocking is not implemented on wasm32

Could you link the source or any doc for this?

@lalitb
Copy link
Member

lalitb commented Sep 3, 2024

Weird that the test are failing for this PR, and not others:

running 1 test
test build_tonic ... FAILED

failures:

---- build_tonic stdout ----
cargo::rerun-if-changed=src/proto/opentelemetry-proto/opentelemetry/proto/common/v1/common.proto
cargo::rerun-if-changed=src/proto/opentelemetry-proto/opentelemetry/proto/resource/v1/resource.proto
error: test failed, to rerun pass `-p opentelemetry-proto --test grpc_build`
cargo::rerun-if-changed=src/proto/opentelemetry-proto/opentelemetry/proto/trace/v1/trace.proto
cargo::rerun-if-changed=src/proto/opentelemetry-proto/opentelemetry/proto/collector/trace/v1/trace_service.proto
cargo::rerun-if-changed=src/proto/opentelemetry-proto/opentelemetry/proto/metrics/v1/metrics.proto
cargo::rerun-if-changed=src/proto/opentelemetry-proto/opentelemetry/proto/collector/metrics/v1/metrics_service.proto
cargo::rerun-if-changed=src/proto/opentelemetry-proto/opentelemetry/proto/logs/v1/logs.proto
cargo::rerun-if-changed=src/proto/opentelemetry-proto/opentelemetry/proto/collector/logs/v1/logs_service.proto
cargo::rerun-if-changed=src/proto/tracez.proto
cargo::rerun-if-changed=src/proto/opentelemetry-proto
cargo::rerun-if-changed=src/proto
thread 'build_tonic' panicked at opentelemetry-proto/tests/grpc_build.rs:161:9:
generated file has changed but it's a CI environment, please rerun this test locally and commit the changes
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@OliverEvans96
Copy link
Contributor Author

reqwest::blocking is not implemented on wasm32

Could you link the source or any doc for this?

https://github.com/seanmonstar/reqwest/blob/cc3dd510c0cb50ee03be217ea864d84e744658d1/src/lib.rs#L370-L377

Note that the wasm directory has no blocking.rs

Copy link

codecov bot commented Sep 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.3%. Comparing base (ff9d50b) to head (70bd531).
Report is 1 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #2073   +/-   ##
=====================================
  Coverage   78.3%   78.3%           
=====================================
  Files        121     121           
  Lines      20815   20815           
=====================================
  Hits       16309   16309           
  Misses      4506    4506           

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

@lalitb lalitb merged commit 7ab5e0f into open-telemetry:main Sep 13, 2024
25 checks passed
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.

[Bug]: The reqwest feature also tries to configure reqwest::blocking::Client and it might not be available
3 participants