diff --git a/CHANGELOG.md b/CHANGELOG.md index 67b4e4ab4e7..77ac5124651 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - Add metadata support for the `/upload` endpoint. ([#6028](https://github.com/getsentry/relay/pull/6028)) - Infer user agents and client addresses in the V2 standalone span pipeline. ([#6047](https://github.com/getsentry/relay/pull/6047)) +- Conditionally allow additional exceptions on minidumps. ([#6063](https://github.com/getsentry/relay/pull/6063)) **Bug Fixes**: diff --git a/relay-dynamic-config/src/feature.rs b/relay-dynamic-config/src/feature.rs index 15b86392c6e..2a38ef41e6a 100644 --- a/relay-dynamic-config/src/feature.rs +++ b/relay-dynamic-config/src/feature.rs @@ -105,6 +105,9 @@ pub enum Feature { /// Stream minidumps to objectstore. #[serde(rename = "projects:relay-minidump-uploads")] MinidumpUploads, + /// Allow additional exceptions to accompany minidumps. + #[serde(rename = "projects:minidump-multi-exception")] + MinidumpMultiException, /// Enables OTLP spans to use the Span V2 processing pipeline in Relay. /// diff --git a/relay-server/src/processing/errors/errors/apple_crash_report.rs b/relay-server/src/processing/errors/errors/apple_crash_report.rs index 2b6bd16ae12..3baa3b22412 100644 --- a/relay-server/src/processing/errors/errors/apple_crash_report.rs +++ b/relay-server/src/processing/errors/errors/apple_crash_report.rs @@ -26,7 +26,10 @@ impl SentryError for AppleCrashReport { let mut event = utils::take_event_from_crash_items(items, &mut metrics, ctx)?; utils::if_processing!(ctx, { - crate::utils::process_apple_crash_report(event.get_or_insert_with(Default::default)); + crate::utils::process_apple_crash_report( + event.get_or_insert_with(Default::default), + ctx.processing.project_info, + ); metrics.bytes_ingested_event_applecrashreport = (apple_crash_report.len() as u64).into(); }); diff --git a/relay-server/src/processing/errors/errors/minidump.rs b/relay-server/src/processing/errors/errors/minidump.rs index b6f801f6305..45aa22fd041 100644 --- a/relay-server/src/processing/errors/errors/minidump.rs +++ b/relay-server/src/processing/errors/errors/minidump.rs @@ -26,7 +26,11 @@ impl SentryError for Minidump { let mut event = utils::take_event_from_crash_items(items, &mut metrics, ctx)?; utils::if_processing!(ctx, { - crate::utils::process_minidump(event.get_or_insert_with(Default::default), &minidump); + crate::utils::process_minidump( + event.get_or_insert_with(Default::default), + &minidump, + ctx.processing.project_info, + ); metrics.bytes_ingested_event_minidump = (minidump.attachment_body_size() as u64).into(); }); diff --git a/relay-server/src/processing/errors/errors/playstation.rs b/relay-server/src/processing/errors/errors/playstation.rs index 3363e07296e..6c2f346c4f3 100644 --- a/relay-server/src/processing/errors/errors/playstation.rs +++ b/relay-server/src/processing/errors/errors/playstation.rs @@ -107,7 +107,7 @@ impl SentryError for Playstation { // If the original prosperodump is already rate limited, so will be the minidump. item.set_rate_limited(prosperodump.rate_limited()); - crate::utils::process_minidump(event.get_or_insert_with(Event::default), &item); + crate::utils::process_minidump(event.get_or_insert_with(Event::default), &item, ctx.processing.project_info); item }; diff --git a/relay-server/src/processing/errors/errors/unreal.rs b/relay-server/src/processing/errors/errors/unreal.rs index 36ddd758daf..44af0186767 100644 --- a/relay-server/src/processing/errors/errors/unreal.rs +++ b/relay-server/src/processing/errors/errors/unreal.rs @@ -103,12 +103,13 @@ impl SentryError for Unreal { crate::utils::process_minidump( event.get_or_insert_with(Default::default), minidump, + ctx.processing.project_info ); metrics.bytes_ingested_event_minidump = (minidump.attachment_body_size() as u64).into(); } if let Some(acr) = &apple_crash_report { crate::utils::process_apple_crash_report( - event.get_or_insert_with(Default::default) + event.get_or_insert_with(Default::default), ctx.processing.project_info ); metrics.bytes_ingested_event_applecrashreport = (acr.len() as u64).into(); } diff --git a/relay-server/src/utils/native.rs b/relay-server/src/utils/native.rs index 45cd0269326..5d23ed50db8 100644 --- a/relay-server/src/utils/native.rs +++ b/relay-server/src/utils/native.rs @@ -10,6 +10,7 @@ use chrono::{TimeZone, Utc}; use minidump::{ MinidumpAnnotation, MinidumpCrashpadInfo, MinidumpModuleList, Module, StabilityReport, }; +use relay_dynamic_config::Feature; use relay_event_schema::protocol::{ ClientSdkInfo, Context, Contexts, Event, Exception, JsonLenientString, Level, Mechanism, StabilityReportContext, Values, @@ -17,6 +18,7 @@ use relay_event_schema::protocol::{ use relay_protocol::{Annotated, Value}; use crate::envelope::{Item, ItemType}; +use crate::services::projects::project::ProjectInfo; type Minidump<'a> = minidump::Minidump<'a, &'a [u8]>; @@ -43,7 +45,11 @@ struct NativePlaceholder { /// /// This will indicate to the ingestion pipeline that this event will need to be processed. The /// payload can be checked via `is_minidump_event`. -fn write_native_placeholder(event: &mut Event, placeholder: NativePlaceholder) { +fn write_native_placeholder( + event: &mut Event, + placeholder: NativePlaceholder, + project_info: &ProjectInfo, +) { // Events must be native platform. let platform = event.platform.value_mut(); *platform = Some("native".to_owned()); @@ -65,19 +71,26 @@ fn write_native_placeholder(event: &mut Event, placeholder: NativePlaceholder) { .value_mut() .get_or_insert_with(Vec::new); - exceptions.clear(); // clear previous errors if any + if !project_info.has_feature(Feature::MinidumpMultiException) { + exceptions.clear(); // clear previous errors if any + } - exceptions.push(Annotated::new(Exception { - ty: Annotated::new(placeholder.exception_type.to_owned()), - value: Annotated::new(JsonLenientString(placeholder.exception_value.to_owned())), - mechanism: Annotated::new(Mechanism { - ty: Annotated::from(placeholder.mechanism_type.to_owned()), - handled: Annotated::from(false), - synthetic: Annotated::from(true), - ..Mechanism::default() + // The placeholder for the minidump exception has to be the first in the list. This is what + // sentry expects: https://github.com/getsentry/sentry/blob/f949db3155fcb6b79d3ee5e875b542460ed7c2c4/src/sentry/lang/native/utils.py#L149 + exceptions.insert( + 0, + Annotated::new(Exception { + ty: Annotated::new(placeholder.exception_type.to_owned()), + value: Annotated::new(JsonLenientString(placeholder.exception_value.to_owned())), + mechanism: Annotated::new(Mechanism { + ty: Annotated::from(placeholder.mechanism_type.to_owned()), + handled: Annotated::from(false), + synthetic: Annotated::from(true), + ..Mechanism::default() + }), + ..Exception::default() }), - ..Exception::default() - })); + ); } /// Generates crashpad contexts for annotations stored in the minidump. @@ -196,14 +209,14 @@ fn write_crashpad_annotations( /// /// This function operates at best-effort. It always attaches the placeholder and returns /// successfully, even if the minidump or part of its data cannot be parsed. -pub fn process_minidump(event: &mut Event, item: &Item) { +pub fn process_minidump(event: &mut Event, item: &Item, project_info: &ProjectInfo) { debug_assert_eq!(item.ty(), &ItemType::Attachment); let placeholder = NativePlaceholder { exception_type: "Minidump", exception_value: "Invalid Minidump", mechanism_type: "minidump", }; - write_native_placeholder(event, placeholder); + write_native_placeholder(event, placeholder, project_info); if item.is_attachment_ref() { // We don't have a full minidump, just a placeholder for something that was uploaded @@ -266,11 +279,11 @@ pub fn process_minidump(event: &mut Event, item: &Item) { /// Writes minimal information into the event to indicate it is associated with an Apple Crash /// Report. -pub fn process_apple_crash_report(event: &mut Event) { +pub fn process_apple_crash_report(event: &mut Event, project_info: &ProjectInfo) { let placeholder = NativePlaceholder { exception_type: "AppleCrashReport", exception_value: "Invalid Apple Crash Report", mechanism_type: "applecrashreport", }; - write_native_placeholder(event, placeholder); + write_native_placeholder(event, placeholder, project_info); } diff --git a/tests/integration/fixtures/processing.py b/tests/integration/fixtures/processing.py index 03e83a60080..c67a93b8596 100644 --- a/tests/integration/fixtures/processing.py +++ b/tests/integration/fixtures/processing.py @@ -502,7 +502,7 @@ def get_event(self, timeout=None): assert message.error() is None event = msgpack.unpackb(message.value(), raw=False, use_list=False) - assert event["type"] == "event" + assert event["type"] == "event", event["type"] return json.loads(event["payload"].decode("utf8")), event def get_message(self): diff --git a/tests/integration/test_minidump.py b/tests/integration/test_minidump.py index 73e02908be2..4523751fa0c 100644 --- a/tests/integration/test_minidump.py +++ b/tests/integration/test_minidump.py @@ -7,7 +7,7 @@ import queue import pytest from requests import HTTPError -from sentry_sdk.envelope import Envelope +from sentry_sdk.envelope import Envelope, Item, PayloadRef from uuid import UUID from urllib3.filepost import encode_multipart_formdata @@ -677,6 +677,94 @@ def test_minidump_with_processing_invalid( ] +@pytest.mark.parametrize("feature_flag", [False, True]) +def test_minidump_with_event_exception( + mini_sentry, relay_with_processing, attachments_consumer, feature_flag +): + """ + An envelope can carry both a minidump attachment and an event item that already + contains an exception with a stack trace. The user-provided exception must be + preserved alongside the minidump placeholder exception. + """ + dmp_path = os.path.join(os.path.dirname(__file__), "fixtures/native/minidump.dmp") + with open(dmp_path, "rb") as f: + content = f.read() + + relay = relay_with_processing() + + project_id = 42 + project_config = mini_sentry.add_full_project_config(project_id) + config = project_config["config"] + if feature_flag: + config.setdefault("features", []).append("projects:minidump-multi-exception") + + # Disable scrubbing, the basic and full project configs from the mini_sentry fixture + # will modify the minidump since it contains user paths in the module list. + del config["piiConfig"] + + attachments_consumer = attachments_consumer() + + event_id = "2dd132e467174db48dbaddabd3cbed57" + envelope = Envelope(headers=[["event_id", event_id]]) + envelope.add_event( + { + "event_id": event_id, + "exception": { + "values": [ + { + "type": "ZeroDivisionError", + "value": "division by zero", + "stacktrace": { + "frames": [ + { + "function": "divide", + "filename": "app.py", + "lineno": 42, + } + ] + }, + } + ] + }, + } + ) + envelope.add_item( + Item( + headers=[ + ["attachment_type", "event.minidump"], + ["content_type", "application/x-dmp"], + ], + type="attachment", + payload=PayloadRef(bytes=content), + filename="minidump.dmp", + ) + ) + + relay.send_envelope(project_id, envelope) + + _ = attachments_consumer.get_attachment_chunk() + event, message = attachments_consumer.get_event() + + assert event["event_id"] == event_id + assert event["platform"] == "native" + + # The minidump placeholder exception must be present and first in the list, + # so the event is picked up for native processing in Sentry. + minidump_exception, *additional_exceptions = event["exception"]["values"] + assert minidump_exception["mechanism"]["type"] == "minidump" + + # The user-provided exception with its stack trace must be preserved. + if feature_flag: + (user_exception,) = additional_exceptions + assert user_exception["value"] == "division by zero" + assert user_exception["stacktrace"]["frames"][0]["function"] == "divide" + else: + assert not additional_exceptions + + # The minidump must still be forwarded as an attachment. + assert any(att["name"] == "minidump.dmp" for att in message["attachments"]) + + @pytest.mark.parametrize("rate_limits", [[], ["error"], ["error", "attachment"]]) def test_minidump_ratelimit( mini_sentry, relay_with_processing, outcomes_consumer, rate_limits