Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(telemetry): add ability to set span attributes on connector span #6125

Merged
merged 21 commits into from
Oct 30, 2024
Merged
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
feat(telemetry): add ability to set span attributes on connector span
Signed-off-by: Benjamin <[email protected]>
bnjjj committed Oct 7, 2024
commit cabb35e3a2b7d79f3ac3c42532f711e30370e5ca
Original file line number Diff line number Diff line change
@@ -10,6 +10,7 @@ use crate::plugins::telemetry::config_new::extendable::Extendable;
#[derive(Clone, Deserialize, JsonSchema, Debug, Default)]
#[serde(deny_unknown_fields, default)]
pub(crate) struct ConnectorEventsKind {
// TODO: I don't think we need this, we should be consistent with what we have today and everything is namespaced like http.*
pubmodmatt marked this conversation as resolved.
Show resolved Hide resolved
pub(crate) http: Extendable<
ConnectorHttpEventsConfig,
Event<ConnectorHttpAttributes, ConnectorHttpSelector>,
Original file line number Diff line number Diff line change
@@ -11,6 +11,7 @@ use crate::plugins::telemetry::config_new::instruments::Instrument;
#[derive(Clone, Deserialize, JsonSchema, Debug, Default)]
#[serde(deny_unknown_fields, default)]
pub(crate) struct ConnectorInstrumentsKind {
// TODO: I don't think we need this, we should be consistent with what we have today and everything is namespaced like http.*
pub(crate) http: Extendable<
ConnectorHttpInstrumentsConfig,
Instrument<ConnectorHttpAttributes, ConnectorHttpSelector, ConnectorHttpValue>,
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
//! Connectors telemetry.

pub(crate) mod events;
pub(crate) mod http;
pub(crate) mod instruments;
pub(crate) mod spans;
27 changes: 27 additions & 0 deletions apollo-router/src/plugins/telemetry/config_new/connector/spans.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
use schemars::JsonSchema;
use serde::Deserialize;

use crate::plugins::telemetry::config_new::attributes::DefaultAttributeRequirementLevel;
use crate::plugins::telemetry::config_new::conditional::Conditional;
use crate::plugins::telemetry::config_new::connector::http::attributes::ConnectorHttpAttributes;
use crate::plugins::telemetry::config_new::connector::http::selectors::ConnectorHttpSelector;
use crate::plugins::telemetry::config_new::extendable::Extendable;
use crate::plugins::telemetry::config_new::DefaultForLevel;
use crate::plugins::telemetry::otlp::TelemetryDataKind;

#[derive(Deserialize, JsonSchema, Clone, Default, Debug)]
#[serde(deny_unknown_fields, default)]
pub(crate) struct ConnectorSpans {
/// Custom attributes that are attached to the connector span.
pub(crate) attributes: Extendable<ConnectorHttpAttributes, Conditional<ConnectorHttpSelector>>,
}

impl DefaultForLevel for ConnectorSpans {
fn defaults_for_level(
&mut self,
requirement_level: DefaultAttributeRequirementLevel,
kind: TelemetryDataKind,
) {
self.attributes.defaults_for_level(requirement_level, kind);
}
}
6 changes: 6 additions & 0 deletions apollo-router/src/plugins/telemetry/config_new/spans.rs
Original file line number Diff line number Diff line change
@@ -2,6 +2,8 @@ use schemars::JsonSchema;
use serde::Deserialize;

use super::conditional::Conditional;
use super::connector::http::selectors::ConnectorHttpSelector;
use super::connector::spans::ConnectorSpans;
use crate::plugins::telemetry::config_new::attributes::DefaultAttributeRequirementLevel;
use crate::plugins::telemetry::config_new::attributes::RouterAttributes;
use crate::plugins::telemetry::config_new::attributes::SubgraphAttributes;
@@ -35,6 +37,10 @@ pub(crate) struct Spans {
/// Attributes to include on the subgraph span.
/// Subgraph spans contain information about the subgraph request and response and therefore contain subgraph specific attributes.
pub(crate) subgraph: SubgraphSpans,

/// Attributes to include on the connector span.
/// Connector spans contain information about the connector request and response and therefore contain connector specific attributes.
pub(crate) connector: ConnectorSpans,
}

impl Spans {
90 changes: 90 additions & 0 deletions apollo-router/src/plugins/telemetry/dynamic_attribute.rs
Original file line number Diff line number Diff line change
@@ -69,6 +69,11 @@ impl DynAttributeLayer {
pub(crate) trait SpanDynAttribute {
fn set_span_dyn_attribute(&self, key: Key, value: opentelemetry::Value);
fn set_span_dyn_attributes(&self, attributes: impl IntoIterator<Item = KeyValue>);
fn set_span_dyn_attributes_for_span_name(
&self,
span_name: &str,
attributes: impl IntoIterator<Item = KeyValue>,
);
}

impl SpanDynAttribute for ::tracing::Span {
@@ -193,6 +198,91 @@ impl SpanDynAttribute for ::tracing::Span {
}
});
}

fn set_span_dyn_attributes_for_span_name(
&self,
span_name: &str,
attributes: impl IntoIterator<Item = KeyValue>,
) {
let mut attributes = attributes.into_iter().peekable();
if attributes.peek().is_none() {
return;
}
self.with_subscriber(move |(id, dispatch)| {
if let Some(reg) = dispatch.downcast_ref::<Registry>() {
match reg.span(id) {
None => eprintln!("no spanref, this is a bug"),
Some(mut s) => {
if s.name() != span_name {
while let Some(parent_span) = s.parent() {
if parent_span.name() == span_name {
s = parent_span;
break;
}
s = parent_span;
}
}

if s.is_sampled() {
let mut extensions = s.extensions_mut();
match extensions.get_mut::<OtelData>() {
Some(otel_data) => {
if otel_data.builder.attributes.is_none() {
otel_data.builder.attributes = Some(
attributes
.inspect(|attr| {
update_otel_data(
otel_data,
&attr.key,
&attr.value,
)
})
.collect(),
);
} else {
let attributes: Vec<KeyValue> = attributes
.inspect(|attr| {
update_otel_data(otel_data, &attr.key, &attr.value)
})
.collect();
otel_data
.builder
.attributes
.as_mut()
.unwrap()
.extend(attributes);
}
}
None => {
// Can't use ::tracing::error! because it could create deadlock on extensions
eprintln!("no OtelData, this is a bug");
}
}
} else {
let mut attributes = attributes
.filter(|kv| !kv.key.as_str().starts_with(APOLLO_PRIVATE_PREFIX))
.peekable();
if attributes.peek().is_none() {
return;
}
let mut extensions = s.extensions_mut();
match extensions.get_mut::<LogAttributes>() {
Some(registered_attributes) => {
registered_attributes.extend(attributes);
}
None => {
// Can't use ::tracing::error! because it could create deadlock on extensions
eprintln!("no LogAttributes, this is a bug");
}
}
}
}
};
} else {
::tracing::error!("no Registry, this is a bug");
}
});
}
}

fn update_otel_data(otel_data: &mut OtelData, key: &Key, value: &opentelemetry::Value) {
63 changes: 50 additions & 13 deletions apollo-router/src/plugins/telemetry/mod.rs
Original file line number Diff line number Diff line change
@@ -83,6 +83,7 @@ pub(crate) use self::span_factory::SpanMode;
use self::tracing::apollo_telemetry::APOLLO_PRIVATE_DURATION_NS;
use self::tracing::apollo_telemetry::CLIENT_NAME_KEY;
use self::tracing::apollo_telemetry::CLIENT_VERSION_KEY;
use super::connectors::tracing::CONNECT_SPAN_NAME;
use crate::apollo_studio_interop::ExtendedReferenceStats;
use crate::apollo_studio_interop::ReferencedEnums;
use crate::context::CONTAINS_GRAPHQL_ERROR;
@@ -864,6 +865,7 @@ impl PluginPrivate for Telemetry {
service: crate::services::http::BoxService,
) -> crate::services::http::BoxService {
let req_fn_config = self.config.clone();
let res_fn_config = self.config.clone();
let static_connector_instruments = self.connector_custom_instruments.read().clone();
ServiceBuilder::new()
.map_future_with_request_data(
@@ -882,37 +884,72 @@ impl PluginPrivate for Telemetry {
.events
.new_connector_http_events();
custom_events.on_request(http_request);

let custom_span_attributes = req_fn_config
.instrumentation
.spans
.connector
.attributes
.on_request(http_request);
pubmodmatt marked this conversation as resolved.
Show resolved Hide resolved

(
http_request.context.clone(),
Some((custom_instruments, custom_events)),
Some((custom_instruments, custom_events, custom_span_attributes)),
)
} else {
(http_request.context.clone(), None)
}
},
move |(context, custom_telemetry): (
Context,
Option<(ConnectorHttpInstruments, ConnectorHttpEvents)>,
Option<(ConnectorHttpInstruments, ConnectorHttpEvents, Vec<KeyValue>)>,
),
f: BoxFuture<
'static,
Result<crate::services::http::HttpResponse, BoxError>,
>| {
let conf = res_fn_config.clone();
async move {
let result = f.await;
if let Some((custom_instruments, custom_events)) = custom_telemetry {
match &result {
Ok(resp) => {
custom_instruments.on_response(resp);
custom_events.on_response(resp);
}
Err(err) => {
custom_instruments.on_error(err, &context);
custom_events.on_error(err, &context);
match custom_telemetry {
Some((custom_instruments, custom_events, custom_span_attributes)) => {
let span = Span::current();
bnjjj marked this conversation as resolved.
Show resolved Hide resolved
span.set_span_dyn_attributes_for_span_name(
CONNECT_SPAN_NAME,
custom_span_attributes,
);
let result = f.await;

match &result {
Ok(resp) => {
span.set_span_dyn_attributes_for_span_name(
CONNECT_SPAN_NAME,
conf.instrumentation
.spans
.connector
.attributes
.on_response(resp),
);
custom_instruments.on_response(resp);
custom_events.on_response(resp);
}
Err(err) => {
span.set_span_dyn_attributes_for_span_name(
CONNECT_SPAN_NAME,
conf.instrumentation
.spans
.connector
.attributes
.on_error(err, &context),
);
custom_instruments.on_error(err, &context);
custom_events.on_error(err, &context);
}
}

result
}
None => f.await,
}
result
}
},
)
3 changes: 3 additions & 0 deletions apollo-router/src/services/connector_service.rs
Original file line number Diff line number Diff line change
@@ -124,6 +124,9 @@ impl tower::Service<ConnectRequest> for ConnectorService {
"apollo.connector.source.detail" = tracing::field::Empty,
"apollo_private.sent_time_offset" = fetch_time_offset,
);
// TODO: I think we should get rid of these attributes by default and only add it from custom telemetry. We just need to double check it's not required for Studio.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @timbotnik I need your knowledge here


// These additionnal attributes will be added to custom telemetry feature
// TODO: apollo.connector.field.alias
// TODO: apollo.connector.field.return_type
// TODO: apollo.connector.field.selection_set