Skip to content

Commit

Permalink
remove grpcio
Browse files Browse the repository at this point in the history
We do not need to use two different gRPC implementations for OTLP, and
grpcio greatly increases the compilation times and CI cache size, so
we're choosing to use tonic instead

* make the OTLP exporters non exclusive

We should be able to build a router with both OTLP/HTTP and OTLP/gRPC
exporters

update the opentelemetry dependency to get access to
SpanExporterBuilder that was not public before
  • Loading branch information
Geal committed Nov 23, 2021
1 parent e272e8f commit a1b20c9
Show file tree
Hide file tree
Showing 29 changed files with 200 additions and 426 deletions.
12 changes: 1 addition & 11 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -144,16 +144,6 @@ commands:

windows_prepare_rust_env:
steps:
# - run:
# # TODO compiling grpcio on Windows is still not working
# # using boringssl gives an error message
# # using openssl hangs indefinitely
# name: Install grpcio build dependencies
# command: |
# choco install activeperl -y
# choco install cmake -y --installargs 'ADD_CMAKE_TO_PATH=System'
# choco install yasm -y
# choco install openssl -y
- run:
name: Install rustup
environment:
Expand Down Expand Up @@ -255,7 +245,7 @@ commands:
- build_common_permutations
- rust/build:
with_cache: false
crate: --no-default-features --features otlp-grpcio -p apollo-router -p apollo-router-core
crate: --no-default-features -p apollo-router -p apollo-router-core
build_workspace:
parameters:
os:
Expand Down
135 changes: 4 additions & 131 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ default-members = ["crates/apollo-router", "crates/apollo-router-core"]
members = ["crates/apollo-router", "crates/apollo-router-core", "xtask"]

[patch.crates-io]
opentelemetry = { git = "https://github.com/open-telemetry/opentelemetry-rust.git", rev = "414b3f459" }
opentelemetry-otlp = { git = "https://github.com/open-telemetry/opentelemetry-rust.git", rev = "414b3f459" }
opentelemetry = { git = "https://github.com/open-telemetry/opentelemetry-rust.git", rev = "6b3aa02aa" }
opentelemetry-otlp = { git = "https://github.com/open-telemetry/opentelemetry-rust.git", rev = "6b3aa02aa" }
5 changes: 2 additions & 3 deletions crates/apollo-router/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,13 @@ name = "router"
path = "src/main.rs"

[features]
default = ["otlp-tonic"]
otlp-tonic = [
default = ["otlp-grpc"]
otlp-grpc = [
"opentelemetry-otlp/tonic",
"opentelemetry-otlp/tonic-build",
"opentelemetry-otlp/prost",
"tonic",
]
otlp-grpcio = ["opentelemetry-otlp/grpc-sys", "opentelemetry-otlp/openssl"]
otlp-http = ["opentelemetry-otlp/http-proto"]
tls = ["opentelemetry-otlp/tls", "tonic/transport", "tonic/tls"]

Expand Down
38 changes: 17 additions & 21 deletions crates/apollo-router/src/configuration/mod.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,6 @@
//! Logic for loading configuration in to an object model

#[cfg(any(
all(
feature = "otlp-tonic",
any(feature = "otlp-grpcio", feature = "otlp-http")
),
all(feature = "otlp-grpcio", feature = "otlp-http")
))]
compile_error!("you can select only one feature otlp-*!");

#[cfg(any(feature = "otlp-tonic", feature = "otlp-grpcio", feature = "otlp-http"))]
#[cfg(any(feature = "otlp-tonic", feature = "otlp-http"))]
pub mod otlp;

use apollo_router_core::prelude::*;
Expand All @@ -32,6 +23,10 @@ pub enum ConfigurationError {
CannotReadSecretFromFile(std::io::Error),
/// Could not read secret from environment variable: {0}
CannotReadSecretFromEnv(std::env::VarError),
/// Missing environment variable: {0}
MissingEnvironmentVariable(String),
/// Invalid environment variable: {0}
InvalidEnvironmentVariable(String),
/// Could not setup OTLP tracing: {0}
OtlpTracing(opentelemetry::trace::TraceError),
/// The configuration could not be loaded because it requires the feature {0:?}
Expand Down Expand Up @@ -204,7 +199,7 @@ impl Cors {
#[allow(clippy::large_enum_variant)]
pub enum OpenTelemetry {
Jaeger(Option<Jaeger>),
#[cfg(any(feature = "otlp-tonic", feature = "otlp-grpcio", feature = "otlp-http"))]
#[cfg(any(feature = "otlp-tonic", feature = "otlp-http"))]
Otlp(otlp::Otlp),
}

Expand Down Expand Up @@ -317,13 +312,21 @@ mod tests {
assert_config_snapshot!("testdata/config_opentelemetry_jaeger_full.yml");
}

#[cfg(any(feature = "otlp-tonic", feature = "otlp-grpcio", feature = "otlp-http"))]
#[cfg(any(feature = "otlp-tonic", feature = "otlp-http"))]
#[test]
fn ensure_configuration_api_does_not_change_common() {
// NOTE: don't take a snapshot here because the optional fields appear with ~ and they vary
// per implementation

#[cfg(feature = "otlp-http")]
serde_yaml::from_str::<Configuration>(include_str!(
"testdata/config_opentelemetry_otlp_tracing_http_common.yml"
))
.unwrap();

#[cfg(feature = "otlp-tonic")]
serde_yaml::from_str::<Configuration>(include_str!(
"testdata/config_opentelemetry_otlp_tracing_common.yml"
"testdata/config_opentelemetry_otlp_tracing_tonic_common.yml"
))
.unwrap();
}
Expand All @@ -335,13 +338,6 @@ mod tests {
assert_config_snapshot!("testdata/config_opentelemetry_otlp_tracing_tonic_full.yml");
}

#[cfg(feature = "otlp-grpcio")]
#[test]
fn ensure_configuration_api_does_not_change_grpcio() {
assert_config_snapshot!("testdata/config_opentelemetry_otlp_tracing_grpcio_basic.yml");
assert_config_snapshot!("testdata/config_opentelemetry_otlp_tracing_grpcio_full.yml");
}

#[cfg(feature = "otlp-http")]
#[test]
fn ensure_configuration_api_does_not_change_http() {
Expand All @@ -352,7 +348,7 @@ mod tests {
#[cfg(all(feature = "tls", feature = "otlp-tonic"))]
#[test]
fn ensure_configuration_api_does_not_change_tls_config() {
assert_config_snapshot!("testdata/config_opentelemetry_otlp_tls.yml");
assert_config_snapshot!("testdata/config_opentelemetry_otlp_tracing_tonic_tls.yml");
}

#[test]
Expand Down
Loading

0 comments on commit a1b20c9

Please sign in to comment.