From 59705bdd30f6d383aa433cf898c418d4ce5c8eb2 Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Thu, 6 Jun 2024 16:24:08 +0200 Subject: [PATCH 1/7] feat(telemetry): add the ability to override span name with otel.name attribute Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- .../src/plugins/telemetry/config_new/spans.rs | 14 ++++++ .../plugins/telemetry/dynamic_attribute.rs | 2 +- .../src/plugins/telemetry/otel/layer.rs | 44 +++++++++++++++++++ .../src/plugins/telemetry/otel/mod.rs | 3 ++ .../src/plugins/telemetry/otel/tracer.rs | 2 + apollo-router/src/plugins/telemetry/reload.rs | 2 +- 6 files changed, 65 insertions(+), 2 deletions(-) diff --git a/apollo-router/src/plugins/telemetry/config_new/spans.rs b/apollo-router/src/plugins/telemetry/config_new/spans.rs index 100875fb49..3304aecca6 100644 --- a/apollo-router/src/plugins/telemetry/config_new/spans.rs +++ b/apollo-router/src/plugins/telemetry/config_new/spans.rs @@ -547,6 +547,16 @@ mod test { value: Arc::new(Default::default()), }, ); + spans.attributes.custom.insert( + "otel.name".to_string(), + Conditional { + selector: RouterSelector::StaticField { + r#static: String::from("new_name").into(), + }, + condition: None, + value: Arc::new(Default::default()), + }, + ); let values = spans.attributes.on_response( &router::Response::fake_builder() .header("my-header", "test_val") @@ -556,6 +566,10 @@ mod test { assert!(values .iter() .any(|key_val| key_val.key == opentelemetry::Key::from_static_str("test"))); + + assert!(values.iter().any(|key_val| key_val.key + == opentelemetry::Key::from_static_str("otel.name") + && key_val.value == opentelemetry::Value::String(String::from("new_name").into()))); } #[test] diff --git a/apollo-router/src/plugins/telemetry/dynamic_attribute.rs b/apollo-router/src/plugins/telemetry/dynamic_attribute.rs index 1e2ea496af..28b9fe79aa 100644 --- a/apollo-router/src/plugins/telemetry/dynamic_attribute.rs +++ b/apollo-router/src/plugins/telemetry/dynamic_attribute.rs @@ -194,7 +194,7 @@ impl SpanDynAttribute for ::tracing::Span { fn update_otel_data(otel_data: &mut OtelData, key: &Key, value: &opentelemetry::Value) { match key.as_str() { - SPAN_NAME_FIELD => otel_data.builder.name = format!("{:?}", value).into(), + SPAN_NAME_FIELD => otel_data.forced_span_name = Some(value.to_string()), SPAN_KIND_FIELD => otel_data.builder.span_kind = str_to_span_kind(&value.as_str()), SPAN_STATUS_CODE_FIELD => otel_data.forced_status = str_to_status(&value.as_str()).into(), SPAN_STATUS_MESSAGE_FIELD => { diff --git a/apollo-router/src/plugins/telemetry/otel/layer.rs b/apollo-router/src/plugins/telemetry/otel/layer.rs index 269b1c41bb..fb835ab71e 100644 --- a/apollo-router/src/plugins/telemetry/otel/layer.rs +++ b/apollo-router/src/plugins/telemetry/otel/layer.rs @@ -709,6 +709,7 @@ where parent_cx, event_attributes: None, forced_status: None, + forced_span_name: None, }); } @@ -884,6 +885,7 @@ where mut builder, parent_cx, forced_status, + forced_span_name, .. }) = extensions.remove::() { @@ -903,6 +905,9 @@ where if let Some(forced_status) = forced_status { builder.status = forced_status; } + if let Some(forced_span_name) = forced_span_name { + builder.name = forced_span_name.into(); + } // Assign end time, build and start span, drop span to export builder @@ -964,8 +969,11 @@ mod tests { use opentelemetry::trace::TraceFlags; use opentelemetry::StringValue; use tracing_subscriber::prelude::*; + use tracing_subscriber::Registry; use super::*; + use crate::plugins::telemetry::dynamic_attribute::SpanDynAttribute; + use crate::plugins::telemetry::reload::SampledSpan; #[derive(Debug, Clone)] struct TestTracer(Arc>>); @@ -993,6 +1001,7 @@ mod tests { parent_cx: parent_cx.clone(), event_attributes: None, forced_status: None, + forced_span_name: None, }); noop::NoopSpan::new() } @@ -1089,6 +1098,41 @@ mod tests { assert_eq!(recorded_name, Some(dynamic_name.into())) } + #[test] + fn forced_dynamic_span_names() { + let dynamic_name = "GET http://example.com".to_string(); + let forced_dynamic_name = "OVERRIDE GET http://example.com".to_string(); + let tracer = TestTracer(Arc::new(Mutex::new(None))); + let subscriber = tracing_subscriber::registry().with(layer().with_tracer(tracer.clone())); + + tracing::subscriber::with_default(subscriber, || { + let span = tracing::debug_span!("static_name", otel.name = dynamic_name.as_str()); + span.with_subscriber(move |(id, dispatch)| { + if let Some(reg) = dispatch.downcast_ref::() { + match reg.span(id) { + Some(s) => { + s.extensions_mut().insert(SampledSpan {}); + } + None => panic!("should not happen"), + } + } + }); + let _entered = span.enter(); + span.set_span_dyn_attribute( + Key::from_static_str("otel.name"), + opentelemetry::Value::String(forced_dynamic_name.clone().into()), + ); + }); + + let recorded_name = tracer + .0 + .lock() + .unwrap() + .as_ref() + .map(|b| b.builder.name.clone()); + assert_eq!(recorded_name, Some(Cow::Owned(forced_dynamic_name))) + } + #[test] fn span_kind() { let tracer = TestTracer(Arc::new(Mutex::new(None))); diff --git a/apollo-router/src/plugins/telemetry/otel/mod.rs b/apollo-router/src/plugins/telemetry/otel/mod.rs index c6173a5fe5..63a1ae72cb 100644 --- a/apollo-router/src/plugins/telemetry/otel/mod.rs +++ b/apollo-router/src/plugins/telemetry/otel/mod.rs @@ -29,4 +29,7 @@ pub(crate) struct OtelData { /// Forced status in case it's coming from the custom attributes pub(crate) forced_status: Option, + + /// Forced span name in case it's coming from the custom attributes + pub(crate) forced_span_name: Option, } diff --git a/apollo-router/src/plugins/telemetry/otel/tracer.rs b/apollo-router/src/plugins/telemetry/otel/tracer.rs index ee9dedb33f..2ea6ecda4a 100644 --- a/apollo-router/src/plugins/telemetry/otel/tracer.rs +++ b/apollo-router/src/plugins/telemetry/otel/tracer.rs @@ -188,6 +188,7 @@ mod tests { parent_cx, event_attributes: None, forced_status: None, + forced_span_name: None, }); let span = cx.span(); let span_context = span.span_context(); @@ -232,6 +233,7 @@ mod tests { parent_cx, event_attributes: None, forced_status: None, + forced_span_name: None, }); assert_eq!( diff --git a/apollo-router/src/plugins/telemetry/reload.rs b/apollo-router/src/plugins/telemetry/reload.rs index bdf182a4aa..96ea8f050a 100644 --- a/apollo-router/src/plugins/telemetry/reload.rs +++ b/apollo-router/src/plugins/telemetry/reload.rs @@ -269,7 +269,7 @@ where } } -struct SampledSpan; +pub(crate) struct SampledSpan; pub(crate) trait IsSampled { fn is_sampled(&self) -> bool; From 95e1bff46e803a5633672b93dba69d3a19dd6e0c Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Thu, 6 Jun 2024 16:30:29 +0200 Subject: [PATCH 2/7] add docs + changelog Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- .../feat_bnjjj_feat_otel_name_attribute.md | 42 +++++++++++++++++++ .../telemetry/instrumentation/spans.mdx | 8 +++- 2 files changed, 49 insertions(+), 1 deletion(-) create mode 100644 .changesets/feat_bnjjj_feat_otel_name_attribute.md diff --git a/.changesets/feat_bnjjj_feat_otel_name_attribute.md b/.changesets/feat_bnjjj_feat_otel_name_attribute.md new file mode 100644 index 0000000000..6cbeb4db3f --- /dev/null +++ b/.changesets/feat_bnjjj_feat_otel_name_attribute.md @@ -0,0 +1,42 @@ +### Add the ability to override span name with otel.name attribute ([Issue #5261](https://github.com/apollographql/router/issues/5261)) + +It gives you the ability to override the span name by using custom telemetry with any [selectors](https://www.apollographql.com/docs/router/configuration/telemetry/instrumentation/selectors/) you want just by setting the `otel.name` attribute. + +Example: + +```yaml +telemetry: + instrumentation: + spans: + router: + otel.name: + static: router # Override the span name to router +``` + + + +--- + +**Checklist** + +Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review. + +- [x] Changes are compatible[^1] +- [ ] Documentation[^2] completed +- [x] Performance impact assessed and acceptable +- Tests added and passing[^3] + - [x] Unit Tests + - [x] Integration Tests + - [x] Manual Tests + +**Exceptions** + +*Note any exceptions here* + +**Notes** + +[^1]: It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. +[^2]: Configuration is an important part of many changes. Where applicable please try to document configuration examples. +[^3]: Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. + +By [@bnjjj](https://github.com/bnjjj) in https://github.com/apollographql/router/pull/5365 \ No newline at end of file diff --git a/docs/source/configuration/telemetry/instrumentation/spans.mdx b/docs/source/configuration/telemetry/instrumentation/spans.mdx index a58c8bd472..8ed6ae8184 100644 --- a/docs/source/configuration/telemetry/instrumentation/spans.mdx +++ b/docs/source/configuration/telemetry/instrumentation/spans.mdx @@ -157,7 +157,11 @@ The `mode` option will be defaulted to `spec_compliant` in a future release, and By default spans are marked in error only if the http status code is different than 200. If you want to mark a span in error for other reason you can override the `otel.status_code` attribute which is responsible to mark a span in error or not. If it's in error then `otel.status_code` = `error`, if not it will be `ok`. -Here is an example if you want to mark the `router` and `supergraph` span in error if you have a graphql error in the payload. +## Span name + +If you want to change the name of spans we're creating for each services you can override this value by setting the `otel.name` attribute using any selectors you want. + +Here is an example if you want to mark the `router` and `supergraph` span in error if you have a graphql error in the payload and you want to enforce the `router` span name to be `graphql_router`. ```yaml title="router.yaml" telemetry: @@ -165,6 +169,8 @@ telemetry: spans: router: attributes: + otel.name: + static: graphql_router # Override the span name to graphql_router otel.status_code: static: error condition: From 7761a7fb75c12d84a656c993575ddfcf1ddc25b7 Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Thu, 6 Jun 2024 16:31:33 +0200 Subject: [PATCH 3/7] fix changelog Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- .../feat_bnjjj_feat_otel_name_attribute.md | 26 ------------------- 1 file changed, 26 deletions(-) diff --git a/.changesets/feat_bnjjj_feat_otel_name_attribute.md b/.changesets/feat_bnjjj_feat_otel_name_attribute.md index 6cbeb4db3f..29ca8940b4 100644 --- a/.changesets/feat_bnjjj_feat_otel_name_attribute.md +++ b/.changesets/feat_bnjjj_feat_otel_name_attribute.md @@ -13,30 +13,4 @@ telemetry: static: router # Override the span name to router ``` - - ---- - -**Checklist** - -Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review. - -- [x] Changes are compatible[^1] -- [ ] Documentation[^2] completed -- [x] Performance impact assessed and acceptable -- Tests added and passing[^3] - - [x] Unit Tests - - [x] Integration Tests - - [x] Manual Tests - -**Exceptions** - -*Note any exceptions here* - -**Notes** - -[^1]: It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. -[^2]: Configuration is an important part of many changes. Where applicable please try to document configuration examples. -[^3]: Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. - By [@bnjjj](https://github.com/bnjjj) in https://github.com/apollographql/router/pull/5365 \ No newline at end of file From 7962a01a4cc1398470c771d1fadab4dca0b20f9a Mon Sep 17 00:00:00 2001 From: Coenen Benjamin Date: Mon, 10 Jun 2024 10:12:21 +0200 Subject: [PATCH 4/7] Update docs/source/configuration/telemetry/instrumentation/spans.mdx Co-authored-by: Bryn Cooke --- docs/source/configuration/telemetry/instrumentation/spans.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/configuration/telemetry/instrumentation/spans.mdx b/docs/source/configuration/telemetry/instrumentation/spans.mdx index 8ed6ae8184..def678efff 100644 --- a/docs/source/configuration/telemetry/instrumentation/spans.mdx +++ b/docs/source/configuration/telemetry/instrumentation/spans.mdx @@ -170,7 +170,7 @@ telemetry: router: attributes: otel.name: - static: graphql_router # Override the span name to graphql_router + static: graphql_router # Override the span name to graphql_router otel.status_code: static: error condition: From dfe533bc6016d47624222eeead3d990e83efc8a1 Mon Sep 17 00:00:00 2001 From: Coenen Benjamin Date: Mon, 10 Jun 2024 10:12:30 +0200 Subject: [PATCH 5/7] Update docs/source/configuration/telemetry/instrumentation/spans.mdx Co-authored-by: Jesse Rosenberger --- docs/source/configuration/telemetry/instrumentation/spans.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/configuration/telemetry/instrumentation/spans.mdx b/docs/source/configuration/telemetry/instrumentation/spans.mdx index def678efff..01f6ba72ad 100644 --- a/docs/source/configuration/telemetry/instrumentation/spans.mdx +++ b/docs/source/configuration/telemetry/instrumentation/spans.mdx @@ -157,7 +157,7 @@ The `mode` option will be defaulted to `spec_compliant` in a future release, and By default spans are marked in error only if the http status code is different than 200. If you want to mark a span in error for other reason you can override the `otel.status_code` attribute which is responsible to mark a span in error or not. If it's in error then `otel.status_code` = `error`, if not it will be `ok`. -## Span name +## Naming If you want to change the name of spans we're creating for each services you can override this value by setting the `otel.name` attribute using any selectors you want. From 62526483cdb35dec1bb18053be56fb32819b2f50 Mon Sep 17 00:00:00 2001 From: Coenen Benjamin Date: Mon, 10 Jun 2024 10:13:40 +0200 Subject: [PATCH 6/7] Update .changesets/feat_bnjjj_feat_otel_name_attribute.md Co-authored-by: Jesse Rosenberger --- .changesets/feat_bnjjj_feat_otel_name_attribute.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changesets/feat_bnjjj_feat_otel_name_attribute.md b/.changesets/feat_bnjjj_feat_otel_name_attribute.md index 29ca8940b4..d24209cd6e 100644 --- a/.changesets/feat_bnjjj_feat_otel_name_attribute.md +++ b/.changesets/feat_bnjjj_feat_otel_name_attribute.md @@ -1,4 +1,4 @@ -### Add the ability to override span name with otel.name attribute ([Issue #5261](https://github.com/apollographql/router/issues/5261)) +### Override tracing span names using custom span selectors ([Issue #5261](https://github.com/apollographql/router/issues/5261)) It gives you the ability to override the span name by using custom telemetry with any [selectors](https://www.apollographql.com/docs/router/configuration/telemetry/instrumentation/selectors/) you want just by setting the `otel.name` attribute. From 834fe61d1ce1eaf818db2f125373321ab64f6cad Mon Sep 17 00:00:00 2001 From: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> Date: Mon, 10 Jun 2024 10:30:30 +0200 Subject: [PATCH 7/7] docs Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- docs/source/configuration/telemetry/instrumentation/spans.mdx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/source/configuration/telemetry/instrumentation/spans.mdx b/docs/source/configuration/telemetry/instrumentation/spans.mdx index 01f6ba72ad..73553d1d61 100644 --- a/docs/source/configuration/telemetry/instrumentation/spans.mdx +++ b/docs/source/configuration/telemetry/instrumentation/spans.mdx @@ -159,6 +159,9 @@ If it's in error then `otel.status_code` = `error`, if not it will be `ok`. ## Naming +By default, we will use a span naming convention that aligns with the current [semantinc conventions for GraphQL server in OpenTelemetry](https://opentelemetry.io/docs/specs/semconv/graphql/graphql-spans/) which means the root span name +must be of format ` ` provided that `graphql.operation.type` and `graphql.operation.name` are available. + If you want to change the name of spans we're creating for each services you can override this value by setting the `otel.name` attribute using any selectors you want. Here is an example if you want to mark the `router` and `supergraph` span in error if you have a graphql error in the payload and you want to enforce the `router` span name to be `graphql_router`.