From 436d2cb520e48a72fadf207c26179c0db199cd00 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Tue, 2 Jun 2020 18:40:12 +0200 Subject: [PATCH 1/2] fix: Make sure we de-duplicate Integrations Integrations are still run in order, but later integrations override former instances, and should run `setup` and be returned by `with_integration`. This also exposes a new global `with_integration` function which is also used in existing integrations. --- sentry-core/src/api.rs | 34 +++++++++++++- sentry-core/src/client.rs | 83 ++++++++++++++++++++++++++++++++--- sentry-core/src/constants.rs | 6 +-- sentry-core/src/hub.rs | 20 +-------- sentry-log/src/integration.rs | 10 +---- sentry-log/src/logger.rs | 16 +++---- sentry-panic/src/lib.rs | 8 ++-- sentry-slog/src/drain.rs | 20 +++------ 8 files changed, 128 insertions(+), 69 deletions(-) diff --git a/sentry-core/src/api.rs b/sentry-core/src/api.rs index d8ac68440..d09689d2b 100644 --- a/sentry-core/src/api.rs +++ b/sentry-core/src/api.rs @@ -1,6 +1,6 @@ use crate::protocol::{Event, Level}; use crate::types::Uuid; -use crate::{Hub, IntoBreadcrumbs, Scope}; +use crate::{Hub, Integration, IntoBreadcrumbs, Scope}; /// Captures an event on the currently active client if any. /// @@ -200,6 +200,38 @@ where } } +/// Looks up an integration on the current Hub. +/// +/// Calls the given function with the requested integration instance when it +/// is active on the currently active client. +/// +/// # Examples +/// +/// ``` +/// use sentry::{ClientOptions, Integration}; +/// +/// struct MyIntegration(usize); +/// impl Integration for MyIntegration {} +/// +/// let options = ClientOptions::default().add_integration(MyIntegration(10)); +/// # let _options = options.clone(); +/// +/// let _sentry = sentry::init(options); +/// +/// # sentry::test::with_captured_events_options(|| { +/// let value = sentry::with_integration(|integration: &MyIntegration, _| integration.0); +/// assert_eq!(value, 10); +/// # }, _options); +/// ``` +pub fn with_integration(f: F) -> R +where + I: Integration, + F: FnOnce(&I, &Hub) -> R, + R: Default, +{ + Hub::with_active(|hub| hub.with_integration(|i| f(i, hub))) +} + /// Returns the last event ID captured. /// /// This uses the current thread local [`Hub`], and will return `None` if no diff --git a/sentry-core/src/client.rs b/sentry-core/src/client.rs index 371c40b7c..204a925e3 100644 --- a/sentry-core/src/client.rs +++ b/sentry-core/src/client.rs @@ -103,13 +103,18 @@ impl Client { let mut sdk_info = SDK_INFO.clone(); - // NOTE: We do not filter out duplicate integrations based on their - // TypeId. - let integrations: Vec<_> = options - .integrations - .iter() - .map(|integration| (integration.as_ref().type_id(), integration.clone())) - .collect(); + let mut integrations = vec![]; + for integration in &options.integrations { + let id = integration.as_ref().type_id(); + match integrations.iter().position(|(iid, _)| *iid == id) { + Some(idx) => { + integrations[idx].1 = integration.clone(); + } + None => { + integrations.push((id, integration.clone())); + } + } + } for (_, integration) in integrations.iter() { integration.setup(&mut options); @@ -273,3 +278,67 @@ impl Client { // Make this unwind safe. It's not out of the box because of the // `BeforeCallback`s inside `ClientOptions`, and the contained Integrations impl RefUnwindSafe for Client {} + +#[test] +fn test_duplicate_integration() { + use crate::test::with_captured_events_options; + use crate::Integration; + use std::sync::atomic::{AtomicBool, Ordering}; + + #[derive(Debug)] + struct DupIntegration { + num: usize, + called: AtomicBool, + }; + impl Integration for DupIntegration { + fn name(&self) -> &'static str { + "dup" + } + fn setup(&self, _: &mut ClientOptions) { + self.called.store(true, Ordering::Relaxed); + } + } + + #[derive(Debug)] + struct MiddleIntegration; + impl Integration for MiddleIntegration { + fn name(&self) -> &'static str { + "middle" + } + } + + let first = Arc::new(DupIntegration { + num: 1, + called: AtomicBool::new(false), + }); + let second = Arc::new(DupIntegration { + num: 2, + called: AtomicBool::new(false), + }); + let integrations: Vec> = + vec![first.clone(), Arc::new(MiddleIntegration), second.clone()]; + + let events = with_captured_events_options( + || { + // we should get the second integration + crate::with_integration(|i: &DupIntegration, _| { + assert_eq!(i.num, 2); + }); + crate::capture_message("foo", crate::protocol::Level::Debug); + }, + ClientOptions { + integrations, + ..Default::default() + }, + ); + + // `setup` is only called for the second one + assert_eq!(first.called.load(Ordering::Relaxed), false); + assert_eq!(second.called.load(Ordering::Relaxed), true); + + // the first one is actually replaced by the second one + assert_eq!( + events[0].sdk.as_ref().unwrap().integrations, + vec!["dup", "middle"] + ); +} diff --git a/sentry-core/src/constants.rs b/sentry-core/src/constants.rs index 5a63bb1c3..2eb71d56f 100644 --- a/sentry-core/src/constants.rs +++ b/sentry-core/src/constants.rs @@ -12,10 +12,6 @@ lazy_static::lazy_static! { name: "cargo:sentry".into(), version: VERSION.into(), }], - integrations: { - #[allow(unused_mut)] - let mut rv = vec![]; - rv - }, + integrations: vec![], }; } diff --git a/sentry-core/src/hub.rs b/sentry-core/src/hub.rs index 067aa19cb..d947e13a7 100644 --- a/sentry-core/src/hub.rs +++ b/sentry-core/src/hub.rs @@ -227,24 +227,8 @@ impl Hub { /// Calls the given function with the requested integration instance when it /// is active on the currently active client. /// - /// # Example - /// ``` - /// # use std::sync::Arc; - /// use sentry_core::{Client, ClientOptions, Hub, Integration}; - /// - /// #[derive(Debug)] - /// struct MyIntegration(usize); - /// impl Integration for MyIntegration {} - /// - /// let client = Arc::new(Client::from( - /// ClientOptions::default().add_integration(MyIntegration(10)), - /// )); - /// let hub = Hub::with(|hub| Hub::new_from_top(hub)); - /// hub.bind_client(Some(client)); - /// - /// let value = hub.with_integration(|integration: &MyIntegration| integration.0); - /// assert_eq!(value, 10); - /// ``` + /// See the global [`capture_event`](fn.capture_event.html) + /// for more documentation. pub fn with_integration(&self, f: F) -> R where I: Integration, diff --git a/sentry-log/src/integration.rs b/sentry-log/src/integration.rs index fa655f739..b1fd7fc77 100644 --- a/sentry-log/src/integration.rs +++ b/sentry-log/src/integration.rs @@ -1,7 +1,7 @@ use std::sync::Once; use log::{Level, LevelFilter, Record}; -use sentry_core::{ClientOptions, Hub, Integration}; +use sentry_core::{ClientOptions, Integration}; use crate::logger::Logger; @@ -117,14 +117,6 @@ impl LogIntegration { } } -pub fn with_integration(f: F) -> R -where - F: FnOnce(&LogIntegration) -> R, - R: Default, -{ - Hub::with_active(|hub| hub.with_integration(f)) -} - #[test] fn test_filters() { let opt_warn = LogIntegration { diff --git a/sentry-log/src/logger.rs b/sentry-log/src/logger.rs index 983194517..6e3155de1 100644 --- a/sentry-log/src/logger.rs +++ b/sentry-log/src/logger.rs @@ -1,7 +1,5 @@ -use sentry_core::{add_breadcrumb, Hub}; - use crate::converters::{breadcrumb_from_record, event_from_record}; -use crate::integration::with_integration; +use crate::LogIntegration; /// Provides a dispatching logger. #[derive(Default)] @@ -9,7 +7,7 @@ pub struct Logger; impl log::Log for Logger { fn enabled(&self, md: &log::Metadata<'_>) -> bool { - with_integration(|integration| { + sentry_core::with_integration(|integration: &LogIntegration, _| { if let Some(global_filter) = integration.global_filter { if md.level() > global_filter { return false; @@ -24,14 +22,12 @@ impl log::Log for Logger { } fn log(&self, record: &log::Record<'_>) { - with_integration(|integration| { + sentry_core::with_integration(|integration: &LogIntegration, hub| { if integration.create_issue_for_record(record) { - Hub::with_active(|hub| { - hub.capture_event(event_from_record(record, integration.attach_stacktraces)) - }); + hub.capture_event(event_from_record(record, integration.attach_stacktraces)); } if integration.emit_breadcrumbs && record.level() <= integration.filter { - add_breadcrumb(|| breadcrumb_from_record(record)) + sentry_core::add_breadcrumb(|| breadcrumb_from_record(record)); } if let Some(ref log) = integration.dest_log { if log.enabled(record.metadata()) { @@ -42,7 +38,7 @@ impl log::Log for Logger { } fn flush(&self) { - with_integration(|integration| { + sentry_core::with_integration(|integration: &LogIntegration, _| { if let Some(ref log) = integration.dest_log { log.flush(); } diff --git a/sentry-panic/src/lib.rs b/sentry-panic/src/lib.rs index 57a4fff14..3479c258f 100644 --- a/sentry-panic/src/lib.rs +++ b/sentry-panic/src/lib.rs @@ -24,7 +24,7 @@ use std::sync::Once; use sentry_backtrace::current_stacktrace; use sentry_core::protocol::{Event, Exception, Level}; -use sentry_core::{ClientOptions, Hub, Integration}; +use sentry_core::{ClientOptions, Integration}; /// A panic handler that sends to Sentry. /// @@ -32,10 +32,8 @@ use sentry_core::{ClientOptions, Hub, Integration}; /// double faults in some cases where it's known to be unsafe to invoke the /// Sentry panic handler. pub fn panic_handler(info: &PanicInfo<'_>) { - Hub::with_active(|hub| { - hub.with_integration(|integration: &PanicIntegration| { - hub.capture_event(integration.event_from_panic_info(info)) - }) + sentry_core::with_integration(|integration: &PanicIntegration, hub| { + hub.capture_event(integration.event_from_panic_info(info)) }); } diff --git a/sentry-slog/src/drain.rs b/sentry-slog/src/drain.rs index 33a864547..f2325b847 100644 --- a/sentry-slog/src/drain.rs +++ b/sentry-slog/src/drain.rs @@ -1,5 +1,4 @@ use crate::SlogIntegration; -use sentry_core::Hub; use slog::{Drain, OwnedKVList, Record}; /// A Drain which passes all Records to sentry. @@ -14,27 +13,20 @@ impl SentryDrain { } } -// TODO: move this into `sentry-core`, as this is generally useful for more -// integrations. -fn with_integration(f: F) -> R -where - F: Fn(&Hub, &SlogIntegration) -> R, - R: Default, -{ - Hub::with_active(|hub| hub.with_integration(|integration| f(hub, integration))) -} - impl slog::Drain for SentryDrain { type Ok = D::Ok; type Err = D::Err; fn log(&self, record: &Record, values: &OwnedKVList) -> Result { - with_integration(|hub, integration| integration.log(hub, record, values)); + sentry_core::with_integration(|integration: &SlogIntegration, hub| { + integration.log(hub, record, values) + }); self.drain.log(record, values) } fn is_enabled(&self, level: slog::Level) -> bool { - with_integration(|_, integration| integration.is_enabled(level)) - || self.drain.is_enabled(level) + sentry_core::with_integration(|integration: &SlogIntegration, _| { + integration.is_enabled(level) + }) || self.drain.is_enabled(level) } } From 7c687d8f7f78846bc9c1171a6174ec7695d7c8c9 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Wed, 3 Jun 2020 13:50:16 +0200 Subject: [PATCH 2/2] revert uniqueness, better document defaults --- sentry-core/src/api.rs | 7 +++- sentry-core/src/client.rs | 83 ++++----------------------------------- sentry/src/defaults.rs | 24 +++++++++++ sentry/src/lib.rs | 5 +++ 4 files changed, 41 insertions(+), 78 deletions(-) diff --git a/sentry-core/src/api.rs b/sentry-core/src/api.rs index d09689d2b..10c21df9b 100644 --- a/sentry-core/src/api.rs +++ b/sentry-core/src/api.rs @@ -203,7 +203,8 @@ where /// Looks up an integration on the current Hub. /// /// Calls the given function with the requested integration instance when it -/// is active on the currently active client. +/// is active on the currently active client. When multiple instances of the +/// same integration are added, the function will be called with the first one. /// /// # Examples /// @@ -213,7 +214,9 @@ where /// struct MyIntegration(usize); /// impl Integration for MyIntegration {} /// -/// let options = ClientOptions::default().add_integration(MyIntegration(10)); +/// let options = ClientOptions::default() +/// .add_integration(MyIntegration(10)) +/// .add_integration(MyIntegration(20)); /// # let _options = options.clone(); /// /// let _sentry = sentry::init(options); diff --git a/sentry-core/src/client.rs b/sentry-core/src/client.rs index 204a925e3..371c40b7c 100644 --- a/sentry-core/src/client.rs +++ b/sentry-core/src/client.rs @@ -103,18 +103,13 @@ impl Client { let mut sdk_info = SDK_INFO.clone(); - let mut integrations = vec![]; - for integration in &options.integrations { - let id = integration.as_ref().type_id(); - match integrations.iter().position(|(iid, _)| *iid == id) { - Some(idx) => { - integrations[idx].1 = integration.clone(); - } - None => { - integrations.push((id, integration.clone())); - } - } - } + // NOTE: We do not filter out duplicate integrations based on their + // TypeId. + let integrations: Vec<_> = options + .integrations + .iter() + .map(|integration| (integration.as_ref().type_id(), integration.clone())) + .collect(); for (_, integration) in integrations.iter() { integration.setup(&mut options); @@ -278,67 +273,3 @@ impl Client { // Make this unwind safe. It's not out of the box because of the // `BeforeCallback`s inside `ClientOptions`, and the contained Integrations impl RefUnwindSafe for Client {} - -#[test] -fn test_duplicate_integration() { - use crate::test::with_captured_events_options; - use crate::Integration; - use std::sync::atomic::{AtomicBool, Ordering}; - - #[derive(Debug)] - struct DupIntegration { - num: usize, - called: AtomicBool, - }; - impl Integration for DupIntegration { - fn name(&self) -> &'static str { - "dup" - } - fn setup(&self, _: &mut ClientOptions) { - self.called.store(true, Ordering::Relaxed); - } - } - - #[derive(Debug)] - struct MiddleIntegration; - impl Integration for MiddleIntegration { - fn name(&self) -> &'static str { - "middle" - } - } - - let first = Arc::new(DupIntegration { - num: 1, - called: AtomicBool::new(false), - }); - let second = Arc::new(DupIntegration { - num: 2, - called: AtomicBool::new(false), - }); - let integrations: Vec> = - vec![first.clone(), Arc::new(MiddleIntegration), second.clone()]; - - let events = with_captured_events_options( - || { - // we should get the second integration - crate::with_integration(|i: &DupIntegration, _| { - assert_eq!(i.num, 2); - }); - crate::capture_message("foo", crate::protocol::Level::Debug); - }, - ClientOptions { - integrations, - ..Default::default() - }, - ); - - // `setup` is only called for the second one - assert_eq!(first.called.load(Ordering::Relaxed), false); - assert_eq!(second.called.load(Ordering::Relaxed), true); - - // the first one is actually replaced by the second one - assert_eq!( - events[0].sdk.as_ref().unwrap().integrations, - vec!["dup", "middle"] - ); -} diff --git a/sentry/src/defaults.rs b/sentry/src/defaults.rs index 83f370c7f..efe82e9df 100644 --- a/sentry/src/defaults.rs +++ b/sentry/src/defaults.rs @@ -12,6 +12,22 @@ use crate::{ClientOptions, Integration}; /// also sets the `dsn`, `release`, `environment`, and proxy settings based on /// environment variables. /// +/// When the `default_integrations` option is set to `true` (by default), the +/// following integrations will be added *before* any manually defined +/// integrations, depending on enabled feature flags: +/// +/// 1. [`AttachStacktraceIntegration`] (`feature = "backtrace"`) +/// 2. [`DebugImagesIntegration`] (`feature = "debug-images"`) +/// 3. [`ErrorChainIntegration`] (`feature = "error-chain"`) +/// 4. [`ContextIntegration`] (`feature = "contexts"`) +/// 5. [`FailureIntegration`] (`feature = "failure"`) +/// 6. [`PanicIntegration`] (`feature = "panic"`) +/// 7. [`ProcessStacktraceIntegration`] (`feature = "backtrace"`) +/// +/// Some integrations can be used multiple times, however, the +/// [`PanicIntegration`] can not, and it will not pick up custom panic +/// extractors when it is defined multiple times. +/// /// # Examples /// ``` /// std::env::set_var("SENTRY_RELEASE", "release-from-env"); @@ -24,6 +40,14 @@ use crate::{ClientOptions, Integration}; /// assert_eq!(options.release, Some("release-from-env".into())); /// assert!(options.transport.is_some()); /// ``` +/// +/// [`AttachStacktraceIntegration`]: integrations/backtrace/struct.AttachStacktraceIntegration.html +/// [`DebugImagesIntegration`]: integrations/debug_images/struct.DebugImagesIntegration.html +/// [`ErrorChainIntegration`]: integrations/error_chain/struct.ErrorChainIntegration.html +/// [`ContextIntegration`]: integrations/contexts/struct.ContextIntegration.html +/// [`FailureIntegration`]: integrations/failure/struct.FailureIntegration.html +/// [`PanicIntegration`]: integrations/panic/struct.PanicIntegration.html +/// [`ProcessStacktraceIntegration`]: integrations/backtrace/struct.ProcessStacktraceIntegration.html pub fn apply_defaults(mut opts: ClientOptions) -> ClientOptions { if opts.transport.is_none() { opts.transport = Some(Arc::new(DefaultTransportFactory)); diff --git a/sentry/src/lib.rs b/sentry/src/lib.rs index 4128cacd6..c3b9ef7f1 100644 --- a/sentry/src/lib.rs +++ b/sentry/src/lib.rs @@ -115,6 +115,11 @@ pub use crate::defaults::apply_defaults; pub use crate::init::{init, ClientInitGuard}; /// Available Sentry Integrations. +/// +/// See the [`defaults`] function for more information on which integrations are +/// used by default. +/// +/// [`defaults`]: fn.defaults.html pub mod integrations { #[cfg(feature = "anyhow")] #[doc(inline)]