feat(telemetry): add opentelemetry support #346
Conversation
TheButlah
changed the title
| service_name: String, | ||
| service_version: String, | ||
| environment: String, |
There was a problem hiding this comment.
these three fields seem like they should be a part of the OpenTelemetryConfig struct instead of the main telemetry struct. They won't be used by anything other than opentelemetry.
|
|
||
| /// Initialize the OpenTelemetry TracerProvider and set it globally. | ||
| fn init_opentelemetry(&self) | ||
| -> Result<(trace::TracerProvider, trace::Tracer), Box<dyn std::error::Error>> |
There was a problem hiding this comment.
FYI: always use eyre::Report instead of Box<dyn std::error::Error>, except in extremely niche scenarios (can't think of any off the top of my head).
But in reality, instead of using eyre::Report or Box<dyn std::error::Error>, lets use the thiserror crate since enum based errors are more suitable for library code than trait object errors.
| KeyValue::new("deployment.environment", self.environment.clone()), | ||
| ]); | ||
|
|
||
| let config = self.otel.as_ref().expect("OpenTelemetry config must be present"); |
There was a problem hiding this comment.
perhaps this is a code smell that indicates we should actually have this as a method on OpenTelemetryConfig instead of TelemetryConfig. If we do that, otel will never be None, and its also more clear that this function only should be called when OpenTelemetryConfig is available. Alternatively, it could be a free function (not a method) that takes an OpenTelemetryConfig as an argument, since its just a helper function.
|
|
||
| /// Try to initialize telemetry (journald/stderr + optional OTLP). | ||
| /// Returns an error if something goes wrong setting up the subscriber stack. | ||
| pub fn try_init(self) -> Result<(TelemetryShutdownHandler, Result<(), tracing_subscriber::util::TryInitError>), Box<dyn std::error::Error>> { |
There was a problem hiding this comment.
Ths type is quite strange. I think you can simplify it by using thiserror for your error type, and the type signature then becomes:
pub fn try_init(self) -> Result<TelemetryShutdownHandler, OrbTelemetryErr> {
// ...
}|
|
||
| impl Drop for TelemetryShutdownHandler { | ||
| fn drop(&mut self) { | ||
| global::shutdown_tracer_provider(); |
There was a problem hiding this comment.
what happens if we aren't using opentelemetry at all (for example, if we only did with_journald but did not do with_opentelemetry)? Will this panic?
| orb_telemetry::TelemetryConfig::new() | ||
| let _telemetry_guard = orb_telemetry::TelemetryConfig::new( | ||
| SYSLOG_IDENTIFIER, | ||
| env!("CARGO_PKG_VERSION"), |
There was a problem hiding this comment.
use orb-build-info crate
This reverts commit 66b2999.
No description provided.