Merge branch 'main' into add-timeout-duration-to-shutdown

Author: mohammadVatandoost

.github/workflows/integration_tests.yml

@@ -23,5 +23,7 @@ jobs:
         with:
           components: rustfmt
       - uses: arduino/setup-protoc@v3
+        with:
+          repo-token: ${{ secrets.GITHUB_TOKEN }}
       - name: Run integration tests
         run: ./scripts/integration_tests.sh

.github/workflows/pr_criterion.yaml

@@ -6,9 +6,12 @@ jobs:
     permissions:
       pull-requests: write
     runs-on: ubuntu-latest
+    if: ${{ contains(github.event.pull_request.labels.*.name, 'performance') }}
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
       - uses: arduino/setup-protoc@v3
+        with:
+          repo-token: ${{ secrets.GITHUB_TOKEN }}
       - uses: dtolnay/rust-toolchain@master
         with:
           toolchain: stable

.github/workflows/pr_naming.yml

@@ -9,7 +9,7 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - name: PR Conventional Commit Validation
-        uses:  ytanikin/[email protected].0
+        uses:  ytanikin/[email protected].1
         with:
           task_types: '["build","chore","ci","docs","feat","fix","perf","refactor","revert","test"]'
           add_label: 'false'

CONTRIBUTING.md

@@ -8,7 +8,7 @@ for specific dates and for Zoom meeting links. "OTel Rust SIG" is the name of
 meeting for this group.
 
 Meeting notes are available as a public [Google
-doc](https://docs.google.com/document/d/1tGKuCsSnyT2McDncVJrMgg74_z8V06riWZa0Sr79I_4/edit).
+doc](https://docs.google.com/document/d/12upOzNk8c3SFTjsL6IRohCWMgzLKoknSCOOdMakbWo4/edit).
 If you have trouble accessing the doc, please get in touch on
 [Slack](https://cloud-native.slack.com/archives/C03GDP0H023).
 

Cargo.toml

@@ -85,5 +85,12 @@ opentelemetry = { path = "opentelemetry" }
 opentelemetry_sdk = { path = "opentelemetry-sdk" }
 opentelemetry-stdout = { path = "opentelemetry-stdout" }
 
+[workspace.lints.rust]
+rust_2024_compatibility = { level = "warn", priority = -1 }
+# No need to enable those, because it either not needed or results in ugly syntax
+edition_2024_expr_fragment_specifier = "allow"
+if_let_rescope = "allow"
+tail_expr_drop_order = "allow"
+
 [workspace.lints.clippy]
 all = { level = "warn", priority = 1 }

docs/adr/001_error_handling.md

@@ -0,0 +1,173 @@
+# Error handling patterns in public API interfaces
+## Date
+27 Feb 2025 
+
+## Summary
+
+This ADR describes the general pattern we will follow when modelling errors in public API interfaces - that is, APIs that are exposed to users of the project's published crates. It summarises the discussion and final option from [#2571](https://github.com/open-telemetry/opentelemetry-rust/issues/2571); for more context check out that issue. 
+
+We will focus on the exporter traits in this example, but the outcome should be applied to _all_ public traits and their fallible operations. 
+
+These include [SpanExporter](https://github.com/open-telemetry/opentelemetry-rust/blob/eca1ce87084c39667061281e662d5edb9a002882/opentelemetry-sdk/src/trace/export.rs#L18), [LogExporter](https://github.com/open-telemetry/opentelemetry-rust/blob/eca1ce87084c39667061281e662d5edb9a002882/opentelemetry-sdk/src/logs/export.rs#L115), and [PushMetricExporter](https://github.com/open-telemetry/opentelemetry-rust/blob/eca1ce87084c39667061281e662d5edb9a002882/opentelemetry-sdk/src/metrics/exporter.rs#L11) which form part of the API surface of `opentelemetry-sdk`.
+
+There are various ways to handle errors on trait methods, including swallowing them and logging, panicing, returning a shared global error, or returning a method-specific error. We strive for consistency, and we want to be sure that we've put enough thought into what this looks like that we don't have to make breaking interface changes unecessarily in the future.
+
+## Design Guidance
+
+### 1. No panics from SDK APIs
+Failures during regular operation should not panic, instead returning errors to the caller where appropriate, _or_ logging an error if not appropriate.
+Some of the opentelemetry SDK interfaces are dictated by the specification in way such that they may not return errors. 
+
+### 2. Consolidate error types within a trait where we can, let them diverge when we can't**
+
+We aim to consolidate error types where possible _without indicating a function may return more errors than it can actually return_. 
+
+**Don't do this** - each function's signature indicates that it returns errors it will _never_ return, forcing the caller to write handlers for dead paths:
+```rust
+enum MegaError {
+  TooBig,
+  TooSmall,
+  TooLong,
+  TooShort
+}
+
+trait MyTrait {
+
+  // Will only ever return TooBig,TooSmall errors
+  fn action_one() -> Result<(), MegaError>;
+
+  // These will only ever return TooLong,TooShort errors
+  fn action_two() -> Result<(), MegaError>;
+  fn action_three() -> Result<(), MegaError>;
+}
+```
+
+**Instead, do this** - each function's signature indicates only the errors it can return, providing an accurate contract to the caller:
+
+```rust
+enum ErrorOne {
+  TooBig,
+  TooSmall,
+}
+
+enum ErrorTwo {
+  TooLong,
+  TooShort
+}
+
+trait MyTrait {
+  fn action_one() -> Result<(), ErrorOne>;
+
+  // Action two and three share the same error type. 
+  // We do not introduce a common error MyTraitError for all operations, as this would
+  // force all methods on the trait to indicate they return errors they do not return,
+  // complicating things for the caller.  
+  fn action_two() -> Result<(), ErrorTwo>;
+  fn action_three() -> Result<(), ErrorTwo>;
+}
+```
+
+## 3. Consolidate error types between signals where we can, let them diverge where we can't
+
+Consider the `Exporter`s mentioned earlier. Each of them has the same failure indicators - as dicated by the OpenTelemetry spec  - and we will
+share the error types accordingly: 
+
+**Don't do this** - each signal has its own error type, despite having exactly the same failure cases: 
+
+```rust
+#[derive(Error, Debug)]
+pub enum OtelTraceError {
+    #[error("Shutdown already invoked")]
+    AlreadyShutdown,
+    
+    #[error("Operation failed: {0}")]
+    InternalFailure(String),
+
+    /** ... additional errors ... **/ 
+}
+
+#[derive(Error, Debug)]
+pub enum OtelLogError {
+    #[error("Shutdown already invoked")]
+    AlreadyShutdown,
+    
+    #[error("Operation failed: {0}")]
+    InternalFailure(String),
+
+    /** ... additional errors ... **/ 
+}
+```
+
+**Instead, do this** - error types are consolidated between signals where this can be done appropriately:
+
+```rust
+
+/// opentelemetry-sdk::error
+
+#[derive(Error, Debug)]
+pub enum OTelSdkError {
+    #[error("Shutdown already invoked")]
+    AlreadyShutdown,
+    
+    #[error("Operation failed: {0}")]
+    InternalFailure(String),
+
+    /** ... additional errors ... **/ 
+}
+
+pub type OTelSdkResult = Result<(), OTelSdkError>;
+
+/// signal-specific exporter traits all share the same 
+/// result types for the exporter operations.
+
+// pub trait LogExporter {
+// pub trait SpanExporter {
+pub trait PushMetricExporter {
+    fn export(&self, /* ... */) -> OtelSdkResult;
+    fn force_flush(&self, /* ... */ ) -> OTelSdkResult;
+    fn shutdown(&self, /* ... */ ) -> OTelSdkResult;
+```
+
+If this were _not_ the case - if we needed to mark an extra error for instance for `LogExporter` that the caller could reasonably handle - 
+we would let that error traits diverge at that point. 
+
+### 4. Box custom errors where a savvy caller may be able to handle them, stringify them if not
+
+Note above that we do not box any `Error` into `InternalFailure`. Our rule here is that if the caller cannot reasonably be expected to handle a particular error variant, we will use a simplified interface that returns only a descriptive string. In the concrete example we are using with the exporters, we have a [strong signal in the opentelemetry-specification](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/sdk.md#export) that indicates that the error types _are not actionable_ by the caller. 
+
+If the caller may potentially recover from an error, we will follow the generally-accepted best practice (e.g., see [canonical's guide](https://canonical.github.io/rust-best-practices/error-and-panic-discipline.html) and instead preserve the nested error:
+
+**Don't do this if the OtherError is potentially recoverable by a savvy caller**:
+```rust
+
+#[derive(Debug, Error)]
+pub enum MyError {
+    #[error("Error one occurred")]
+    ErrorOne, 
+
+    #[error("Operation failed: {0}")]
+    OtherError(String),
+```
+
+**Instead, do this**, allowing the caller to match on the nested error:
+
+```rust
+#[derive(Debug, Error)]
+pub enum MyError {
+    #[error("Error one occurred")]
+    ErrorOne, 
+
+    #[error("Operation failed: {source}")]
+    OtherError {
+        #[from]
+        source: Box<dyn Error + Send + Sync>,
+    },
+}
+```
+
+Note that at the time of writing, there is no instance we have identified within the project that has required this. 
+
+### 5. Use thiserror by default
+We will use [thiserror](https://docs.rs/thiserror/latest/thiserror/) by default to implement Rust's [error trait](https://doc.rust-lang.org/core/error/trait.Error.html).
+This keeps our code clean, and as it does not appear in our interface, we can choose to replace any particular usage with a hand-rolled implementation should we need to.
+

docs/adr/README.md

@@ -0,0 +1,5 @@
+# Architectural Decision Records
+
+This directory contains architectural decision records made for the opentelemetry-rust project. These allow us to consolidate discussion, options, and outcomes, around key architectural decisions. 
+
+* [001 - Error Handling](001_error_handling.md)

docs/design/logs.md

@@ -296,6 +296,19 @@ convey that decision back to logger, allowing appender to avoid even the cost of
 creating a `LogRecord` in the first place if there is no listener. This check is
 done for each log emission, and can react dynamically to changes in interest, by
 enabling/disabling ETW/user-event listener.
+5. `tracing` has a notion of "target", which is expected to be mapped to OTel's
+concept of Instrumentation Scope for Logs, when `OpenTelemetry-Tracing-Appender`
+bridges `tracing` to OpenTelemetry. Since scopes are tied to Loggers, a naive
+approach would require creating a separate logger for each unique target. This
+would necessitate an RWLock-protected HashMap lookup, introducing contention and
+reducing throughput. To avoid this, `OpenTelemetry-Tracing-Appender` instead
+stores the target directly in the LogRecord as a top-level field, ensuring fast
+access in the hot path. Components processing the LogRecord can retrieve the
+target via LogRecord.target(), treating it as the scope. The OTLP Exporter
+already handles this automatically, so end-users will see “target” reflected in
+the Instrumentation Scope. An alternative design would be to use thread-local
+HashMaps - but it can cause increased memory usage, as there can be 100s of
+unique targets. (because `tracing` defaults to using module path as target).
 
 ### Perf test - benchmarks
 

examples/tracing-grpc/Cargo.toml

@@ -19,7 +19,7 @@ opentelemetry_sdk = { path = "../../opentelemetry-sdk", features = ["rt-tokio"]
 opentelemetry-stdout = { workspace = true, features = ["trace"] }
 prost = { workspace = true }
 tokio = { workspace = true, features = ["full"] }
-tonic = { workspace = true, features = ["server"] }
+tonic = { workspace = true, features = ["server", "codegen", "channel", "prost"] }
 
 [build-dependencies]
 tonic-build = { workspace = true }

examples/tracing-grpc/src/client.rs

@@ -13,7 +13,7 @@ fn init_tracer() -> sdktrace::SdkTracerProvider {
     global::set_text_map_propagator(TraceContextPropagator::new());
     // Install stdout exporter pipeline to be able to retrieve the collected spans.
     let provider = sdktrace::SdkTracerProvider::builder()
-        .with_batch_exporter(SpanExporter::default())
+        .with_simple_exporter(SpanExporter::default())
         .build();
 
     global::set_tracer_provider(provider.clone());
@@ -43,10 +43,14 @@ async fn greet() -> Result<(), Box<dyn std::error::Error + Send + Sync + 'static
     let span = tracer
         .span_builder("Greeter/client")
         .with_kind(SpanKind::Client)
-        .with_attributes([KeyValue::new("component", "grpc")])
+        .with_attributes([
+            KeyValue::new("rpc.system", "grpc"),
+            KeyValue::new("server.port", 50052),
+            KeyValue::new("rpc.method", "say_hello"),
+        ])
         .start(&tracer);
     let cx = Context::current_with_span(span);
-    let mut client = GreeterClient::connect("http://[::1]:50051").await?;
+    let mut client = GreeterClient::connect("http://[::1]:50052").await?;
 
     let mut request = tonic::Request::new(HelloRequest {
         name: "Tonic".into(),
@@ -58,16 +62,23 @@ async fn greet() -> Result<(), Box<dyn std::error::Error + Send + Sync + 'static
 
     let response = client.say_hello(request).await;
 
+    let span = cx.span();
     let status = match response {
-        Ok(_res) => "OK".to_string(),
+        Ok(_res) => {
+            span.set_attribute(KeyValue::new("response", "OK"));
+            "OK".to_string()
+        }
         Err(status) => {
             // Access the status code
             let status_code = status.code();
+            span.set_attribute(KeyValue::new(
+                "response_code_desc",
+                status_code.description(),
+            ));
             status_code.to_string()
         }
     };
-    cx.span()
-        .add_event("Got response!", vec![KeyValue::new("status", status)]);
+    span.add_event("Got response!", vec![KeyValue::new("status", status)]);
 
     Ok(())
 }