Make OTLP exporter optional and allow building without Prometheus exporter (/metrics) #372

jirutka · 2022-09-04T14:12:05Z

jirutka commented

2022-09-04 14:12:05 +00:00

Please see commit messages.

lx approved these changes 2022-09-05 09:06:58 +00:00

Dismissed

lx left a comment

Thanks for making this, it's pretty much what I imagined. There's just a small issue with the warn! when OTLP is disabled (see below). Also if you have the opportunity of running cargo fmt and checking the cargo clippy lints, that would be nice (depending on your Clippy version, it might report a bunch of lints about Eq/PartialEq which we don't care about).

We also need to update the .nix files to integrate this with our CI but I can do it myself.

Thanks for making this, it's pretty much what I imagined. There's just a small issue with the `warn!` when OTLP is disabled (see below). Also if you have the opportunity of running `cargo fmt` and checking the `cargo clippy` lints, that would be nice (depending on your Clippy version, it might report a bunch of lints about `Eq`/`PartialEq` which we don't care about). We also need to update the `.nix` files to integrate this with our CI but I can do it myself.

src/garage/server.rs Outdated

					
				@ -41,1 +45,4 @@

						init_tracing(&export_to, garage.system.id)?;

						#[cfg(not(feature = "telemetry-otlp"))]

						warn!("Garage was built without OTLP exporter, admin.trace_sink is ignored.");

lx commented

2022-09-05 08:58:16 +00:00

Looks like this will never be called because we are in a #[cfg(feature = "telemetry-otlp")] block

Looks like this will never be called because we are in a `#[cfg(feature = "telemetry-otlp")]` block

jirutka marked this conversation as resolved

lx dismissed lx’s review 2022-09-05 09:07:44 +00:00

lx referenced this pull request

2022-09-05 10:21:19 +00:00

Reorganize dependencies #373

lx changed target branch from main to improve-deps

2022-09-05 10:21:56 +00:00

lx commented

2022-09-05 10:22:46 +00:00

Changed to merge this into #373 (a temporary branch where I'll do the Nix shenanigans). This caused a conflict in src/garage/Cargo.toml because of #370.

Changed to merge this into #373 (a temporary branch where I'll do the Nix shenanigans). This caused a conflict in `src/garage/Cargo.toml` because of #370.

jirutka force-pushed telemetry-and-metrics from a095c831fc to 75b11bdfc1

2022-09-05 23:11:51 +00:00

Compare

jirutka force-pushed telemetry-and-metrics from 75b11bdfc1 to ea36b9ff90

2022-09-05 23:16:04 +00:00

Compare

jirutka commented

2022-09-05 23:18:28 +00:00

Okay, I fixed it and rebased against the improve-deps branch.

Okay, I fixed it and rebased against the `improve-deps` branch.

lx commented

2022-09-06 13:11:22 +00:00