feat(eap): Implement gen_ai attribute transformations - #6201
feat(eap): Implement gen_ai attribute transformations#6201constantinius wants to merge 6 commits into
Conversation
|
@constantinius talked with the team, we'll look into dealing with the transaction normalization case in #6216 - Since you already removed the code for it, is this normalization required to run on transaction spans? So to get you unblocked I would propose we merge this change for the span streaming pipeline and we as a team take over the transaction change, if it is necessary. Also please remove the redundant information from the PR description, changed files and tests I can tell by the diff, even the functional changes I can see in code, the important information in the description and later commit is the motivation and the effect not the functional changes, which are already present in code. |
loewenheim
left a comment
There was a problem hiding this comment.
This is a good start. Please feel free to ask if you need help with the requested changes.
There was a problem hiding this comment.
This shouldn't be necessary anymore, master has a more up to date version.
There was a problem hiding this comment.
This sentry-conventions PR contains the transformations. We should point to that version, once its merged.
5b1564d to
8f8a074
Compare
loewenheim
left a comment
There was a problem hiding this comment.
This is a lot better. I have two comments, otherwise I'm good with merging this. If you like I can also take care of these.
| /// A message in the old `gen_ai.request.messages` format. | ||
| #[derive(Deserialize)] | ||
| struct OldMessage { | ||
| #[serde(default)] |
There was a problem hiding this comment.
This isn't necessary for Option.
| #[serde(default)] |
| // and let the rest carry the original fields. | ||
| return NewMessage { | ||
| role: None, | ||
| parts: vec![], |
There was a problem hiding this comment.
parts doesn't have skip_serializing_if. That means that in this case it will be serialized as parts: []. Not sure if that is intentional.
There was a problem hiding this comment.
I've split up the messages struct to make this explicit. Hope this improves it. But parts shouldn't be empty anyways.
ad98c57 to
063a813
Compare
Implement the two attribute transformations from sentry-conventions PR #465: `gen_ai_request_messages_to_input_messages` and `gen_ai_response_to_output_messages`. These reshape attribute values beyond simple key renaming. `gen_ai.request.messages` with `content` fields is converted to `gen_ai.input.messages` with `parts` arrays. `gen_ai.response.text` (plain string, JSON string array, or objects with content) and `gen_ai.response.tool_calls` are combined into a single `gen_ai.output.messages` assistant message with typed parts. The transformation is generic over `AttributesLike` so it works on both `Attributes` (SpanV2) and `SpanData` (SpanV1) without duplication. It runs inside both `normalize_ai` (EAP pipeline) and `enrich_ai_span_data` (transaction/legacy pipeline). Attributes with the new `"transform"` deprecation status produce `WriteBehavior::CurrentName` so `normalize_attribute_names` leaves them alone — the dedicated transformation code handles the full move-and-reshape. Deprecated keys are always cleaned up, even when the canonical key already exists or the value cannot be parsed. Contributes to TET-2587
- Remove redundant #[serde(default)] on Option<OldContent> - Use TransformedMessage struct with skip_serializing_if for request message output instead of raw serde_json::Value - Split OutputMessage (response only) from TransformedMessage (request) - Messages without content field pass through unchanged without spurious empty parts array - Remove inline integration test module
Fix collapsible_if lint in gen_ai_transform content_to_parts. Update normalize_mobile_measurements snapshot for new app.vitals.stall.duration attribute from conventions bump.
06f26cd to
0214391
Compare
da5df35 to
298d646
Compare
Run the gen_ai attribute transformation inside normalize_attribute_names rather than from each AI normalization function separately. This ensures the transformation runs on every path that normalizes attribute names, without needing explicit call sites in normalize_ai and enrich_ai_span_data.
298d646 to
48fa42d
Compare
Dav1dde
left a comment
There was a problem hiding this comment.
The original code had a very loose conversion going with just using serde_json::Value for anything.
The suggestion was to type the effective schema which is actually sent and convert it into the schema of what will actually be produced. If any part fails to deserialize we know this is not something the product can understand and we can just fall back to keeping the original string and skipping the transform.
The PR now only addresses the original concern on a single level, keeping a lot of the loose conversion stuff still around.
Overall this allows the product to actually expect a certain schema Relay can enforce, but also is something we can build on and add more and more variants as we discover them with proper documentation.
It also means we won't be producing schema invalid messages according to otel.
Ideally in the end there is no more serde_json::Value except where the schema is expected to match 1:1 (parts) or must be extensible (not can be). For these cases we can also use &'a RawValue. serde_json::json! shouldn't be necessary at all.
I suggest starting with a single message you want to convert, add an integration test, type and implement the schema. Then go to the next message variant, write a new integration test, adjust the schema/types and so on.
|
|
||
| mod ai; | ||
| mod attribute_like; | ||
| pub(crate) mod gen_ai_transform; |
There was a problem hiding this comment.
Why did you make this pub(crate)?
| TEST_CONFIG = { | ||
| "outcomes": { | ||
| "emit_outcomes": True, | ||
| } | ||
| } |
There was a problem hiding this comment.
Curious why you enabled outcomes, doesn't look like you're actually consuming them.
|
|
||
| use super::attribute_like::{AttributeLike, AttributesLike}; | ||
|
|
||
| // --- Input models (what SDKs send) --- |
There was a problem hiding this comment.
Please do a human pass over these comments, if you need more organization for readability, we can also make more modules, though that doesn't seem to be necessary.
| //! | ||
| //! The functions are generic over [`AttributesLike`] so they work on both | ||
| //! [`Attributes`](relay_event_schema::protocol::Attributes) (SpanV2) and | ||
| //! [`SpanData`](relay_event_schema::protocol::SpanData) (SpanV1). |
| // --- Transformation logic --- | ||
|
|
||
| /// Applies gen_ai attribute transformations. | ||
| pub(crate) fn transform_gen_ai<T: AttributesLike>(attributes: &mut T) { |
There was a problem hiding this comment.
If you use other modules as a reference, you see that we don't use pub(crate) and if you follow the visibility rules of Rust, it's also not necessary.
| struct OldMessage { | ||
| content: Option<OldContent>, | ||
| #[serde(flatten)] | ||
| rest: serde_json::Map<String, serde_json::Value>, |
There was a problem hiding this comment.
Why is this necessary, do you now have a schema which exactly describes what other fields are allowed?
| #[serde(untagged)] | ||
| enum OldContent { | ||
| String(String), | ||
| Parts(Vec<serde_json::Map<String, serde_json::Value>>), |
There was a problem hiding this comment.
Again here, curious why you fall back to Map<>, if the idea was to type out the Schema, this will also make the conversion logic easier.
|
|
||
| if parts.is_empty() { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Incomplete output messages when tool_calls parsing fails alongside text
Medium Severity
When both gen_ai.response.text and an unparseable gen_ai.response.tool_calls are present, gen_ai.response.text is unconditionally removed (line 175), and gen_ai.output.messages is created containing only the text parts. This produces an incomplete gen_ai.output.messages that omits tool call data, while gen_ai.response.tool_calls is preserved under its deprecated key. Downstream consumers seeing gen_ai.output.messages may incorrectly conclude the response contained no tool calls. The gen_ai.response.text removal is unconditional regardless of whether the overall transformation succeeded.
Reviewed by Cursor Bugbot for commit d76f1bf. Configure here.
Remove _meta from span 0 (invoke_agent, no deprecated attrs) and HTTP client spans (no deprecated attrs). Add _meta to gen_ai and tool spans that now produce deprecation remarks. Update deprecated attribute values to match current conventions behavior.
d76f1bf to
193a8ca
Compare
| enum SdkContentItem { | ||
| Tagged(SdkPart), | ||
| Untagged(SdkContentObject), | ||
| Generic(GenericPart), | ||
| } |
There was a problem hiding this comment.
Bug: Deserializing a content item with an unknown type and a text field incorrectly matches SdkContentObject::Text, causing other fields in the object to be silently discarded.
Severity: HIGH
Suggested Fix
Reorder the SdkContentItem enum variants to prioritize a generic catch-all before the more specific SdkContentObject. Alternatively, add #[serde(deny_unknown_fields)] to specific objects like SdkTextObject to prevent incorrect matches. Adding test coverage for this scenario is also recommended.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: relay-event-normalization/src/eap/gen_ai_transform.rs#L39-L43
Potential issue: The `SdkContentItem` enum uses `#[serde(untagged)]`, which deserializes
variants in order. When an object with an unknown `type` and a `text` field (e.g.,
`{"type": "future_type", "text": "some text"}`) is processed, it fails to match any
`SdkPart` variant. It then falls back to `SdkContentObject`, where it successfully
matches `SdkContentObject::Text` because a `text` field is present. This causes all
other fields, including the original `type`, to be silently discarded, leading to data
loss and breaking forward compatibility with new SDK content types.
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Bug: The gen_ai.response.text attribute is removed even if it parses to zero parts (e.g., from "[]"), but the new gen_ai.output.messages attribute is not created, causing silent data loss.
Severity: MEDIUM
Suggested Fix
The gen_ai.response.text attribute should only be removed if extract_text_parts successfully produces one or more parts and the new gen_ai.output.messages attribute is created. If no parts are produced, the original attribute should be preserved, similar to how unparseable tool_calls are handled.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: relay-event-normalization/src/eap/gen_ai_transform.rs#L551-L553
Potential issue: In `transform_response_to_output_messages`, the `gen_ai.response.text`
attribute is unconditionally removed after being read. If the attribute's value is a
JSON string that parses into zero content parts (e.g., an empty array `"[]"`), the
function exits early without creating the new `gen_ai.output.messages` attribute. This
results in the silent deletion of the original attribute without creating its
replacement, causing data loss.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 193a8ca. Configure here.
| #[serde(default)] | ||
| name: Option<String>, | ||
| content: Option<RequestMessageContent>, | ||
| } |
There was a problem hiding this comment.
Parts dropped under old key
High Severity
RequestMessage only reads content and ignores parts. Messages that are already in the canonical parts shape under gen_ai.request.messages still deserialize successfully, then get rewritten with empty parts, so the original content is lost instead of being moved as-is.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 193a8ca. Configure here.
| modality: Option<String>, | ||
| #[serde(default)] | ||
| file_id: Option<String>, | ||
| } |
There was a problem hiding this comment.
File inline data discarded
Medium Severity
type: "file" parts with inline data are mapped to FilePart, which only keeps file_id/mime_type/modality. The inline payload is ignored, so common SDK file parts lose their content after transformation.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 193a8ca. Configure here.
| let Ok(messages) = serde_json::from_str::<Vec<RequestMessage>>(&raw) else { | ||
| if let Some(attr) = attributes.remove(GEN_AI__REQUEST__MESSAGES) { | ||
| attributes.insert(GEN_AI__INPUT__MESSAGES.to_owned(), attr); | ||
| } | ||
| return; | ||
| }; | ||
|
|
||
| let new_messages: Vec<InputMessage> = messages | ||
| .into_iter() | ||
| .map(|msg| { | ||
| let parts: Vec<InputPart> = match msg.content { | ||
| Some(RequestMessageContent::String(s)) => { | ||
| vec![InputPart::Text(TextPart { content: s })] | ||
| } | ||
| Some(RequestMessageContent::Array(items)) => { | ||
| items.into_iter().map(InputPart::from).collect() | ||
| } | ||
| Some(RequestMessageContent::Single(item)) => vec![InputPart::from(item)], | ||
| None => vec![], | ||
| }; | ||
|
|
||
| InputMessage { | ||
| role: msg.role.unwrap_or_default(), | ||
| parts, | ||
| name: msg.name, | ||
| } | ||
| }) | ||
| .collect(); | ||
|
|
||
| if let Ok(json) = serde_json::to_string(&new_messages) { | ||
| attributes.insert( | ||
| GEN_AI__INPUT__MESSAGES.to_owned(), | ||
| T::Value::from(json).into(), | ||
| ); | ||
| } | ||
| attributes.remove(GEN_AI__REQUEST__MESSAGES); | ||
| } | ||
|
|
||
| /// Transforms `gen_ai.response.text` + `gen_ai.response.tool_calls` → `gen_ai.output.messages`. | ||
| fn transform_response_to_output_messages<T: AttributesLike>(attributes: &mut T) { | ||
| if attributes.contains_key(GEN_AI__OUTPUT__MESSAGES) { | ||
| attributes.remove(GEN_AI__RESPONSE__TEXT); | ||
| attributes.remove(GEN_AI__RESPONSE__TOOL_CALLS); | ||
| return; | ||
| } | ||
|
|
||
| let response_text = get_str(attributes, GEN_AI__RESPONSE__TEXT); | ||
| let tool_calls_raw = get_str(attributes, GEN_AI__RESPONSE__TOOL_CALLS); | ||
|
|
||
| if response_text.is_none() && tool_calls_raw.is_none() { | ||
| return; | ||
| } | ||
|
|
||
| let mut parts = Vec::new(); | ||
|
|
||
| if let Some(ref text) = response_text { | ||
| extract_text_parts(text, &mut parts); | ||
| } | ||
|
|
||
| let tool_calls_parsed = tool_calls_raw | ||
| .as_deref() | ||
| .is_none_or(|raw| extract_tool_call_parts(raw, &mut parts)); | ||
|
|
||
| // Clean up deprecated keys. Keep tool_calls if it couldn't be parsed. | ||
| attributes.remove(GEN_AI__RESPONSE__TEXT); | ||
| if tool_calls_parsed { | ||
| attributes.remove(GEN_AI__RESPONSE__TOOL_CALLS); | ||
| } | ||
|
|
||
| if parts.is_empty() { | ||
| return; | ||
| } | ||
|
|
||
| let output = [OutputMessage { | ||
| role: "assistant", | ||
| parts, | ||
| }]; | ||
| if let Ok(json) = serde_json::to_string(&output) { | ||
| attributes.insert( | ||
| GEN_AI__OUTPUT__MESSAGES.to_owned(), | ||
| T::Value::from(json).into(), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /// Extracts text parts from a `gen_ai.response.text` value. | ||
| fn extract_text_parts(raw: &str, parts: &mut Vec<serde_json::Value>) { | ||
| let Ok(parsed) = serde_json::from_str::<ResponseText>(raw) else { | ||
| // Not valid JSON — treat the entire raw string as plain text. | ||
| parts.push(serde_json::json!({"type": "text", "content": raw})); | ||
| return; | ||
| }; | ||
|
|
||
| match parsed { | ||
| ResponseText::String(s) => { | ||
| parts.push(serde_json::json!({"type": "text", "content": s})); | ||
| } | ||
| ResponseText::Array(items) => { | ||
| for item in items { | ||
| match item { | ||
| ResponseTextItem::String(s) => { | ||
| parts.push(serde_json::json!({"type": "text", "content": s})); | ||
| } | ||
| ResponseTextItem::Object { content } => { | ||
| parts.push(serde_json::json!({"type": "text", "content": content})); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ResponseText::Object { content } => { | ||
| parts.push(serde_json::json!({"type": "text", "content": content})); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Extracts tool_call parts from a `gen_ai.response.tool_calls` value. | ||
| /// | ||
| /// Returns `true` if the value was successfully parsed. | ||
| fn extract_tool_call_parts(raw: &str, parts: &mut Vec<serde_json::Value>) -> bool { | ||
| let Ok(tool_calls) = serde_json::from_str::<Vec<ToolCall>>(raw) else { |
There was a problem hiding this comment.
gen_ai attribute JSON parsed without depth limit
Attacker-controlled attribute strings are parsed with serde_json::from_str into recursive structures without caller-side depth limits, allowing a small deeply nested JSON payload to overflow the stack and abort the process.
Evidence
transform_request_messagesat line 469 callsserde_json::from_str::<Vec<RequestMessage>>(&raw)on thegen_ai.request.messagesattribute string.extract_text_partsat line 556 callsserde_json::from_str::<ResponseText>(raw)ongen_ai.response.text;ResponseText::ObjectandResponseTextItem::Objectboth containcontent: serde_json::Value.extract_tool_call_partsat line 588 callsserde_json::from_str::<Vec<ToolCall>>(raw)ongen_ai.response.tool_calls;ToolCalluses#[serde(flatten)]onserde_json::Map<String, serde_json::Value>, so deeply nested JSON values deserialize recursively intoserde_json::Value.- No caller-side depth, size, or time bound is enforced before any of these calls; a ~60 KB string with 10 000 levels of nesting is easily within payload limits and can exhaust the native stack during deserialization, causing an uncatchable process abort.
- The codebase shows awareness of this exact risk:
relay-server/src/utils/rmp.rsexplicitly setsMAX_DEPTH = 128onrmp_serdedeserializers to "protect against stack overflows from maliciously nested payloads".
Identified by Warden · wrdn-dos-review · AR5-SYK


Summary
Implements the two gen_ai attribute transformations introduced in sentry-conventions PR #465.
Old SDKs send AI messages and responses using deprecated attributes (
gen_ai.request.messages,gen_ai.response.text,gen_ai.response.tool_calls) whose value shapes differ from the canonical replacements (gen_ai.input.messages,gen_ai.output.messages). Simple key renaming is not enough — the values need reshaping to match the newparts-based message schema.These transformations run inside AI span normalization on both the SpanV1 (
enrich_ai_span_data) and SpanV2 (normalize_ai) paths, using theAttributesLikeabstraction. Attributes with the new"transform"deprecation status produceWriteBehavior::CurrentNamesonormalize_attribute_namesleaves them alone.Contributes to TET-2587