Add `aws-smithy-observability` and `aws-smithy-observability-otel` crates #3912

landonxjames · 2024-11-11T20:04:51Z

Motivation and Context

We would like to have a consistent way to measure the performance of the SDK going forward so we can evaluate how updates change the performance over time. The changes in this PR are a first step towards that, adding the interfaces (and one implementation) that we will use to instrument our runtime crates.

Description

Add two new crates

aws-smithy-observability - contains traits for our observability solution and a global module for managing the global telemetry provider
aws-smithy-observability-otel - contains an OpenTelemetry based implementation of the traits

Testing

Added new tests in both crates

Checklist

Note: leaving out a changelog entry for now since these crates are somewhat useless until we instrument our runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

github-actions · 2024-11-11T20:21:35Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

rust-runtime/aws-smithy-observability-otel/Cargo.toml

github-actions · 2024-11-12T15:51:51Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

aajtodd

Good start but I have some questions and concerns to be discussed.

rust-runtime/aws-smithy-observability/README.md

aajtodd · 2024-11-12T16:17:38Z

rust-runtime/aws-smithy-observability/src/meter.rs

+/// Provides named instances of [Meter].
+pub trait MeterProvider {
+    /// Get or create a named [Meter].
+    fn get_meter(&self, scope: &'static str, attributes: Option<&Attributes>) -> Box<dyn Meter>;


discuss Should this be -> impl Meter?

I don't think that would work. Returning impl Meter would make MeterProvider not object safe and so we couldn't pass it around as a dyn everywhere.

From docs:

All associated functions must either be dispatchable from a trait object or be explicitly non-dispatchable:

Dispatchable functions must:

Not have an opaque return type; that is,

Not have a return position impl Trait type (fn example(&self) -> impl Trait).

rust-runtime/aws-smithy-observability/src/meter.rs

aajtodd · 2024-11-12T16:20:19Z

rust-runtime/aws-smithy-observability/src/meter.rs

+        callback: Box<dyn Fn(&dyn AsyncMeasurement<Value = f64>) + Send + Sync>,
+        units: Option<String>,
+        description: Option<String>,
+    ) -> Box<dyn AsyncMeasurement<Value = f64>>;


Same question should these be Box<T> or should they be impl -> T?

See response above, using impl T would make these not object safe
#3912 (comment)

aajtodd · 2024-11-12T16:22:07Z

rust-runtime/aws-smithy-observability/src/meter.rs

+    #[allow(clippy::type_complexity)]
+    fn create_gauge(
+        &self,
+        name: String,


These should probably be more flexible like impl Into<String> or impl Into<Cow<'static, str>>, etc.

I tried that, but once again it makes the Meter trait no longer object safe

aajtodd · 2024-11-12T16:37:23Z

rust-runtime/aws-smithy-observability/src/provider.rs

+    }
+
+    /// Get the set [MeterProvider]
+    pub fn meter_provider(&self) -> &(dyn MeterProvider + Send + Sync) {


should this return impl MeterProvider + Send + Sync?

I don't think you can return a reference to an impl Trait, and I didn't want to hand ownership of the MeterProvider to the caller, so settled for the &dyn that is there now.

aajtodd · 2024-11-12T16:41:18Z

rust-runtime/aws-smithy-observability/src/lib.rs

+//TODO(smithyobservability): once we have finalized everything and integrated metrics with our runtime
+// libraries update this with detailed usage docs and examples
+
+pub mod attributes;


Several of these modules probably should be private and just re-export their type instead.

use aws_smithy_observability::Error; use aws_smithy_observability::Attributes`

vs

use aws_smithy_observability::error::Error; use aws_smithy_observability::attributes::Attributes`

In other words consider how you'd like the API to be exposed and consumed.

Also missing docs on public modules.

Added some pub uses for these types in 0559e8b

The public modules are already documented, but the docs are in the module file instead of in lib.rs. I prefer keeping them there because I think it is cleaner than putting them all in lib.rs, but will move them if that is a consensus.

rust-runtime/aws-smithy-observability/src/attributes.rs

rust-runtime/aws-smithy-observability/src/meter.rs

aajtodd · 2024-11-12T16:49:39Z

rust-runtime/aws-smithy-observability/src/meter.rs

+}
+
+/// Collects a set of events with an event count and sum for all events.
+pub trait Histogram {


Do we need to consider whether we want to force (or need) instrument types to implement Clone?

Sized is a supertrait of Clone and Sized supertraits break object safety

ysaito1001

Thanks for the work! Will circle back, leaving initial comments...

rust-runtime/aws-smithy-observability-otel/Cargo.toml

rust-runtime/aws-smithy-observability-otel/src/attributes.rs

rust-runtime/aws-smithy-observability-otel/src/meter.rs

rust-runtime/aws-smithy-observability/src/meter.rs

github-actions · 2024-11-13T21:53:45Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

rust-runtime/aws-smithy-observability/src/meter.rs

ysaito1001 · 2024-11-14T17:23:52Z

I see that the design specifies an async API such as create_guage returns an async measurement handle. Do we need to support that, not necessarily in this PR but before the aws-smithy-observability goes 1.0 (in which case, like many traits do in the smithy runtime crates, a trait needs to return a struct and that struct implements the Future trait)?

github-actions · 2024-11-18T02:36:40Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

landonxjames · 2024-11-18T22:16:34Z

I see that the design specifies an async API such as create_guage returns an async measurement handle. Do we need to support that, not necessarily in this PR but before the aws-smithy-observability goes 1.0 (in which case, like many traits do in the smithy runtime crates, a trait needs to return a struct and that struct implements the Future trait)?

The AsyncMeasurement name is kind of a confusing one in Rust since it doesn't really map to async fn, but is really a synchronous callback function registered when the instrument is created that is invoked when a instrument is observed. This bit in the SRA was inspired by OpenTelemetry and you can see their description of it here and their Rust implementation of it here

github-actions · 2024-11-18T22:38:35Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2024-11-19T23:25:38Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2024-11-20T00:16:55Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

ysaito1001

Thanks for the great work! Left the final round of comments.

rust-runtime/aws-smithy-observability/src/attributes.rs

rust-runtime/aws-smithy-observability/src/global.rs

github-actions · 2024-11-21T18:45:43Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2024-11-21T23:14:40Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2024-11-22T18:39:15Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

github-actions · 2024-11-23T04:22:48Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Also fix all warnings in both observability crates

The updates caused CI to fail since it depended on the newly added crates, but those were not yet present in the main lockfile that is copied around for tests

* Update some clippy style issues * Config the aws-smithy-observability-otel crate to not compile on powerpc * Run tests for global module serially so they don't poison the RwLock

Pin version of value-bag in otel crate Powerpc dep updates Pin more transitive deps

Co-authored-by: Zelda Hessler <zhessler@amazon.com>

Aligns with standard introduced in #3065 Update doc comments External types updates

Remove non-public type from docs

github-actions · 2024-11-25T20:53:17Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

landonxjames changed the title ~~Landonxjames/obs~~ Add aws-smithy-observability and aws-smithy-observability-otel crates Nov 11, 2024

landonxjames marked this pull request as ready for review November 11, 2024 20:34

landonxjames requested review from a team as code owners November 11, 2024 20:34

Velfi reviewed Nov 12, 2024

View reviewed changes

rust-runtime/aws-smithy-observability-otel/Cargo.toml Outdated Show resolved Hide resolved

Velfi approved these changes Nov 12, 2024

View reviewed changes

aajtodd requested changes Nov 12, 2024

View reviewed changes

ysaito1001 reviewed Nov 12, 2024

View reviewed changes

rust-runtime/aws-smithy-observability-otel/Cargo.toml Show resolved Hide resolved

rust-runtime/aws-smithy-observability-otel/src/attributes.rs Outdated Show resolved Hide resolved

rust-runtime/aws-smithy-observability-otel/src/meter.rs Show resolved Hide resolved

ysaito1001 reviewed Nov 12, 2024

View reviewed changes

rust-runtime/aws-smithy-observability/src/meter.rs Outdated Show resolved Hide resolved

ysaito1001 reviewed Nov 13, 2024

View reviewed changes

rust-runtime/aws-smithy-observability/src/meter.rs Outdated Show resolved Hide resolved

landonxjames added the needs-server-review label Nov 14, 2024

landonxjames force-pushed the landonxjames/obs branch from 71cd2ef to 0559e8b Compare November 18, 2024 01:48

landonxjames force-pushed the landonxjames/obs branch from fbba9b4 to 88c9f4c Compare November 19, 2024 22:58

landonxjames force-pushed the landonxjames/obs branch from 88c9f4c to f1f5be2 Compare November 19, 2024 23:59

ysaito1001 approved these changes Nov 20, 2024

View reviewed changes

rust-runtime/aws-smithy-observability/src/attributes.rs Outdated Show resolved Hide resolved

rust-runtime/aws-smithy-observability/src/global.rs Outdated Show resolved Hide resolved

landonxjames force-pushed the landonxjames/obs branch from 9f0df17 to 4e11e53 Compare November 21, 2024 22:56

landonxjames enabled auto-merge November 25, 2024 17:52

landonxjames disabled auto-merge November 25, 2024 18:07

landonxjames and others added 22 commits November 25, 2024 12:35

First impl of Metrics interfaces

35c44be

Updates to observability interfaces

55957b0

Chunk of the Otel TelemetryProvider impled

c06061e

Pausing work on metering

a20f9e9

Further along piping callback through telemetry

005094d

First compiling pass of Otel metrics impl

53593fc

Add initial working test for otel metric provider

eccf5f4

Add tests for Otel/Sdk attribute conversions

769a09f

Also fix all warnings in both observability crates

Add testing to aws-smithy-observability-otel crate

178ec48

Cleaning up some tests and doc comments

d57288a

Update READMEs leave TODO about adding usage docs

0b01272

Revert changes to rust-runtime/Cargo.lock

cd38cf9

The updates caused CI to fail since it depended on the newly added crates, but those were not yet present in the main lockfile that is copied around for tests

Updates for CI failures

759bb0f

* Update some clippy style issues * Config the aws-smithy-observability-otel crate to not compile on powerpc * Run tests for global module serially so they don't poison the RwLock

Update rust-runtime/Cargo.lock to get around CI issues

ba0fd95

Exclude otel dependencies from powerpc target_arch

0578511

Pin version of value-bag in otel crate Powerpc dep updates Pin more transitive deps

Update external types for observability crates

5e0c3b8

Update rust-runtime/aws-smithy-observability-otel/Cargo.toml

0c1fc59

Co-authored-by: Zelda Hessler <zhessler@amazon.com>

PR feedback

a5fff54

Add Send + Sync + Debug supertraits to obs traits

26f4dd5

Rename some traits to be more verby

93da5f9

Aligns with standard introduced in #3065 Update doc comments External types updates

PR feedback

c2a9af9

Remove non-public type from docs

Refactor get/set_telemetry_provider functions to return Result

05bb688

landonxjames force-pushed the landonxjames/obs branch from b964231 to 05bb688 Compare November 25, 2024 20:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `aws-smithy-observability` and `aws-smithy-observability-otel` crates #3912

Add `aws-smithy-observability` and `aws-smithy-observability-otel` crates #3912

landonxjames commented Nov 11, 2024

github-actions bot commented Nov 11, 2024

github-actions bot commented Nov 12, 2024

aajtodd left a comment

aajtodd Nov 12, 2024

landonxjames Nov 13, 2024

aajtodd Nov 12, 2024

landonxjames Nov 13, 2024

aajtodd Nov 12, 2024

landonxjames Nov 13, 2024

aajtodd Nov 12, 2024

landonxjames Nov 18, 2024

aajtodd Nov 12, 2024

landonxjames Nov 18, 2024

aajtodd Nov 12, 2024

landonxjames Nov 13, 2024

ysaito1001 left a comment

github-actions bot commented Nov 13, 2024

ysaito1001 commented Nov 14, 2024

github-actions bot commented Nov 18, 2024

landonxjames commented Nov 18, 2024

github-actions bot commented Nov 18, 2024

github-actions bot commented Nov 19, 2024

github-actions bot commented Nov 20, 2024

ysaito1001 left a comment

github-actions bot commented Nov 21, 2024

github-actions bot commented Nov 21, 2024

github-actions bot commented Nov 22, 2024

github-actions bot commented Nov 23, 2024

github-actions bot commented Nov 25, 2024

Add aws-smithy-observability and aws-smithy-observability-otel crates #3912

Are you sure you want to change the base?

Add aws-smithy-observability and aws-smithy-observability-otel crates #3912

Conversation

landonxjames commented Nov 11, 2024

Motivation and Context

Description

Testing

Checklist

github-actions bot commented Nov 11, 2024

github-actions bot commented Nov 12, 2024

aajtodd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysaito1001 left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 13, 2024

ysaito1001 commented Nov 14, 2024

github-actions bot commented Nov 18, 2024

landonxjames commented Nov 18, 2024

github-actions bot commented Nov 18, 2024

github-actions bot commented Nov 19, 2024

github-actions bot commented Nov 20, 2024

ysaito1001 left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 21, 2024

github-actions bot commented Nov 21, 2024

github-actions bot commented Nov 22, 2024

github-actions bot commented Nov 23, 2024

github-actions bot commented Nov 25, 2024

Add `aws-smithy-observability` and `aws-smithy-observability-otel` crates #3912

Add `aws-smithy-observability` and `aws-smithy-observability-otel` crates #3912