-
-
Notifications
You must be signed in to change notification settings - Fork 166
feat(tracing): support combined EventFilters and EventMappings #847
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
base: master
Are you sure you want to change the base?
Conversation
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
- support combined EventFilters and EventMappings ([#847](https://github.com/getsentry/sentry-rust/pull/847)) If none of the above apply, you can opt out of this check by adding |
@@ -182,7 +182,7 @@ impl Visit for FieldVisitor { | |||
/// Creates a [`Breadcrumb`] from a given [`tracing_core::Event`]. | |||
pub fn breadcrumb_from_event<'context, S>( | |||
event: &tracing_core::Event, | |||
ctx: impl Into<Option<Context<'context, S>>>, | |||
ctx: impl Into<Option<&'context Context<'context, S>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaking change here and in the 2 functions below.
This could be avoided by cloning the Context
in on_event
.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #847 +/- ##
==========================================
+ Coverage 74.05% 74.23% +0.18%
==========================================
Files 64 64
Lines 7781 7805 +24
==========================================
+ Hits 5762 5794 +32
+ Misses 2019 2011 -8 🚀 New features to boost your workflow:
|
/// Create a [`sentry_core::protocol::Log`] from this [`Event`] | ||
#[cfg(feature = "logs")] | ||
Log, | ||
bitflags! { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaking change because we switch from enum to struct.
This could be avoided by adding a Combined
enum variant and possibly implementing BitOr
on EventFilter
if we want the same syntax that bitflags offers.
match item { | ||
EventMapping::Event(event) => { | ||
sentry_core::capture_event(event); | ||
let items = CombinedEventMapping::from(items); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I convert to CombinedEventMapping
here so that below I can just handle it with the for loop instead of handling single and combined separately.
It seems common enough that someone would want to want to map a
tracing
event to multiple item types, and right now that's not possible, you have to choose between Ignore/Event/Breadcrumb/Log.For example, in Relay we want to send everything at or above INFO level to logs, while still sending ERROR events to Sentry events at the same time.
This is a proposal for how to implement that.
This particular solution requires 2 breaking changes, even though both could be avoided.
I would argue that they are not actually breaking for most users though, i.e. those that just set up a custom filter, as this is still backwards compatible in terms of syntax, even though the underlying type changes.