From da9f2b6c7df04c0e51b043c67967baad1488dc36 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Fri, 7 Nov 2025 16:14:47 -0800 Subject: [PATCH 01/13] Fixed issues validating tracing pages which were converted to page iterators --- sdk/core/azure_core/src/http/pager.rs | 46 +++- sdk/core/azure_core_test/src/tracing.rs | 89 +++++++- sdk/keyvault/assets.json | 2 +- .../tests/secret_client.rs | 207 ++++++++++++++---- 4 files changed, 276 insertions(+), 68 deletions(-) diff --git a/sdk/core/azure_core/src/http/pager.rs b/sdk/core/azure_core/src/http/pager.rs index 9ae1b94f95..bc5518bfb1 100644 --- a/sdk/core/azure_core/src/http/pager.rs +++ b/sdk/core/azure_core/src/http/pager.rs @@ -391,10 +391,10 @@ impl ItemIterator

{ /// ``` pub fn from_callback< // This is a bit gnarly, but the only thing that differs between the WASM/non-WASM configs is the presence of Send bounds. - #[cfg(not(target_arch = "wasm32"))] C: AsRef + FromStr + Send + 'static, + #[cfg(not(target_arch = "wasm32"))] C: AsRef + FromStr + fmt::Debug + Send + 'static, #[cfg(not(target_arch = "wasm32"))] F: Fn(PagerState, Context<'static>) -> Fut + Send + 'static, #[cfg(not(target_arch = "wasm32"))] Fut: Future>> + Send + 'static, - #[cfg(target_arch = "wasm32")] C: AsRef + FromStr + 'static, + #[cfg(target_arch = "wasm32")] C: AsRef + FromStr + fmt::Debug + 'static, #[cfg(target_arch = "wasm32")] F: Fn(PagerState, Context<'static>) -> Fut + 'static, #[cfg(target_arch = "wasm32")] Fut: Future>> + 'static, >( @@ -693,10 +693,10 @@ impl

PageIterator

{ /// ``` pub fn from_callback< // This is a bit gnarly, but the only thing that differs between the WASM/non-WASM configs is the presence of Send bounds. - #[cfg(not(target_arch = "wasm32"))] C: AsRef + FromStr + Send + 'static, + #[cfg(not(target_arch = "wasm32"))] C: AsRef + FromStr + fmt::Debug + Send + 'static, #[cfg(not(target_arch = "wasm32"))] F: Fn(PagerState, Context<'static>) -> Fut + Send + 'static, #[cfg(not(target_arch = "wasm32"))] Fut: Future>> + Send + 'static, - #[cfg(target_arch = "wasm32")] C: AsRef + FromStr + 'static, + #[cfg(target_arch = "wasm32")] C: AsRef + FromStr + fmt::Debug + 'static, #[cfg(target_arch = "wasm32")] F: Fn(PagerState, Context<'static>) -> Fut + 'static, #[cfg(target_arch = "wasm32")] Fut: Future>> + 'static, >( @@ -812,13 +812,19 @@ impl

fmt::Debug for PageIterator

{ } #[derive(Debug, Clone, Eq)] -enum State { +enum State +where + C: fmt::Debug, +{ Init, More(C), Done, } -impl PartialEq for State { +impl PartialEq for State +where + C: fmt::Debug, +{ fn eq(&self, other: &Self) -> bool { // Only needs to compare if both states are Init or Done; internally, we don't care about any other states. matches!( @@ -829,7 +835,10 @@ impl PartialEq for State { } #[derive(Debug)] -struct StreamState<'a, C, F> { +struct StreamState<'a, C, F> +where + C: fmt::Debug, +{ state: State, make_request: F, continuation_token: Arc>>, @@ -840,10 +849,10 @@ struct StreamState<'a, C, F> { fn iter_from_callback< P, // This is a bit gnarly, but the only thing that differs between the WASM/non-WASM configs is the presence of Send bounds. - #[cfg(not(target_arch = "wasm32"))] C: AsRef + FromStr + Send + 'static, + #[cfg(not(target_arch = "wasm32"))] C: AsRef + fmt::Debug + FromStr + Send + 'static, #[cfg(not(target_arch = "wasm32"))] F: Fn(PagerState, Context<'static>) -> Fut + Send + 'static, #[cfg(not(target_arch = "wasm32"))] Fut: Future>> + Send + 'static, - #[cfg(target_arch = "wasm32")] C: AsRef + FromStr + 'static, + #[cfg(target_arch = "wasm32")] C: AsRef + FromStr + fmt::Debug + 'static, #[cfg(target_arch = "wasm32")] F: Fn(PagerState, Context<'static>) -> Fut + 'static, #[cfg(target_arch = "wasm32")] Fut: Future>> + 'static, >( @@ -870,7 +879,24 @@ where let result = match stream_state.continuation_token.lock() { Ok(next_token) => match next_token.as_deref() { Some(n) => match n.parse() { - Ok(s) => Ok(State::More(s)), + Ok(s) => { + // When resuming from a continuation token, create a span for the entire request, and attach it to the context. + if stream_state.state == State::Init { + tracing::debug!( + "resuming pager from continuation token: {:?}", + n + ); + + // At the very start of polling, create a span for the entire request, and attach it to the context + let span = + create_public_api_span(&stream_state.ctx, None, None); + if let Some(ref s) = span { + stream_state.added_span = true; + stream_state.ctx = stream_state.ctx.with_value(s.clone()); + } + } + Ok(State::More(s)) + } Err(err) => Err(crate::Error::with_message_fn( ErrorKind::DataConversion, || format!("invalid continuation token: {err}"), diff --git a/sdk/core/azure_core_test/src/tracing.rs b/sdk/core/azure_core_test/src/tracing.rs index dc4296144f..244a17d838 100644 --- a/sdk/core/azure_core_test/src/tracing.rs +++ b/sdk/core/azure_core_test/src/tracing.rs @@ -65,10 +65,10 @@ impl TracerProvider for MockTracingProvider { /// Mock Tracer - used for testing distributed tracing without involving a specific tracing implementation. #[derive(Debug)] pub struct MockTracer { - pub namespace: Option<&'static str>, - pub package_name: &'static str, - pub package_version: Option<&'static str>, - pub spans: Mutex>>, + namespace: Option<&'static str>, + package_name: &'static str, + package_version: Option<&'static str>, + spans: Mutex>>, } impl Tracer for MockTracer { @@ -83,9 +83,14 @@ impl Tracer for MockTracer { attributes: Vec, parent: Arc, ) -> Arc { - let span = Arc::new(MockSpan::new(name, kind, attributes.clone(), Some(parent))); + let span = Arc::new(MockSpanInner::new( + name, + kind, + attributes.clone(), + Some(parent), + )); self.spans.lock().unwrap().push(span.clone()); - span + Arc::new(MockSpan { inner: span }) } fn start_span( @@ -101,15 +106,15 @@ impl Tracer for MockTracer { value: attr.value.clone(), }) .collect(); - let span = Arc::new(MockSpan::new(name, kind, attributes, None)); + let span = Arc::new(MockSpanInner::new(name, kind, attributes, None)); self.spans.lock().unwrap().push(span.clone()); - span + Arc::new(MockSpan { inner: span }) } } /// Mock span for testing purposes. #[derive(Debug)] -pub struct MockSpan { +struct MockSpanInner { pub name: Cow<'static, str>, pub kind: SpanKind, pub parent: Option<[u8; 8]>, @@ -118,7 +123,7 @@ pub struct MockSpan { pub state: Mutex, pub is_open: Mutex, } -impl MockSpan { +impl MockSpanInner { fn new( name: C, kind: SpanKind, @@ -144,9 +149,22 @@ impl MockSpan { is_open: Mutex::new(true), } } + + fn is_open(&self) -> bool { + let is_open = self.is_open.lock().unwrap(); + *is_open + } } -impl Span for MockSpan { +impl AsAny for MockSpanInner { + fn as_any(&self) -> &dyn std::any::Any { + // Convert to an object that doesn't expose the lifetime parameter + // We're essentially erasing the lifetime here to satisfy the static requirement + self as &dyn std::any::Any + } +} + +impl Span for MockSpanInner { fn set_attribute(&self, key: &'static str, value: AttributeValue) { eprintln!("{}: Setting attribute {}: {:?}", self.name, key, value); let mut attributes = self.attributes.lock().unwrap(); @@ -195,6 +213,19 @@ impl Span for MockSpan { } } +pub struct MockSpan { + inner: Arc, +} + +impl Drop for MockSpan { + fn drop(&mut self) { + if self.inner.is_open() { + eprintln!("Warning: Dropping open span: {}", self.inner.name); + self.inner.end(); + } + } +} + impl AsAny for MockSpan { fn as_any(&self) -> &dyn std::any::Any { // Convert to an object that doesn't expose the lifetime parameter @@ -203,6 +234,40 @@ impl AsAny for MockSpan { } } +impl Span for MockSpan { + fn set_attribute(&self, key: &'static str, value: AttributeValue) { + self.inner.set_attribute(key, value); + } + + fn set_status(&self, status: crate::tracing::SpanStatus) { + self.inner.set_status(status); + } + + fn end(&self) { + self.inner.end(); + } + + fn is_recording(&self) -> bool { + self.inner.is_recording() + } + + fn span_id(&self) -> [u8; 8] { + self.inner.span_id() + } + + fn record_error(&self, error: &dyn std::error::Error) { + self.inner.record_error(error); + } + + fn set_current(&self, context: &Context) -> Box { + self.inner.set_current(context) + } + + fn propagate_headers(&self, request: &mut Request) { + self.inner.propagate_headers(request); + } +} + /// Expected information about a tracer. #[derive(Debug)] pub struct ExpectedTracerInformation<'a> { @@ -293,7 +358,7 @@ pub struct ExpectedSpanInformation<'a> { } fn check_span_information( - span: &Arc, + span: &Arc, expected: &ExpectedSpanInformation<'_>, parent_span_map: &HashMap, ) { diff --git a/sdk/keyvault/assets.json b/sdk/keyvault/assets.json index debd05b5d2..595344126c 100644 --- a/sdk/keyvault/assets.json +++ b/sdk/keyvault/assets.json @@ -2,5 +2,5 @@ "AssetsRepo": "Azure/azure-sdk-assets", "AssetsRepoPrefixPath": "rust", "TagPrefix": "rust/keyvault", - "Tag": "rust/keyvault_7e75eabce0" + "Tag": "rust/keyvault_5961c5368d" } diff --git a/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs b/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs index 59319b0bb9..ec6f5cd39f 100644 --- a/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs +++ b/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs @@ -347,6 +347,7 @@ async fn list_secrets_verify_telemetry(ctx: TestContext) -> Result<()> { let mut secrets = client.list_secret_properties(None)?; while let Some(secret) = secrets.try_next().await? { let _ = secret.resource_id()?; + println!("Found secret: {}", secret.resource_id()?.name); } Ok(()) @@ -379,6 +380,14 @@ async fn list_secrets_verify_telemetry(ctx: TestContext) -> Result<()> { api_verb: azure_core::http::Method::Get, ..Default::default() }, + ExpectedRestApiSpan { + api_verb: azure_core::http::Method::Get, + ..Default::default() + }, + ExpectedRestApiSpan { + api_verb: azure_core::http::Method::Get, + ..Default::default() + }, ], ..Default::default() }], @@ -390,7 +399,116 @@ async fn list_secrets_verify_telemetry(ctx: TestContext) -> Result<()> { } #[recorded::test] -#[ignore = "Test does not currently work because instrumentation of PageIterators doesn't quite work."] +async fn list_secrets_by_pages_verify_telemetry(ctx: TestContext) -> Result<()> { + use azure_core_test::tracing::ExpectedRestApiSpan; + + const SECRET_COUNT: usize = 50; + + let recording = ctx.recording(); + + { + let secret_client = { + let mut options = SecretClientOptions::default(); + recording.instrument(&mut options.client_options); + SecretClient::new( + recording.var("AZURE_KEYVAULT_URL", None).as_str(), + recording.credential(), + Some(options), + ) + }?; + for i in 0..SECRET_COUNT { + let secret = secret_client + .set_secret( + &format!("secret-list-telemetry-by-page{}", i), + SetSecretParameters { + value: Some(format!("secret-list-telemetry-by-page-value-{}", i)), + ..Default::default() + } + .try_into()?, + None, + ) + .await? + .into_model()?; + assert_eq!( + secret.value, + Some(format!("secret-list-telemetry-by-page-value-{}", i)) + ); + } + } + // Verify that the distributed tracing traces generated from the API call below match the expected traces. + let validate_result = azure_core_test::tracing::assert_instrumentation_information( + |tracer_provider| { + let mut options = SecretClientOptions::default(); + recording.instrument(&mut options.client_options); + options.client_options.instrumentation = InstrumentationOptions { + tracer_provider: Some(tracer_provider), + }; + SecretClient::new( + recording.var("AZURE_KEYVAULT_URL", None).as_str(), + recording.credential(), + Some(options), + ) + }, + |client: SecretClient| { + Box::pin(async move { + let mut secrets = client.list_secret_properties(None)?.into_pages(); + while let Some(page) = secrets.try_next().await? { + let items = page.into_model()?; + for item in items.value { + let _ = item.resource_id()?; + } + } + + Ok(()) + }) + }, + ExpectedInstrumentation { + package_name: recording.var("CARGO_PKG_NAME", None), + package_version: recording.var("CARGO_PKG_VERSION", None), + package_namespace: Some("KeyVault"), + api_calls: vec![ExpectedApiInformation { + api_name: Some("KeyVault.getSecrets"), + api_children: vec![ + ExpectedRestApiSpan { + api_verb: azure_core::http::Method::Get, + ..Default::default() + }, + ExpectedRestApiSpan { + api_verb: azure_core::http::Method::Get, + ..Default::default() + }, + ExpectedRestApiSpan { + api_verb: azure_core::http::Method::Get, + ..Default::default() + }, + ExpectedRestApiSpan { + api_verb: azure_core::http::Method::Get, + ..Default::default() + }, + ExpectedRestApiSpan { + api_verb: azure_core::http::Method::Get, + ..Default::default() + }, + ExpectedRestApiSpan { + api_verb: azure_core::http::Method::Get, + ..Default::default() + }, + ExpectedRestApiSpan { + api_verb: azure_core::http::Method::Get, + ..Default::default() + }, + ], + ..Default::default() + }], + }, + ) + .await; + + validate_result +} + +#[recorded::test] +//#[ignore = "Test does not currently work because instrumentation of PageIterators doesn't quite work."] async fn list_secrets_verify_telemetry_rehydrated(ctx: TestContext) -> Result<()> { use azure_core_test::tracing::ExpectedRestApiSpan; @@ -443,24 +561,25 @@ async fn list_secrets_verify_telemetry_rehydrated(ctx: TestContext) -> Result<() }, |client: SecretClient| { Box::pin(async move { - let mut first_pager = client.list_secret_properties(None)?.into_pages(); - - // Prime the iteration. - let first_page = first_pager - .try_next() - .await? - .expect("expected at least one page"); - { - let secrets = first_page.into_model()?; - for secret in secrets.value { - let _ = secret.resource_id()?; + let rehydration_token = { + let mut first_pager = client.list_secret_properties(None)?.into_pages(); + + // Prime the iteration. + let first_page = first_pager + .try_next() + .await? + .expect("expected at least one page"); + { + let secrets = first_page.into_model()?; + for secret in secrets.value { + let _ = secret.resource_id()?; + } } - } - - let rehydration_token = first_pager - .continuation_token() - .expect("expected continuation token to be created after first page"); + first_pager + .continuation_token() + .expect("expected continuation token to be created after first page") + }; let mut rehydrated_pager = client .list_secret_properties(None)? .into_pages() @@ -491,34 +610,32 @@ async fn list_secrets_verify_telemetry_rehydrated(ctx: TestContext) -> Result<() }, ExpectedApiInformation { api_name: Some("KeyVault.getSecrets"), - api_children: vec![ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }], - ..Default::default() - }, - ExpectedApiInformation { - api_name: Some("KeyVault.getSecrets"), - api_children: vec![ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }], - ..Default::default() - }, - ExpectedApiInformation { - api_name: Some("KeyVault.getSecrets"), - api_children: vec![ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }], - ..Default::default() - }, - ExpectedApiInformation { - api_name: Some("KeyVault.getSecrets"), - api_children: vec![ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }], + api_children: vec![ + ExpectedRestApiSpan { + api_verb: azure_core::http::Method::Get, + ..Default::default() + }, + ExpectedRestApiSpan { + api_verb: azure_core::http::Method::Get, + ..Default::default() + }, + ExpectedRestApiSpan { + api_verb: azure_core::http::Method::Get, + ..Default::default() + }, + ExpectedRestApiSpan { + api_verb: azure_core::http::Method::Get, + ..Default::default() + }, + ExpectedRestApiSpan { + api_verb: azure_core::http::Method::Get, + ..Default::default() + }, + ExpectedRestApiSpan { + api_verb: azure_core::http::Method::Get, + ..Default::default() + }, + ], ..Default::default() }, ], From de89a018a8e1c2a20eb4eb8e0c84e4e7530ce8b4 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Fri, 7 Nov 2025 17:07:32 -0800 Subject: [PATCH 02/13] Early PR feedback --- sdk/core/azure_core/src/http/pager.rs | 65 +++++++++++-------- .../tests/secret_client.rs | 1 - 2 files changed, 37 insertions(+), 29 deletions(-) diff --git a/sdk/core/azure_core/src/http/pager.rs b/sdk/core/azure_core/src/http/pager.rs index bc5518bfb1..d72f526be0 100644 --- a/sdk/core/azure_core/src/http/pager.rs +++ b/sdk/core/azure_core/src/http/pager.rs @@ -391,10 +391,10 @@ impl ItemIterator

{ /// ``` pub fn from_callback< // This is a bit gnarly, but the only thing that differs between the WASM/non-WASM configs is the presence of Send bounds. - #[cfg(not(target_arch = "wasm32"))] C: AsRef + FromStr + fmt::Debug + Send + 'static, + #[cfg(not(target_arch = "wasm32"))] C: AsRef + FromStr + Send + 'static, #[cfg(not(target_arch = "wasm32"))] F: Fn(PagerState, Context<'static>) -> Fut + Send + 'static, #[cfg(not(target_arch = "wasm32"))] Fut: Future>> + Send + 'static, - #[cfg(target_arch = "wasm32")] C: AsRef + FromStr + fmt::Debug + 'static, + #[cfg(target_arch = "wasm32")] C: AsRef + FromStr + 'static, #[cfg(target_arch = "wasm32")] F: Fn(PagerState, Context<'static>) -> Fut + 'static, #[cfg(target_arch = "wasm32")] Fut: Future>> + 'static, >( @@ -693,10 +693,10 @@ impl

PageIterator

{ /// ``` pub fn from_callback< // This is a bit gnarly, but the only thing that differs between the WASM/non-WASM configs is the presence of Send bounds. - #[cfg(not(target_arch = "wasm32"))] C: AsRef + FromStr + fmt::Debug + Send + 'static, + #[cfg(not(target_arch = "wasm32"))] C: AsRef + FromStr + Send + 'static, #[cfg(not(target_arch = "wasm32"))] F: Fn(PagerState, Context<'static>) -> Fut + Send + 'static, #[cfg(not(target_arch = "wasm32"))] Fut: Future>> + Send + 'static, - #[cfg(target_arch = "wasm32")] C: AsRef + FromStr + fmt::Debug + 'static, + #[cfg(target_arch = "wasm32")] C: AsRef + FromStr + 'static, #[cfg(target_arch = "wasm32")] F: Fn(PagerState, Context<'static>) -> Fut + 'static, #[cfg(target_arch = "wasm32")] Fut: Future>> + 'static, >( @@ -814,16 +814,29 @@ impl

fmt::Debug for PageIterator

{ #[derive(Debug, Clone, Eq)] enum State where - C: fmt::Debug, + C: AsRef, { Init, More(C), Done, } +impl AsRef for State +where + C: AsRef, +{ + fn as_ref(&self) -> &str { + match self { + State::Init => "Init", + State::More(c) => c.as_ref(), + State::Done => "Done", + } + } +} + impl PartialEq for State where - C: fmt::Debug, + C: AsRef, { fn eq(&self, other: &Self) -> bool { // Only needs to compare if both states are Init or Done; internally, we don't care about any other states. @@ -837,7 +850,7 @@ where #[derive(Debug)] struct StreamState<'a, C, F> where - C: fmt::Debug, + C: AsRef, { state: State, make_request: F, @@ -849,10 +862,10 @@ where fn iter_from_callback< P, // This is a bit gnarly, but the only thing that differs between the WASM/non-WASM configs is the presence of Send bounds. - #[cfg(not(target_arch = "wasm32"))] C: AsRef + fmt::Debug + FromStr + Send + 'static, + #[cfg(not(target_arch = "wasm32"))] C: AsRef + FromStr + Send + 'static, #[cfg(not(target_arch = "wasm32"))] F: Fn(PagerState, Context<'static>) -> Fut + Send + 'static, #[cfg(not(target_arch = "wasm32"))] Fut: Future>> + Send + 'static, - #[cfg(target_arch = "wasm32")] C: AsRef + FromStr + fmt::Debug + 'static, + #[cfg(target_arch = "wasm32")] C: AsRef + FromStr + 'static, #[cfg(target_arch = "wasm32")] F: Fn(PagerState, Context<'static>) -> Fut + 'static, #[cfg(target_arch = "wasm32")] Fut: Future>> + 'static, >( @@ -865,7 +878,7 @@ where { unfold( StreamState { - state: State::Init, + state: State::::Init, make_request, continuation_token, ctx, @@ -874,29 +887,13 @@ where |mut stream_state| async move { // Get the `continuation_token` to pick up where we left off, or None for the initial page, // but don't override the terminal `State::Done`. + tracing::trace!("pager state: {}", AsRef::::as_ref(&stream_state.state)); if stream_state.state != State::Done { let result = match stream_state.continuation_token.lock() { Ok(next_token) => match next_token.as_deref() { Some(n) => match n.parse() { - Ok(s) => { - // When resuming from a continuation token, create a span for the entire request, and attach it to the context. - if stream_state.state == State::Init { - tracing::debug!( - "resuming pager from continuation token: {:?}", - n - ); - - // At the very start of polling, create a span for the entire request, and attach it to the context - let span = - create_public_api_span(&stream_state.ctx, None, None); - if let Some(ref s) = span { - stream_state.added_span = true; - stream_state.ctx = stream_state.ctx.with_value(s.clone()); - } - } - Ok(State::More(s)) - } + Ok(s) => Ok(State::More(s)), Err(err) => Err(crate::Error::with_message_fn( ErrorKind::DataConversion, || format!("invalid continuation token: {err}"), @@ -910,6 +907,18 @@ where })), }; + // When resuming from a continuation token, create a span for the entire request, and attach it to the context. + if result.is_ok() && stream_state.state == State::Init { + tracing::debug!("resuming pager from continuation token, re-establish public API span for new pager."); + + // At the very start of polling, create a span for the entire request, and attach it to the context + let span = create_public_api_span(&stream_state.ctx, None, None); + if let Some(ref s) = span { + stream_state.added_span = true; + stream_state.ctx = stream_state.ctx.with_value(s.clone()); + } + } + match result { Ok(state) => stream_state.state = state, Err(err) => { diff --git a/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs b/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs index ec6f5cd39f..3599f1c6ff 100644 --- a/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs +++ b/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs @@ -508,7 +508,6 @@ async fn list_secrets_by_pages_verify_telemetry(ctx: TestContext) -> Result<()> } #[recorded::test] -//#[ignore = "Test does not currently work because instrumentation of PageIterators doesn't quite work."] async fn list_secrets_verify_telemetry_rehydrated(ctx: TestContext) -> Result<()> { use azure_core_test::tracing::ExpectedRestApiSpan; From 0c36c6795c2262b6c7a0f4a1e2d057e5cb329c8c Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Fri, 7 Nov 2025 17:09:54 -0800 Subject: [PATCH 03/13] Early PR feedback 2 --- sdk/core/azure_core/src/http/pager.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sdk/core/azure_core/src/http/pager.rs b/sdk/core/azure_core/src/http/pager.rs index d72f526be0..44959dfeed 100644 --- a/sdk/core/azure_core/src/http/pager.rs +++ b/sdk/core/azure_core/src/http/pager.rs @@ -878,7 +878,7 @@ where { unfold( StreamState { - state: State::::Init, + state: State::Init, make_request, continuation_token, ctx, @@ -887,7 +887,6 @@ where |mut stream_state| async move { // Get the `continuation_token` to pick up where we left off, or None for the initial page, // but don't override the terminal `State::Done`. - tracing::trace!("pager state: {}", AsRef::::as_ref(&stream_state.state)); if stream_state.state != State::Done { let result = match stream_state.continuation_token.lock() { From 3f8792d6df5542f7f0ce87007cd4237c39b3a592 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Fri, 7 Nov 2025 17:15:09 -0800 Subject: [PATCH 04/13] Debug instead of AsRef for State --- sdk/core/azure_core/src/http/pager.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/sdk/core/azure_core/src/http/pager.rs b/sdk/core/azure_core/src/http/pager.rs index 44959dfeed..b35c4a6008 100644 --- a/sdk/core/azure_core/src/http/pager.rs +++ b/sdk/core/azure_core/src/http/pager.rs @@ -811,7 +811,7 @@ impl

fmt::Debug for PageIterator

{ } } -#[derive(Debug, Clone, Eq)] +#[derive(Clone, Eq)] enum State where C: AsRef, @@ -821,15 +821,15 @@ where Done, } -impl AsRef for State +impl fmt::Debug for State where C: AsRef, { - fn as_ref(&self) -> &str { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - State::Init => "Init", - State::More(c) => c.as_ref(), - State::Done => "Done", + State::Init => write!(f, "State::Init"), + State::More(c) => write!(f, "State::More({})", c.as_ref()), + State::Done => write!(f, "State::Done"), } } } From 94f53cb2de0b5baf9268a638bdff1645ce35df62 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Mon, 10 Nov 2025 09:43:24 -0800 Subject: [PATCH 05/13] Use debug_struct to output continuation; sanitize continuation URL --- sdk/core/azure_core/src/http/mod.rs | 2 +- sdk/core/azure_core/src/http/pager.rs | 15 +++++++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/sdk/core/azure_core/src/http/mod.rs b/sdk/core/azure_core/src/http/mod.rs index 9ad5ce8009..465ab60825 100644 --- a/sdk/core/azure_core/src/http/mod.rs +++ b/sdk/core/azure_core/src/http/mod.rs @@ -23,7 +23,7 @@ pub use response::{AsyncRawResponse, AsyncResponse, RawResponse, Response}; pub use typespec_client_core::http::response; pub use typespec_client_core::http::{ new_http_client, AppendToUrlQuery, Context, DeserializeWith, Format, HttpClient, JsonFormat, - Method, NoFormat, StatusCode, Url, UrlExt, + Method, NoFormat, Sanitizer, StatusCode, Url, UrlExt, }; pub use crate::error::check_success; diff --git a/sdk/core/azure_core/src/http/pager.rs b/sdk/core/azure_core/src/http/pager.rs index b35c4a6008..8ca913ec1b 100644 --- a/sdk/core/azure_core/src/http/pager.rs +++ b/sdk/core/azure_core/src/http/pager.rs @@ -7,13 +7,14 @@ use crate::{ error::ErrorKind, http::{ headers::HeaderName, policies::create_public_api_span, response::Response, Context, - DeserializeWith, Format, JsonFormat, + DeserializeWith, Format, JsonFormat, Sanitizer, Url, }, tracing::{Span, SpanStatus}, }; use async_trait::async_trait; use futures::{stream::unfold, FutureExt, Stream}; use std::{ + collections::HashSet, fmt, future::Future, ops::Deref, @@ -828,7 +829,17 @@ where fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { State::Init => write!(f, "State::Init"), - State::More(c) => write!(f, "State::More({})", c.as_ref()), + State::More(c) => { + let continuation = if let Ok(url) = Url::parse(c.as_ref()) { + url.sanitize(&HashSet::new()).to_string() + } else { + c.as_ref().to_string() + }; + + f.debug_struct("State::More") + .field("continuation", &continuation) + .finish() + } State::Done => write!(f, "State::Done"), } } From 7d12773bac06d097968c2da78e4e2932fe9bda01 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Mon, 10 Nov 2025 10:55:59 -0800 Subject: [PATCH 06/13] Use default filtering rules, not 'none' --- sdk/core/azure_core/src/http/pager.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sdk/core/azure_core/src/http/pager.rs b/sdk/core/azure_core/src/http/pager.rs index 8ca913ec1b..5eec258b91 100644 --- a/sdk/core/azure_core/src/http/pager.rs +++ b/sdk/core/azure_core/src/http/pager.rs @@ -14,7 +14,6 @@ use crate::{ use async_trait::async_trait; use futures::{stream::unfold, FutureExt, Stream}; use std::{ - collections::HashSet, fmt, future::Future, ops::Deref, @@ -23,6 +22,7 @@ use std::{ sync::{Arc, Mutex}, task, }; +use typespec_client_core::http::DEFAULT_ALLOWED_QUERY_PARAMETERS; /// Represents the state of a [`Pager`] or [`PageIterator`]. #[derive(Debug, Default, PartialEq, Eq)] @@ -831,7 +831,7 @@ where State::Init => write!(f, "State::Init"), State::More(c) => { let continuation = if let Ok(url) = Url::parse(c.as_ref()) { - url.sanitize(&HashSet::new()).to_string() + url.sanitize(&DEFAULT_ALLOWED_QUERY_PARAMETERS).to_string() } else { c.as_ref().to_string() }; @@ -898,6 +898,7 @@ where |mut stream_state| async move { // Get the `continuation_token` to pick up where we left off, or None for the initial page, // but don't override the terminal `State::Done`. + tracing::trace!("current stream state: {:?}", stream_state.state); if stream_state.state != State::Done { let result = match stream_state.continuation_token.lock() { From 413449f8e89c8abeaeb27249043ce766d57874cc Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Mon, 10 Nov 2025 11:58:18 -0800 Subject: [PATCH 07/13] Establish tracer only once; removed sanitization --- sdk/core/azure_core/src/http/pager.rs | 51 ++++++++++----------------- 1 file changed, 18 insertions(+), 33 deletions(-) diff --git a/sdk/core/azure_core/src/http/pager.rs b/sdk/core/azure_core/src/http/pager.rs index 5eec258b91..415a655350 100644 --- a/sdk/core/azure_core/src/http/pager.rs +++ b/sdk/core/azure_core/src/http/pager.rs @@ -7,7 +7,7 @@ use crate::{ error::ErrorKind, http::{ headers::HeaderName, policies::create_public_api_span, response::Response, Context, - DeserializeWith, Format, JsonFormat, Sanitizer, Url, + DeserializeWith, Format, JsonFormat, }, tracing::{Span, SpanStatus}, }; @@ -22,7 +22,6 @@ use std::{ sync::{Arc, Mutex}, task, }; -use typespec_client_core::http::DEFAULT_ALLOWED_QUERY_PARAMETERS; /// Represents the state of a [`Pager`] or [`PageIterator`]. #[derive(Debug, Default, PartialEq, Eq)] @@ -829,17 +828,10 @@ where fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { State::Init => write!(f, "State::Init"), - State::More(c) => { - let continuation = if let Ok(url) = Url::parse(c.as_ref()) { - url.sanitize(&DEFAULT_ALLOWED_QUERY_PARAMETERS).to_string() - } else { - c.as_ref().to_string() - }; - - f.debug_struct("State::More") - .field("continuation", &continuation) - .finish() - } + State::More(c) => f + .debug_struct("State::More") + .field("continuation", &c.as_ref()) + .finish(), State::Done => write!(f, "State::Done"), } } @@ -896,10 +888,21 @@ where added_span: false, }, |mut stream_state| async move { + // When in the "Init" state, we are either starting fresh or resuming from a continuation token. In either case, + // attach a span to the context for the entire paging operation. + if stream_state.state == State::Init { + tracing::debug!("establish a public API span for new pager."); + + // At the very start of polling, create a span for the entire request, and attach it to the context + let span = create_public_api_span(&stream_state.ctx, None, None); + if let Some(ref s) = span { + stream_state.added_span = true; + stream_state.ctx = stream_state.ctx.with_value(s.clone()); + } + } + // Get the `continuation_token` to pick up where we left off, or None for the initial page, // but don't override the terminal `State::Done`. - tracing::trace!("current stream state: {:?}", stream_state.state); - if stream_state.state != State::Done { let result = match stream_state.continuation_token.lock() { Ok(next_token) => match next_token.as_deref() { @@ -918,18 +921,6 @@ where })), }; - // When resuming from a continuation token, create a span for the entire request, and attach it to the context. - if result.is_ok() && stream_state.state == State::Init { - tracing::debug!("resuming pager from continuation token, re-establish public API span for new pager."); - - // At the very start of polling, create a span for the entire request, and attach it to the context - let span = create_public_api_span(&stream_state.ctx, None, None); - if let Some(ref s) = span { - stream_state.added_span = true; - stream_state.ctx = stream_state.ctx.with_value(s.clone()); - } - } - match result { Ok(state) => stream_state.state = state, Err(err) => { @@ -941,12 +932,6 @@ where let result = match stream_state.state { State::Init => { tracing::debug!("initial page request"); - // At the very start of polling, create a span for the entire request, and attach it to the context - let span = create_public_api_span(&stream_state.ctx, None, None); - if let Some(ref s) = span { - stream_state.added_span = true; - stream_state.ctx = stream_state.ctx.with_value(s.clone()); - } (stream_state.make_request)(PagerState::Initial, stream_state.ctx.clone()).await } State::More(n) => { From 0f4aa33e6018cc86922159c775061fb169751168 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Mon, 10 Nov 2025 15:01:38 -0800 Subject: [PATCH 08/13] Added warnings about using recording.var --- sdk/core/azure_core_test/src/tracing.rs | 7 +++++++ .../tests/secret_client.rs | 20 ++++++++++++------- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/sdk/core/azure_core_test/src/tracing.rs b/sdk/core/azure_core_test/src/tracing.rs index 244a17d838..78b0bb9e15 100644 --- a/sdk/core/azure_core_test/src/tracing.rs +++ b/sdk/core/azure_core_test/src/tracing.rs @@ -470,8 +470,14 @@ impl Default for ExpectedRestApiSpan { #[derive(Debug, Default, Clone)] pub struct ExpectedInstrumentation { /// The package name for the service client. + /// + /// **NOTE**: Make sure that the package name comes from `env!("CARGO_PKG_NAME")` to ensure that this continues to work + /// if test recordings were created with a previous version of the package. pub package_name: String, /// The package version for the service client. + /// + /// **NOTE**: Make sure that the package name comes from `env!("CARGO_PKG_VERSION")` to ensure that this continues to work + /// if test recordings were created with a previous version of the package. pub package_version: String, /// The namespace for the service client. pub package_namespace: Option<&'static str>, @@ -502,6 +508,7 @@ pub struct ExpectedInstrumentation { /// The `test_api` call may issue multiple service client calls, if it does, this function will verify that all expected spans were created. The caller of the `test_instrumentation_for_api` call /// should make sure to include all expected APIs in the call. /// +/// pub async fn assert_instrumentation_information( create_client: FnInit, test_api: FnTest, diff --git a/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs b/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs index 3599f1c6ff..02a53fcae8 100644 --- a/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs +++ b/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs @@ -267,7 +267,10 @@ async fn round_trip_secret_verify_telemetry(ctx: TestContext) -> Result<()> { }) }, ExpectedInstrumentation { - package_name: recording.var("CARGO_PKG_NAME", None), + // Don't use `recording.var` here in case the recording was made with a different package version. + package_name: env!("CARGO_PKG_NAME").into(), + + // Don't use `recording.var` here in case the recording was made with a different package version package_version: env!("CARGO_PKG_VERSION").into(), package_namespace: Some("KeyVault"), api_calls: vec![ @@ -354,8 +357,9 @@ async fn list_secrets_verify_telemetry(ctx: TestContext) -> Result<()> { }) }, ExpectedInstrumentation { - package_name: recording.var("CARGO_PKG_NAME", None), - package_version: recording.var("CARGO_PKG_VERSION", None), + // Don't use `recording.var` here in case the recording was made with a different package version + package_name: env!("CARGO_PKG_NAME").into(), + package_version: env!("CARGO_PKG_VERSION").into(), package_namespace: Some("KeyVault"), api_calls: vec![ExpectedApiInformation { api_name: Some("KeyVault.getSecrets"), @@ -463,8 +467,9 @@ async fn list_secrets_by_pages_verify_telemetry(ctx: TestContext) -> Result<()> }) }, ExpectedInstrumentation { - package_name: recording.var("CARGO_PKG_NAME", None), - package_version: recording.var("CARGO_PKG_VERSION", None), + // Don't use `recording.var` here in case the recording was made with a different package version + package_name: env!("CARGO_PKG_NAME").into(), + package_version: env!("CARGO_PKG_VERSION").into(), package_namespace: Some("KeyVault"), api_calls: vec![ExpectedApiInformation { api_name: Some("KeyVault.getSecrets"), @@ -595,8 +600,9 @@ async fn list_secrets_verify_telemetry_rehydrated(ctx: TestContext) -> Result<() }) }, ExpectedInstrumentation { - package_name: recording.var("CARGO_PKG_NAME", None), - package_version: recording.var("CARGO_PKG_VERSION", None), + // Don't use `recording.var` here in case the recording was made with a different package version + package_name: env!("CARGO_PKG_NAME").into(), + package_version: env!("CARGO_PKG_VERSION").into(), package_namespace: Some("KeyVault"), api_calls: vec![ ExpectedApiInformation { From a55dfe1a03b4fc927046a03b94d16fd419a71e31 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Tue, 11 Nov 2025 14:01:26 -0800 Subject: [PATCH 09/13] Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- sdk/core/azure_core_test/src/tracing.rs | 2 +- .../tests/secret_client.rs | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/sdk/core/azure_core_test/src/tracing.rs b/sdk/core/azure_core_test/src/tracing.rs index 78b0bb9e15..9e565e6f2b 100644 --- a/sdk/core/azure_core_test/src/tracing.rs +++ b/sdk/core/azure_core_test/src/tracing.rs @@ -476,7 +476,7 @@ pub struct ExpectedInstrumentation { pub package_name: String, /// The package version for the service client. /// - /// **NOTE**: Make sure that the package name comes from `env!("CARGO_PKG_VERSION")` to ensure that this continues to work + /// **NOTE**: Make sure that the package version comes from `env!("CARGO_PKG_VERSION")` to ensure that this continues to work /// if test recordings were created with a previous version of the package. pub package_version: String, /// The namespace for the service client. diff --git a/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs b/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs index fed8a9506f..3918837562 100644 --- a/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs +++ b/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs @@ -270,7 +270,7 @@ async fn round_trip_secret_verify_telemetry(ctx: TestContext) -> Result<()> { // Don't use `recording.var` here in case the recording was made with a different package version. package_name: env!("CARGO_PKG_NAME").into(), - // Don't use `recording.var` here in case the recording was made with a different package version + // Don't use `recording.var` here in case the recording was made with a different package version. package_version: env!("CARGO_PKG_VERSION").into(), package_namespace: Some("KeyVault"), api_calls: vec![ @@ -350,7 +350,7 @@ async fn list_secrets_verify_telemetry(ctx: TestContext) -> Result<()> { let mut secrets = client.list_secret_properties(None)?; while let Some(secret) = secrets.try_next().await? { let _ = secret.resource_id()?; - println!("Found secret: {}", secret.resource_id()?.name); + } Ok(()) @@ -358,7 +358,7 @@ async fn list_secrets_verify_telemetry(ctx: TestContext) -> Result<()> { }, ExpectedInstrumentation { package_name: recording.var("CARGO_PKG_NAME", None), - // Don't use `recording.var` here in case the recording was made with a different package version + // Don't use `recording.var` here in case the recording was made with a different package version. package_version: env!("CARGO_PKG_VERSION").into(), package_namespace: Some("KeyVault"), api_calls: vec![ExpectedApiInformation { @@ -467,7 +467,7 @@ async fn list_secrets_by_pages_verify_telemetry(ctx: TestContext) -> Result<()> }) }, ExpectedInstrumentation { - // Don't use `recording.var` here in case the recording was made with a different package version + // Don't use `recording.var` here in case the recording was made with a different package version. package_name: env!("CARGO_PKG_NAME").into(), package_version: env!("CARGO_PKG_VERSION").into(), package_namespace: Some("KeyVault"), @@ -600,7 +600,7 @@ async fn list_secrets_verify_telemetry_rehydrated(ctx: TestContext) -> Result<() }) }, ExpectedInstrumentation { - // Don't use `recording.var` here in case the recording was made with a different package version + // Don't use `recording.var` here in case the recording was made with a different package version. package_name: env!("CARGO_PKG_NAME").into(), package_version: env!("CARGO_PKG_VERSION").into(), package_namespace: Some("KeyVault"), From ed0b8710371d9c2bc6f16b8a258561b9bd59c806 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Tue, 11 Nov 2025 16:19:19 -0800 Subject: [PATCH 10/13] Cargo fmt fix --- .../azure_security_keyvault_secrets/tests/secret_client.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs b/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs index 3918837562..06b27417b3 100644 --- a/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs +++ b/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs @@ -350,7 +350,6 @@ async fn list_secrets_verify_telemetry(ctx: TestContext) -> Result<()> { let mut secrets = client.list_secret_properties(None)?; while let Some(secret) = secrets.try_next().await? { let _ = secret.resource_id()?; - } Ok(()) From 5ed67a4468bef62f2109ebe392377cf0ed107eb2 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Wed, 12 Nov 2025 14:48:19 -0800 Subject: [PATCH 11/13] Enable wildcards in telemetry tests to account for pagers (and pollers) that return a non-deterministic number of responses --- .../public_api_instrumentation.rs | 8 +- .../request_instrumentation.rs | 4 + .../tests/telemetry_service_macros.rs | 15 +- sdk/core/azure_core_test/src/tracing.rs | 134 +++++++++++-- .../tests/certificate_client.rs | 41 ++-- .../tests/secret_client.rs | 189 ++++++------------ 6 files changed, 208 insertions(+), 183 deletions(-) diff --git a/sdk/core/azure_core/src/http/policies/instrumentation/public_api_instrumentation.rs b/sdk/core/azure_core/src/http/policies/instrumentation/public_api_instrumentation.rs index 3c9abbfb15..a577cd1ae0 100644 --- a/sdk/core/azure_core/src/http/policies/instrumentation/public_api_instrumentation.rs +++ b/sdk/core/azure_core/src/http/policies/instrumentation/public_api_instrumentation.rs @@ -555,8 +555,7 @@ mod tests { status: SpanStatus::Unset, kind: SpanKind::Internal, span_id: Uuid::new_v4(), - parent_id: None, - attributes: vec![], + ..Default::default() }], }], ) @@ -599,9 +598,9 @@ mod tests { span_name: "MyClient.MyApi", status: SpanStatus::Unset, span_id: Uuid::new_v4(), - parent_id: None, kind: SpanKind::Internal, attributes: vec![(AZ_NAMESPACE_ATTRIBUTE, "test namespace".into())], + ..Default::default() }], }], ); @@ -652,6 +651,7 @@ mod tests { (AZ_NAMESPACE_ATTRIBUTE, "test namespace".into()), (ERROR_TYPE_ATTRIBUTE, "500".into()), ], + ..Default::default() }], }], ); @@ -701,6 +701,7 @@ mod tests { (AZ_NAMESPACE_ATTRIBUTE, "test.namespace".into()), ("az.fake_attribute", "attribute value".into()), ], + ..Default::default() }, ExpectedSpanInformation { span_name: "PUT", @@ -716,6 +717,7 @@ mod tests { ("server.port", 80.into()), ("http.response.status_code", 200.into()), ], + ..Default::default() }, ], }], diff --git a/sdk/core/azure_core/src/http/policies/instrumentation/request_instrumentation.rs b/sdk/core/azure_core/src/http/policies/instrumentation/request_instrumentation.rs index 442f5cdab3..d221ff0eaf 100644 --- a/sdk/core/azure_core/src/http/policies/instrumentation/request_instrumentation.rs +++ b/sdk/core/azure_core/src/http/policies/instrumentation/request_instrumentation.rs @@ -302,6 +302,7 @@ pub(crate) mod tests { ), ), ], + ..Default::default() }], }], ); @@ -388,6 +389,7 @@ pub(crate) mod tests { AttributeValue::from("https://example.com/client_request_id"), ), ], + ..Default::default() }], }], ); @@ -433,6 +435,7 @@ pub(crate) mod tests { (SERVER_ADDRESS_ATTRIBUTE, AttributeValue::from("host")), (SERVER_PORT_ATTRIBUTE, AttributeValue::from(8080)), ], + ..Default::default() }], }], ); @@ -502,6 +505,7 @@ pub(crate) mod tests { AttributeValue::from("https://microsoft.com/request_failed.htm"), ), ], + ..Default::default() }], }], ); diff --git a/sdk/core/azure_core_opentelemetry/tests/telemetry_service_macros.rs b/sdk/core/azure_core_opentelemetry/tests/telemetry_service_macros.rs index 4f5919a565..0179d0de63 100644 --- a/sdk/core/azure_core_opentelemetry/tests/telemetry_service_macros.rs +++ b/sdk/core/azure_core_opentelemetry/tests/telemetry_service_macros.rs @@ -685,10 +685,7 @@ mod tests { let package_version = env!("CARGO_PKG_VERSION").to_string(); azure_core_test::tracing::assert_instrumentation_information( |tracer_provider| Ok(create_service_client(&ctx, tracer_provider)), - |client| { - let client = client; - Box::pin(async move { client.get("get", None).await }) - }, + async move |client| client.get("get", None).await, ExpectedInstrumentation { package_name, package_version, @@ -710,10 +707,7 @@ mod tests { let package_version = env!("CARGO_PKG_VERSION").to_string(); azure_core_test::tracing::assert_instrumentation_information( |tracer_provider| Ok(create_service_client(&ctx, tracer_provider)), - |client| { - let client = client; - Box::pin(async move { client.get_with_function_tracing("get", None).await }) - }, + async move |client| client.get_with_function_tracing("get", None).await, ExpectedInstrumentation { package_name, package_version, @@ -741,10 +735,7 @@ mod tests { let package_version = env!("CARGO_PKG_VERSION").to_string(); azure_core_test::tracing::assert_instrumentation_information( |tracer_provider| Ok(create_service_client(&ctx, tracer_provider)), - |client| { - let client = client; - Box::pin(async move { client.get_with_function_tracing("index.htm", None).await }) - }, + async move |client| client.get_with_function_tracing("index.htm", None).await, ExpectedInstrumentation { package_name, package_version, diff --git a/sdk/core/azure_core_test/src/tracing.rs b/sdk/core/azure_core_test/src/tracing.rs index 9e565e6f2b..19db29b9a9 100644 --- a/sdk/core/azure_core_test/src/tracing.rs +++ b/sdk/core/azure_core_test/src/tracing.rs @@ -18,7 +18,6 @@ use std::{ borrow::Cow, collections::HashMap, fmt::Debug, - pin::Pin, sync::{Arc, Mutex}, }; @@ -317,21 +316,50 @@ pub fn check_instrumentation_result( assert_eq!(tracer.namespace, expected.namespace); let spans = tracer.spans.lock().unwrap(); - assert_eq!( - spans.len(), - expected.spans.len(), - "Unexpected number of spans for tracer {}", - expected.name - ); - for (span_index, span_expected) in expected.spans.iter().enumerate() { + // assert_eq!( + // spans.len(), + // expected.spans.len(), + // "Unexpected number of spans for tracer {}", + // expected.name + // ); + + let mut expected_index = 0; + for (span_index, span_actual) in spans.iter().enumerate() { eprintln!( "Checking span {} of tracer {}: {}", - span_index, expected.name, span_expected.span_name + span_index, expected.name, span_actual.name + ); + check_span_information( + span_actual, + &expected.spans[expected_index], + &parent_span_map, ); - check_span_information(&spans[span_index], span_expected, &parent_span_map); // Now that we've verified the span, add the mapping between expected span ID and the actual span ID. - parent_span_map.insert(span_expected.span_id, spans[span_index].id); + parent_span_map.insert(expected.spans[expected_index].span_id, span_actual.id); + if expected.spans[expected_index].is_wildcard { + // If this is a wildcard span, we don't increment the expected index. + eprintln!( + "Span {} is a wildcard, not incrementing expected index", + span_actual.name + ); + if spans.len() > span_index + 1 { + let next_span = &spans[span_index + 1]; + if !compare_span_information( + next_span, + &expected.spans[expected_index], + &parent_span_map, + ) { + eprintln!( + "Next actual span does not match expected span: {}", + expected.spans[expected_index + 1].span_name + ); + expected_index += 1; + } + } + } else { + expected_index += 1; + } } } } @@ -355,6 +383,22 @@ pub struct ExpectedSpanInformation<'a> { /// The expected attributes associated with the span. pub attributes: Vec<(&'a str, AttributeValue)>, + + pub is_wildcard: bool, +} + +impl Default for ExpectedSpanInformation<'_> { + fn default() -> Self { + Self { + span_name: "get", + status: SpanStatus::Unset, + span_id: Uuid::new_v4(), + parent_id: None, + kind: SpanKind::Client, + attributes: vec![], + is_wildcard: false, + } + } } fn check_span_information( @@ -407,6 +451,64 @@ fn check_span_information( ); } +/// Returns true if the spans match, false otherwise. +fn compare_span_information( + actual: &Arc, + expected: &ExpectedSpanInformation<'_>, + parent_span_map: &HashMap, +) -> bool { + if actual.name != expected.span_name { + return false; + } + if actual.kind != expected.kind { + return false; + } + if *actual.state.lock().unwrap() != expected.status { + return false; + } + match actual.parent { + None => { + if expected.parent_id.is_some() { + return false; + } + } + Some(ref parent) => { + let parent_id = parent_span_map + .get(expected.parent_id.as_ref().unwrap()) + .unwrap(); + if *parent != *parent_id { + return false; + } + } + } + let attributes = actual.attributes.lock().unwrap(); + eprintln!("Expected attributes: {:?}", expected.attributes); + eprintln!("Found attributes: {:?}", attributes); + for (index, attr) in attributes.iter().enumerate() { + eprintln!("Attribute {}: {} = {:?}", index, attr.key, attr.value); + let mut found = false; + for (key, value) in &expected.attributes { + if attr.key == *key { + // Skip checking the value for "" as it is a placeholder + if *value != AttributeValue::String("".into()) && attr.value != *value { + return false; + } + found = true; + break; + } + } + if !found { + return false; + } + } + for (key, _) in expected.attributes.iter() { + if !attributes.iter().any(|attr| attr.key == *key) { + return false; + } + } + true +} + /// Information about an instrumented API call. /// /// This structure is used to collect information about a specific API call that is being instrumented for tracing. @@ -455,6 +557,9 @@ pub struct ExpectedRestApiSpan { /// Expected status code returned by the service. pub expected_status_code: azure_core::http::StatusCode, + + /// Whether an unknown multiple of this span will be found. + pub is_wildcard: bool, } impl Default for ExpectedRestApiSpan { @@ -462,6 +567,7 @@ impl Default for ExpectedRestApiSpan { Self { api_verb: azure_core::http::Method::Get, expected_status_code: azure_core::http::StatusCode::Ok, + is_wildcard: false, } } } @@ -516,7 +622,7 @@ pub async fn assert_instrumentation_information( ) -> azure_core::Result<()> where FnInit: FnOnce(Arc) -> azure_core::Result, - FnTest: FnOnce(C) -> Pin>>>, + FnTest: AsyncFnOnce(C) -> azure_core::Result, { // Initialize the mock tracer provider let mock_tracer = Arc::new(MockTracingProvider::new()); @@ -584,6 +690,7 @@ where status: span_status, kind: SpanKind::Internal, parent_id: None, + is_wildcard: false, // Public API spans cannot be wildcards. attributes: public_api_attributes, }); } @@ -626,6 +733,9 @@ where } else { None }, + // If allow_unknown_children is set, we don't know how many child spans there will be. + // Use a wildcard span ID to indicate that. + is_wildcard: rest_api_call.is_wildcard, span_id: Uuid::new_v4(), status: if !rest_api_call.expected_status_code.is_success() { SpanStatus::Error { diff --git a/sdk/keyvault/azure_security_keyvault_certificates/tests/certificate_client.rs b/sdk/keyvault/azure_security_keyvault_certificates/tests/certificate_client.rs index 4e33ce9273..44fcaf338b 100644 --- a/sdk/keyvault/azure_security_keyvault_certificates/tests/certificate_client.rs +++ b/sdk/keyvault/azure_security_keyvault_certificates/tests/certificate_client.rs @@ -25,7 +25,7 @@ use azure_security_keyvault_keys::{ KeyClient, KeyClientOptions, }; use azure_security_keyvault_test::Retry; -use futures::{FutureExt, TryStreamExt}; +use futures::TryStreamExt; use openssl::sha::sha256; use std::{collections::HashMap, sync::LazyLock}; @@ -93,24 +93,21 @@ async fn certificate_validate_instrumentation(ctx: TestContext) -> Result<()> { )?; Ok(client) }, - |client| { - async move { - // Create a self-signed certificate. - let body = CreateCertificateParameters { - certificate_policy: Some(DEFAULT_CERTIFICATE_POLICY.clone()), - ..Default::default() - }; - let _certificate = client - .create_certificate( - "certificate-validate-instrumentation", - body.try_into()?, - None, - )? - .await? - .into_model()?; - Ok(()) - } - .boxed() + async move |client| { + // Create a self-signed certificate. + let body = CreateCertificateParameters { + certificate_policy: Some(DEFAULT_CERTIFICATE_POLICY.clone()), + ..Default::default() + }; + let _certificate = client + .create_certificate( + "certificate-validate-instrumentation", + body.try_into()?, + None, + )? + .await? + .into_model()?; + Ok(()) }, ExpectedInstrumentation { package_name: recording.var("CARGO_PKG_NAME", None), @@ -122,14 +119,12 @@ async fn certificate_validate_instrumentation(ctx: TestContext) -> Result<()> { ExpectedRestApiSpan { api_verb: Method::Post, expected_status_code: StatusCode::Accepted, + is_wildcard: false, }, ExpectedRestApiSpan { api_verb: Method::Get, expected_status_code: StatusCode::Ok, - }, - ExpectedRestApiSpan { - api_verb: Method::Get, - expected_status_code: StatusCode::Ok, + is_wildcard: true, }, ], ..Default::default() diff --git a/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs b/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs index 06b27417b3..887cec0d1c 100644 --- a/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs +++ b/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs @@ -345,15 +345,13 @@ async fn list_secrets_verify_telemetry(ctx: TestContext) -> Result<()> { Some(options), ) }, - |client: SecretClient| { - Box::pin(async move { - let mut secrets = client.list_secret_properties(None)?; - while let Some(secret) = secrets.try_next().await? { - let _ = secret.resource_id()?; - } + async move |client: SecretClient| { + let mut secrets = client.list_secret_properties(None)?; + while let Some(secret) = secrets.try_next().await? { + let _ = secret.resource_id()?; + } - Ok(()) - }) + Ok(()) }, ExpectedInstrumentation { package_name: recording.var("CARGO_PKG_NAME", None), @@ -362,36 +360,11 @@ async fn list_secrets_verify_telemetry(ctx: TestContext) -> Result<()> { package_namespace: Some("KeyVault"), api_calls: vec![ExpectedApiInformation { api_name: Some("KeyVault.getSecrets"), - api_children: vec![ - ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }, - ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }, - ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }, - ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }, - ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }, - ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }, - ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }, - ], + api_children: vec![ExpectedRestApiSpan { + api_verb: azure_core::http::Method::Get, + is_wildcard: true, + ..Default::default() + }], ..Default::default() }], }, @@ -452,18 +425,16 @@ async fn list_secrets_by_pages_verify_telemetry(ctx: TestContext) -> Result<()> Some(options), ) }, - |client: SecretClient| { - Box::pin(async move { - let mut secrets = client.list_secret_properties(None)?.into_pages(); - while let Some(page) = secrets.try_next().await? { - let items = page.into_model()?; - for item in items.value { - let _ = item.resource_id()?; - } + async move |client: SecretClient| { + let mut secrets = client.list_secret_properties(None)?.into_pages(); + while let Some(page) = secrets.try_next().await? { + let items = page.into_model()?; + for item in items.value { + let _ = item.resource_id()?; } + } - Ok(()) - }) + Ok(()) }, ExpectedInstrumentation { // Don't use `recording.var` here in case the recording was made with a different package version. @@ -472,36 +443,11 @@ async fn list_secrets_by_pages_verify_telemetry(ctx: TestContext) -> Result<()> package_namespace: Some("KeyVault"), api_calls: vec![ExpectedApiInformation { api_name: Some("KeyVault.getSecrets"), - api_children: vec![ - ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }, - ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }, - ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }, - ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }, - ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }, - ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }, - ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }, - ], + api_children: vec![ExpectedRestApiSpan { + api_verb: azure_core::http::Method::Get, + is_wildcard: true, + ..Default::default() + }], ..Default::default() }], }, @@ -562,41 +508,39 @@ async fn list_secrets_verify_telemetry_rehydrated(ctx: TestContext) -> Result<() Some(options), ) }, - |client: SecretClient| { - Box::pin(async move { - let rehydration_token = { - let mut first_pager = client.list_secret_properties(None)?.into_pages(); - - // Prime the iteration. - let first_page = first_pager - .try_next() - .await? - .expect("expected at least one page"); - { - let secrets = first_page.into_model()?; - for secret in secrets.value { - let _ = secret.resource_id()?; - } - } + async move |client: SecretClient| { + let rehydration_token = { + let mut first_pager = client.list_secret_properties(None)?.into_pages(); - first_pager - .continuation_token() - .expect("expected continuation token to be created after first page") - }; - let mut rehydrated_pager = client - .list_secret_properties(None)? - .into_pages() - .with_continuation_token(rehydration_token); - - while let Some(secret_page) = rehydrated_pager.try_next().await? { - let secrets = secret_page.into_model()?; + // Prime the iteration. + let first_page = first_pager + .try_next() + .await? + .expect("expected at least one page"); + { + let secrets = first_page.into_model()?; for secret in secrets.value { let _ = secret.resource_id()?; } } - Ok(()) - }) + first_pager + .continuation_token() + .expect("expected continuation token to be created after first page") + }; + let mut rehydrated_pager = client + .list_secret_properties(None)? + .into_pages() + .with_continuation_token(rehydration_token); + + while let Some(secret_page) = rehydrated_pager.try_next().await? { + let secrets = secret_page.into_model()?; + for secret in secrets.value { + let _ = secret.resource_id()?; + } + } + + Ok(()) }, ExpectedInstrumentation { // Don't use `recording.var` here in case the recording was made with a different package version. @@ -614,32 +558,11 @@ async fn list_secrets_verify_telemetry_rehydrated(ctx: TestContext) -> Result<() }, ExpectedApiInformation { api_name: Some("KeyVault.getSecrets"), - api_children: vec![ - ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }, - ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }, - ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }, - ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }, - ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }, - ExpectedRestApiSpan { - api_verb: azure_core::http::Method::Get, - ..Default::default() - }, - ], + api_children: vec![ExpectedRestApiSpan { + api_verb: azure_core::http::Method::Get, + is_wildcard: true, + ..Default::default() + }], ..Default::default() }, ], From f4cd80cb59219b4e4e7f80ac924a16601f8dd4ec Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Wed, 12 Nov 2025 15:31:46 -0800 Subject: [PATCH 12/13] PR feedback --- .../tests/telemetry_service_macros.rs | 4 ++-- .../azure_security_keyvault_secrets/tests/secret_client.rs | 7 +++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/sdk/core/azure_core_opentelemetry/tests/telemetry_service_macros.rs b/sdk/core/azure_core_opentelemetry/tests/telemetry_service_macros.rs index 0179d0de63..0fa1ff590f 100644 --- a/sdk/core/azure_core_opentelemetry/tests/telemetry_service_macros.rs +++ b/sdk/core/azure_core_opentelemetry/tests/telemetry_service_macros.rs @@ -703,7 +703,7 @@ mod tests { #[recorded::test()] async fn test_function_tracing_tests(ctx: TestContext) -> Result<()> { - let package_name = env!("CARGO_PKG_NAME").to_string(); + let package_name = ctx.recording().var("CARGO_PKG_NAME", None).to_string(); let package_version = env!("CARGO_PKG_VERSION").to_string(); azure_core_test::tracing::assert_instrumentation_information( |tracer_provider| Ok(create_service_client(&ctx, tracer_provider)), @@ -731,7 +731,7 @@ mod tests { async fn test_function_tracing_tests_error(ctx: TestContext) -> Result<()> { use azure_core_test::tracing::ExpectedRestApiSpan; - let package_name = env!("CARGO_PKG_NAME").to_string(); + let package_name = ctx.recording().var("CARGO_PKG_NAME", None).to_string(); let package_version = env!("CARGO_PKG_VERSION").to_string(); azure_core_test::tracing::assert_instrumentation_information( |tracer_provider| Ok(create_service_client(&ctx, tracer_provider)), diff --git a/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs b/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs index 887cec0d1c..7d24a67276 100644 --- a/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs +++ b/sdk/keyvault/azure_security_keyvault_secrets/tests/secret_client.rs @@ -267,8 +267,7 @@ async fn round_trip_secret_verify_telemetry(ctx: TestContext) -> Result<()> { }) }, ExpectedInstrumentation { - // Don't use `recording.var` here in case the recording was made with a different package version. - package_name: env!("CARGO_PKG_NAME").into(), + package_name: recording.var("CARGO_PKG_NAME", None), // Don't use `recording.var` here in case the recording was made with a different package version. package_version: env!("CARGO_PKG_VERSION").into(), @@ -437,8 +436,8 @@ async fn list_secrets_by_pages_verify_telemetry(ctx: TestContext) -> Result<()> Ok(()) }, ExpectedInstrumentation { + package_name: recording.var("CARGO_PKG_NAME", None), // Don't use `recording.var` here in case the recording was made with a different package version. - package_name: env!("CARGO_PKG_NAME").into(), package_version: env!("CARGO_PKG_VERSION").into(), package_namespace: Some("KeyVault"), api_calls: vec![ExpectedApiInformation { @@ -543,8 +542,8 @@ async fn list_secrets_verify_telemetry_rehydrated(ctx: TestContext) -> Result<() Ok(()) }, ExpectedInstrumentation { + package_name: recording.var("CARGO_PKG_NAME", None), // Don't use `recording.var` here in case the recording was made with a different package version. - package_name: env!("CARGO_PKG_NAME").into(), package_version: env!("CARGO_PKG_VERSION").into(), package_namespace: Some("KeyVault"), api_calls: vec![ From f25dd5dcafaaa1e1e90ac7b4ac1b087d9c1d9416 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Wed, 12 Nov 2025 16:04:23 -0800 Subject: [PATCH 13/13] PR feedback --- sdk/core/azure_core/src/http/pager.rs | 9 +++------ sdk/core/azure_core_test/src/tracing.rs | 27 ++++++++++++++++++------- 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/sdk/core/azure_core/src/http/pager.rs b/sdk/core/azure_core/src/http/pager.rs index 415a655350..719ff79d93 100644 --- a/sdk/core/azure_core/src/http/pager.rs +++ b/sdk/core/azure_core/src/http/pager.rs @@ -827,12 +827,9 @@ where { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - State::Init => write!(f, "State::Init"), - State::More(c) => f - .debug_struct("State::More") - .field("continuation", &c.as_ref()) - .finish(), - State::Done => write!(f, "State::Done"), + State::Init => write!(f, "Init"), + State::More(c) => f.debug_tuple("More").field(&c.as_ref()).finish(), + State::Done => write!(f, "Done"), } } } diff --git a/sdk/core/azure_core_test/src/tracing.rs b/sdk/core/azure_core_test/src/tracing.rs index 19db29b9a9..484ed5823c 100644 --- a/sdk/core/azure_core_test/src/tracing.rs +++ b/sdk/core/azure_core_test/src/tracing.rs @@ -317,12 +317,15 @@ pub fn check_instrumentation_result( let spans = tracer.spans.lock().unwrap(); - // assert_eq!( - // spans.len(), - // expected.spans.len(), - // "Unexpected number of spans for tracer {}", - // expected.name - // ); + // Check span lengths if there are no wildcard spans. + if !expected.spans.iter().any(|s| s.is_wildcard) { + assert_eq!( + spans.len(), + expected.spans.len(), + "Unexpected number of spans for tracer {}", + expected.name + ); + } let mut expected_index = 0; for (span_index, span_actual) in spans.iter().enumerate() { @@ -352,15 +355,25 @@ pub fn check_instrumentation_result( ) { eprintln!( "Next actual span does not match expected span: {}", - expected.spans[expected_index + 1].span_name + expected.spans[expected_index].span_name ); expected_index += 1; } + } else { + // At the very end, bump the expected index past the wildcard entry. + // This ensures that we consume all the expected spans. + expected_index += 1; } } else { expected_index += 1; } } + assert_eq!( + expected_index, + expected.spans.len(), + "Not all expected spans were found for tracer {}", + expected.name + ); } }