Skip to content

Commit 5045d7e

Browse files
LarryOstermanLarryOsterman
committed
OTel feedback from Liudmila
1 parent 5e2e14a commit 5045d7e

File tree

7 files changed

+199
-78
lines changed

7 files changed

+199
-78
lines changed

‎sdk/core/azure_core/src/http/policies/request_instrumentation.rs

Lines changed: 38 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ impl RequestInstrumentationPolicy {
5858
if let Some(tracing_provider) = &options.tracing_provider {
5959
Self {
6060
tracer: Some(tracing_provider.get_tracer(
61-
azure_namespace.unwrap_or("Unknown"),
61+
azure_namespace,
6262
crate_name.unwrap_or("unknown"),
6363
crate_version.unwrap_or("unknown"),
6464
)),
@@ -94,16 +94,20 @@ impl Policy for RequestInstrumentationPolicy {
9494
key: HTTP_REQUEST_METHOD_ATTRIBUTE,
9595
value: request.method().to_string().into(),
9696
},
97-
Attribute {
98-
key: AZ_NAMESPACE_ATTRIBUTE,
99-
value: tracer.namespace().into(),
100-
},
10197
Attribute {
10298
key: AZ_SCHEMA_URL_ATTRIBUTE,
10399
value: request.url().scheme().into(),
104100
},
105101
];
106102

103+
if let Some(namespace) = tracer.namespace() {
104+
// If the tracer has a namespace, we set it as an attribute.
105+
span_attributes.push(Attribute {
106+
key: AZ_NAMESPACE_ATTRIBUTE,
107+
value: namespace.into(),
108+
});
109+
}
110+
107111
if !request.url().username().is_empty() || request.url().password().is_some() {
108112
// If the URL contains a password, we do not log it for security reasons.
109113
let full_url = format!(
@@ -186,12 +190,17 @@ impl Policy for RequestInstrumentationPolicy {
186190

187191
let result = next[0].send(ctx, request, &next[1..]).await;
188192

189-
if result.is_err() {
193+
if let Some(err) = result.as_ref().err() {
190194
// If the request failed, set an error type attribute.
191-
span.set_attribute(
192-
ERROR_TYPE_ATTRIBUTE,
193-
result.as_ref().err().unwrap().to_string().into(),
194-
);
195+
let azure_error = err.downcast_ref::<crate::Error>();
196+
if let Some(err_kind) = azure_error.map(|e| e.kind()) {
197+
// If the error is an Azure core error, we set the error type.
198+
span.set_attribute(ERROR_TYPE_ATTRIBUTE, err_kind.to_string().into());
199+
} else {
200+
// Otherwise, we set the error type to the error's text. This should never happen
201+
// as the error should be an Azure core error.
202+
span.set_attribute(ERROR_TYPE_ATTRIBUTE, err.to_string().into());
203+
}
195204
}
196205
if let Ok(response) = result.as_ref() {
197206
// If the request was successful, set the HTTP response status code.
@@ -202,13 +211,12 @@ impl Policy for RequestInstrumentationPolicy {
202211

203212
if response.status().is_server_error() || response.status().is_client_error() {
204213
// If the response status indicates an error, set the span status to error.
214+
// Since the reason can be inferred from the status code, description is left empty.
205215
span.set_status(crate::tracing::SpanStatus::Error {
206-
description: format!(
207-
"HTTP request failed with status code {}: {}",
208-
response.status(),
209-
response.status().canonical_reason()
210-
),
216+
description: "".to_string(),
211217
});
218+
// Set the error type attribute for all HTTP 4XX or 5XX errors.
219+
span.set_attribute(ERROR_TYPE_ATTRIBUTE, response.status().to_string().into());
212220
}
213221
}
214222

@@ -250,7 +258,7 @@ mod tests {
250258
impl TracerProvider for MockTracingProvider {
251259
fn get_tracer(
252260
&self,
253-
azure_namespace: &'static str,
261+
azure_namespace: Option<&'static str>,
254262
crate_name: &'static str,
255263
crate_version: &'static str,
256264
) -> Arc<dyn crate::tracing::Tracer> {
@@ -269,14 +277,14 @@ mod tests {
269277

270278
#[derive(Debug)]
271279
struct MockTracer {
272-
namespace: &'static str,
280+
namespace: Option<&'static str>,
273281
name: &'static str,
274282
version: &'static str,
275283
spans: Mutex<Vec<Arc<MockSpan>>>,
276284
}
277285

278286
impl Tracer for MockTracer {
279-
fn namespace(&self) -> &'static str {
287+
fn namespace(&self) -> Option<&'static str> {
280288
self.namespace
281289
}
282290

@@ -420,7 +428,7 @@ mod tests {
420428
}
421429
fn check_instrumentation_result(
422430
mock_tracer: Arc<MockTracingProvider>,
423-
expected_namespace: &str,
431+
expected_namespace: Option<&str>,
424432
expected_name: &str,
425433
expected_version: &str,
426434
expected_method: &str,
@@ -494,7 +502,7 @@ mod tests {
494502

495503
check_instrumentation_result(
496504
mock_tracer,
497-
"test namespace",
505+
Some("test namespace"),
498506
"test_crate",
499507
"1.0.0",
500508
"GET",
@@ -530,7 +538,7 @@ mod tests {
530538
request.insert_header(headers::CLIENT_REQUEST_ID, "test-client-request-id");
531539

532540
let mock_tracer = run_instrumentation_test(
533-
Some("test namespace"),
541+
None,
534542
Some("test_crate"),
535543
Some("1.0.0"),
536544
&mut request,
@@ -550,16 +558,12 @@ mod tests {
550558

551559
check_instrumentation_result(
552560
mock_tracer.clone(),
553-
"test namespace",
561+
None,
554562
"test_crate",
555563
"1.0.0",
556564
"GET",
557565
SpanStatus::Unset,
558566
vec![
559-
(
560-
AZ_NAMESPACE_ATTRIBUTE,
561-
AttributeValue::from("test namespace"),
562-
),
563567
(AZ_SCHEMA_URL_ATTRIBUTE, AttributeValue::from("https")),
564568
(
565569
AZ_CLIENT_REQUEST_ID_ATTRIBUTE,
@@ -604,13 +608,12 @@ mod tests {
604608

605609
check_instrumentation_result(
606610
mock_tracer_provider,
607-
"Unknown",
611+
None,
608612
"unknown",
609613
"unknown",
610614
"GET",
611615
SpanStatus::Unset,
612616
vec![
613-
(AZ_NAMESPACE_ATTRIBUTE, AttributeValue::from("Unknown")),
614617
(AZ_SCHEMA_URL_ATTRIBUTE, AttributeValue::from("https")),
615618
(
616619
HTTP_RESPONSE_STATUS_CODE_ATTRIBUTE,
@@ -654,14 +657,19 @@ mod tests {
654657

655658
check_instrumentation_result(
656659
mock_tracer.clone(),
657-
"test namespace",
660+
Some("test namespace"),
658661
"test_crate",
659662
"1.0.0",
660663
"PUT",
661664
SpanStatus::Error {
662-
description: "HTTP request failed with status code 404: Not Found".to_string(),
665+
description: "".to_string(),
663666
},
664667
vec![
668+
(ERROR_TYPE_ATTRIBUTE, AttributeValue::from("404")),
669+
(
670+
AZ_SERVICE_REQUEST_ID_ATTRIBUTE,
671+
AttributeValue::from("test-service-request-id"),
672+
),
665673
(
666674
AZ_NAMESPACE_ATTRIBUTE,
667675
AttributeValue::from("test namespace"),

‎sdk/core/azure_core_opentelemetry/src/span.rs

Lines changed: 15 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,6 @@ impl Span for OpenTelemetrySpan {
7171
fn set_status(&self, status: SpanStatus) {
7272
let otel_status = match status {
7373
SpanStatus::Unset => opentelemetry::trace::Status::Unset,
74-
SpanStatus::Ok => opentelemetry::trace::Status::Ok,
7574
SpanStatus::Error { description } => opentelemetry::trace::Status::error(description),
7675
};
7776
self.context.span().set_status(otel_status);
@@ -142,9 +141,10 @@ mod tests {
142141

143142
let tracer_provider = OpenTelemetryTracerProvider::new(otel_tracer_provider);
144143
assert!(tracer_provider.is_ok());
145-
let tracer = tracer_provider
146-
.unwrap()
147-
.get_tracer("Microsoft.SpecialCase", "test", "0.1.0");
144+
let tracer =
145+
tracer_provider
146+
.unwrap()
147+
.get_tracer(Some("Microsoft.SpecialCase"), "test", "0.1.0");
148148
let span = tracer.start_span("test_span", SpanKind::Client, vec![]);
149149
span.end();
150150

@@ -165,7 +165,7 @@ mod tests {
165165
assert!(tracer_provider.is_ok());
166166
let tracer = tracer_provider
167167
.unwrap()
168-
.get_tracer("Special Name", "test", "0.1.0");
168+
.get_tracer(Some("Special Name"), "test", "0.1.0");
169169
let parent_span = tracer.start_span("parent_span", SpanKind::Server, vec![]);
170170
let child_span = tracer.start_span_with_parent(
171171
"child_span",
@@ -199,7 +199,7 @@ mod tests {
199199
assert!(tracer_provider.is_ok());
200200
let tracer = tracer_provider
201201
.unwrap()
202-
.get_tracer("MyNamespace", "test", "0.1.0");
202+
.get_tracer(Some("MyNamespace"), "test", "0.1.0");
203203
let span1 = tracer.start_span("span1", SpanKind::Internal, vec![]);
204204
let span2 = tracer.start_span("span2", SpanKind::Server, vec![]);
205205
let child_span =
@@ -231,7 +231,7 @@ mod tests {
231231
assert!(tracer_provider.is_ok());
232232
let tracer = tracer_provider
233233
.unwrap()
234-
.get_tracer("Namespace", "test", "0.1.0");
234+
.get_tracer(Some("Namespace"), "test", "0.1.0");
235235
let span1 = tracer.start_span("span1", SpanKind::Internal, vec![]);
236236
let span2 = tracer.start_span("span2", SpanKind::Server, vec![]);
237237
let _span_guard = span2
@@ -265,7 +265,7 @@ mod tests {
265265
assert!(tracer_provider.is_ok());
266266
let tracer = tracer_provider
267267
.unwrap()
268-
.get_tracer("ThisNamespace", "test", "0.1.0");
268+
.get_tracer(Some("ThisNamespace"), "test", "0.1.0");
269269
let span = tracer.start_span("test_span", SpanKind::Internal, vec![]);
270270

271271
span.set_attribute("test_key", AttributeValue::String("test_value".to_string()));
@@ -291,7 +291,7 @@ mod tests {
291291
assert!(tracer_provider.is_ok());
292292
let tracer = tracer_provider
293293
.unwrap()
294-
.get_tracer("namespace", "test", "0.1.0");
294+
.get_tracer(Some("namespace"), "test", "0.1.0");
295295
let span = tracer.start_span("test_span", SpanKind::Client, vec![]);
296296

297297
let error = Error::new(ErrorKind::NotFound, "resource not found");
@@ -320,11 +320,10 @@ mod tests {
320320
assert!(tracer_provider.is_ok());
321321
let tracer = tracer_provider
322322
.unwrap()
323-
.get_tracer("Namespace", "test", "0.1.0");
323+
.get_tracer(Some("Namespace"), "test", "0.1.0");
324324

325-
// Test Ok status
326-
let span = tracer.start_span("test_span_ok", SpanKind::Server, vec![]);
327-
span.set_status(SpanStatus::Ok);
325+
// Test Unset status
326+
let span = tracer.start_span("test_span_unset", SpanKind::Server, vec![]);
328327
span.end();
329328

330329
// Test Error status
@@ -337,14 +336,13 @@ mod tests {
337336
let spans = otel_exporter.get_finished_spans().unwrap();
338337
assert_eq!(spans.len(), 2);
339338

340-
let ok_span = spans.iter().find(|s| s.name == "test_span_ok").unwrap();
341-
assert_eq!(ok_span.status, opentelemetry::trace::Status::Ok);
342-
343339
let error_span = spans.iter().find(|s| s.name == "test_span_error").unwrap();
344340
assert_eq!(
345341
error_span.status,
346342
opentelemetry::trace::Status::error("test error")
347343
);
344+
let unset_span = spans.iter().find(|s| s.name == "test_span_unset").unwrap();
345+
assert_eq!(unset_span.status, opentelemetry::trace::Status::Unset);
348346
}
349347

350348
#[tokio::test]
@@ -354,7 +352,7 @@ mod tests {
354352
assert!(tracer_provider.is_ok());
355353
let tracer = tracer_provider
356354
.unwrap()
357-
.get_tracer("Namespace", "test", "0.1.0");
355+
.get_tracer(Some("Namespace"), "test", "0.1.0");
358356

359357
let future = async {
360358
let context = Context::current();

‎sdk/core/azure_core_opentelemetry/src/telemetry.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ impl OpenTelemetryTracerProvider {
2626
impl TracerProvider for OpenTelemetryTracerProvider {
2727
fn get_tracer(
2828
&self,
29-
namespace: &'static str,
29+
namespace: Option<&'static str>,
3030
package_name: &'static str,
3131
package_version: &'static str,
3232
) -> Arc<dyn azure_core::tracing::Tracer> {

‎sdk/core/azure_core_opentelemetry/src/tracer.rs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,13 @@ use opentelemetry::{
1515
use std::sync::Arc;
1616

1717
pub struct OpenTelemetryTracer {
18-
namespace: &'static str,
18+
namespace: Option<&'static str>,
1919
inner: BoxedTracer,
2020
}
2121

2222
impl OpenTelemetryTracer {
2323
/// Creates a new OpenTelemetry tracer with the given inner tracer.
24-
pub(super) fn new(namespace: &'static str, tracer: BoxedTracer) -> Self {
24+
pub(super) fn new(namespace: Option<&'static str>, tracer: BoxedTracer) -> Self {
2525
Self {
2626
namespace,
2727
inner: tracer,
@@ -30,7 +30,7 @@ impl OpenTelemetryTracer {
3030
}
3131

3232
impl Tracer for OpenTelemetryTracer {
33-
fn namespace(&self) -> &'static str {
33+
fn namespace(&self) -> Option<&'static str> {
3434
self.namespace
3535
}
3636

@@ -112,7 +112,7 @@ mod tests {
112112
fn test_create_tracer() {
113113
let noop_tracer = NoopTracerProvider::new();
114114
let otel_provider = OpenTelemetryTracerProvider::new(Arc::new(noop_tracer)).unwrap();
115-
let tracer = otel_provider.get_tracer("name", "test_tracer", "1.0.0");
115+
let tracer = otel_provider.get_tracer(Some("name"), "test_tracer", "1.0.0");
116116
let span = tracer.start_span("test_span", SpanKind::Internal, vec![]);
117117
span.end();
118118
}
@@ -121,14 +121,14 @@ mod tests {
121121
fn test_create_tracer_with_sdk_tracer() {
122122
let provider = SdkTracerProvider::builder().build();
123123
let otel_provider = OpenTelemetryTracerProvider::new(Arc::new(provider)).unwrap();
124-
let _tracer = otel_provider.get_tracer("My.Namespace", "test_tracer", "1.0.0");
124+
let _tracer = otel_provider.get_tracer(Some("My.Namespace"), "test_tracer", "1.0.0");
125125
}
126126

127127
#[test]
128128
fn test_create_span_from_tracer() {
129129
let provider = SdkTracerProvider::builder().build();
130130
let otel_provider = OpenTelemetryTracerProvider::new(Arc::new(provider)).unwrap();
131-
let tracer = otel_provider.get_tracer("My.Namespace", "test_tracer", "1.0.0");
131+
let tracer = otel_provider.get_tracer(Some("My.Namespace"), "test_tracer", "1.0.0");
132132
let _span = tracer.start_span("test_span", SpanKind::Internal, vec![]);
133133
}
134134
}

‎sdk/core/azure_core_opentelemetry/tests/integration_test.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ async fn test_span_creation() -> Result<(), Box<dyn Error>> {
1414
let azure_provider = OpenTelemetryTracerProvider::new(sdk_provider)?;
1515

1616
// Get a tracer from the Azure provider
17-
let tracer = azure_provider.get_tracer("test_namespace", "test_tracer", "1.0.0");
17+
let tracer = azure_provider.get_tracer(Some("test_namespace"), "test_tracer", "1.0.0");
1818

1919
// Create a span using the Azure tracer
2020
let span = tracer.start_span("test_span", SpanKind::Internal, vec![]);
@@ -42,7 +42,7 @@ async fn test_tracer_provider_creation() -> Result<(), Box<dyn Error>> {
4242
let azure_provider = OpenTelemetryTracerProvider::new(sdk_provider)?;
4343

4444
// Get a tracer and verify it works
45-
let tracer = azure_provider.get_tracer("tes.namespace", "test_tracer", "1.0.0");
45+
let tracer = azure_provider.get_tracer(Some("test.namespace"), "test_tracer", "1.0.0");
4646
let span = tracer.start_span("test_span", SpanKind::Internal, vec![]);
4747
span.end();
4848

@@ -56,7 +56,7 @@ async fn test_span_attributes() -> Result<(), Box<dyn Error>> {
5656
let azure_provider = OpenTelemetryTracerProvider::new(sdk_provider)?;
5757

5858
// Get a tracer from the Azure provider
59-
let tracer = azure_provider.get_tracer("test.namespace", "test_tracer", "1.0.0");
59+
let tracer = azure_provider.get_tracer(Some("test.namespace"), "test_tracer", "1.0.0");
6060

6161
// Create span with multiple attributes
6262
let span = tracer.start_span("test_span", SpanKind::Internal, vec![]);

0 commit comments

Comments
 (0)