-
Notifications
You must be signed in to change notification settings - Fork 80
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
chore(runtime): add otel support to runtime #2085
base: 02-21-chore_hub-embed_disable_all_node-related_code_if_hub_building_is_disabled
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR adds OpenTelemetry support to the Rivet runtime, enabling distributed tracing, metrics, and logging capabilities for improved observability.
- Added new
otel.rs
module that configures tracer, meter, and logger providers with GRPC export to configurable endpoint - Simplified runtime initialization by removing
RunConfig
struct and replacing with directrun()
calls - Fixed error handling in server main functions by correcting double question mark (
??
) issues - Added multiple OpenTelemetry-related dependencies in
Cargo.toml
including exporters and semantic conventions - Removed
RivetRuntime
error variant fromManagerError
enum as part of error handling refactoring
8 file(s) reviewed, 9 comment(s)
Edit PR Review Bot Settings | Greptile
opentelemetry = { version = "0.28", features = ["trace", "metrics", "logs"] } | ||
opentelemetry-otlp = { version = "0.28", features = ["trace", "metrics", "logs", "grpc-tonic"] } | ||
opentelemetry-semantic-conventions = { version = "0.28", features = ["semconv_experimental"] } | ||
opentelemetry-stdout = { version = "0.28.0", features = ["trace", "metrics", "logs"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: The version for opentelemetry-stdout includes a patch version (0.28.0) while other opentelemetry packages use just 0.28. Consider using consistent versioning across all opentelemetry packages.
packages/common/runtime/src/lib.rs
Outdated
@@ -1,157 +1,71 @@ | |||
use std::{env, future::Future, sync::Once, time::Duration}; | |||
use std::{env, future::Future, time::Duration, process}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: The process
module is imported but not used anywhere in the code
use std::{env, future::Future, time::Duration, process}; | |
use std::{env, future::Future, time::Duration}; |
}); | ||
|
||
if let Ok(thread_stack_size) = env::var("TOKIO_THREAD_STACK_SIZE") { | ||
rt_builder.thread_stack_size(thread_stack_size.parse().unwrap()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Using unwrap()
here will cause a panic if the environment variable contains an invalid integer. The previous implementation returned a Result with a proper error message.
} | ||
}) | ||
if let Ok(worker_threads) = env::var("TOKIO_WORKER_THREADS") { | ||
rt_builder.worker_threads(worker_threads.parse().unwrap()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Same issue with unwrap()
- this will panic on invalid input rather than returning an error like the previous implementation
packages/common/runtime/src/otel.rs
Outdated
use tracing_opentelemetry::{MetricsLayer, OpenTelemetryLayer}; | ||
use tracing_subscriber::{layer::SubscriberExt, util::SubscriberInitExt, EnvFilter}; | ||
|
||
const SERVICE_NAME_ENV: &str = "RIVET_SERVICE_NAME"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: SERVICE_NAME_ENV is defined but never used in the code.
packages/common/runtime/src/otel.rs
Outdated
[ | ||
KeyValue::new(SERVICE_NAME, env!("CARGO_PKG_NAME")), | ||
KeyValue::new(SERVICE_VERSION, env!("CARGO_PKG_VERSION")), | ||
KeyValue::new(DEPLOYMENT_ENVIRONMENT_NAME, "develop"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Environment is hardcoded as "develop". Consider using an environment variable like RIVET_ENVIRONMENT to make this configurable.
packages/common/runtime/src/otel.rs
Outdated
attribute::{DEPLOYMENT_ENVIRONMENT_NAME, SERVICE_NAME, SERVICE_VERSION}, | ||
SCHEMA_URL, | ||
}; | ||
use tracing_core::Level; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Level is imported from tracing_core but never used.
use opentelemetry_sdk::{ | ||
metrics::{MeterProviderBuilder, PeriodicReader, SdkMeterProvider, }, | ||
trace::{RandomIdGenerator, Sampler, SdkTracerProvider}, | ||
logs::SdkLoggerProvider, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Inconsistent indentation (tab instead of spaces) in the import block.
logs::SdkLoggerProvider, | |
logs::SdkLoggerProvider, |
packages/common/runtime/src/otel.rs
Outdated
OtelGuard { | ||
tracer_provider, | ||
meter_provider, | ||
logger_provider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Inconsistent indentation (tab instead of spaces) in the struct initialization.
logger_provider | |
logger_provider |
f299ca1
to
9b7cef1
Compare
825aabc
to
8b2c381
Compare
8b2c381
to
dd72a3b
Compare
Deploying rivet-hub with
|
Latest commit: |
76f9546
|
Status: | ✅ Deploy successful! |
Preview URL: | https://69ace143.rivet-hub-7jb.pages.dev |
Branch Preview URL: | https://02-26-chore-runtime-add-otel.rivet-hub-7jb.pages.dev |
dd72a3b
to
76f9546
Compare
9b7cef1
to
d02c977
Compare
76f9546
to
65091d6
Compare
d02c977
to
8c0f338
Compare
65091d6
to
2a8a4c4
Compare
8c0f338
to
d02c977
Compare
2a8a4c4
to
65091d6
Compare
Changes