getsentry / sentry-rust

Official Sentry SDK for Rust
https://sentry.io/
Apache License 2.0
620 stars 153 forks source link

sentry-tracing: `SentryLayer` ignores `event_filter` if `event_mapper` is set #630

Open jinohkang-theori opened 11 months ago

jinohkang-theori commented 11 months ago

Environment

Linux x86_64

Steps to Reproduce

  1. Build SentryLayer with both event_filter and event_mapper set.
    • event_filter ignores tracing events below Level::ERROR.
    • event_mapper always returns EventMapping::Event(..).
  2. Emit a tracing event with Level::WARN (warn!()).

Expected Result

The warn event is discarded by the filter; the Sentry instance does not receive any events.

Actual Result

The warn event is not discarded by the filter; the Sentry instance receives the mapped event.

Swatinem commented 11 months ago

Yes, this is pretty much intentional. The event_mapper takes precedence over the filter, as you can also filter things in it, you just return an EventMapping::Ignore.

https://github.com/getsentry/sentry-rust/blob/3e7eec2c27467d528ed3316fe05dc3e5d52f176c/sentry-tracing/src/layer.rs#L136-L138

Though I admit the docs might not fully reflect this.

jinohkang-theori commented 11 months ago

Yes, this is pretty much intentional. The event_mapper takes precedence over the filter, as you can also filter things in it, you just return an EventMapping::Ignore.

https://github.com/getsentry/sentry-rust/blob/3e7eec2c27467d528ed3316fe05dc3e5d52f176c/sentry-tracing/src/layer.rs#L136-L138

Though I admit the docs might not fully reflect this.

Thank you for your swift response! I appreciate it, really.

I believe this behavior is part of stable API and guaranteed to stay the same in the foreseeable future. Is this correct? If this is the case, I could submit a PR to document this explicitly.

Swatinem commented 11 months ago

I believe this behavior is part of stable API and guaranteed to stay the same in the foreseeable future. Is this correct? If this is the case, I could submit a PR to document this explicitly.

Yes, we wouldn’t want to change this behavior suddenly. PRs to improve docs are always appreciated ❤️