Skip to content
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

tracing: send spans' attributes along with the event ones #629

Merged
merged 5 commits into from
Dec 6, 2023

Conversation

tsionyx
Copy link
Contributor

@tsionyx tsionyx commented Dec 2, 2023

Implementing #617

Adds a enable_span_attributes() method for the sentry_tracing::SentryLayer that activates the desired behaviour.
The root span's attributes can only be propagated once the traces enabled (e.g. through ClientOptions::traces_sample_rate).

@tsionyx tsionyx changed the title tracing: sent spans' attributes along with the event ones tracing: send spans' attributes along with the event ones Dec 2, 2023
@tsionyx
Copy link
Contributor Author

tsionyx commented Dec 2, 2023

use sentry::{
    integrations::tracing::{self as sentry_tracing, EventFilter},
    ClientOptions,
};
use tracing::{span, warn, Level};
use tracing_subscriber::{layer::SubscriberExt as _, util::SubscriberInitExt as _};

const DSN: &str = "MY_DSN";

fn main() {
    let _guard = sentry::init((
        DSN,
        ClientOptions {
            traces_sample_rate: 1.0,
            ..Default::default()
        },
    ));

    let sentry_layer = sentry_tracing::layer()
        .enable_span_attributes()
        .event_filter(|md| match *md.level() {
            Level::ERROR | Level::WARN => EventFilter::Event,
            _ => EventFilter::Ignore,
        });

    tracing_subscriber::registry()
        .with(sentry_layer)
        .with(tracing_subscriber::fmt::layer())
        .init();

    // create very simple spans hierarchy
    let span = span!(Level::ERROR, "root span", span_int = 42, span_str = "hello");
    span.in_scope(|| {
        span!(
            Level::WARN,
            "child span",
            span_num = 144,
            span_str = "world"
        )
        .in_scope(|| {
            warn!(event_data = 3.1417, "The event message");
        })
    });
}

gives the following output in the dashboard

image

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@@ -174,11 +220,11 @@ fn contexts_from_event(
}

/// Creates an [`Event`] from a given [`tracing_core::Event`]
pub fn event_from_event<S>(event: &tracing_core::Event, _ctx: Context<S>) -> Event<'static>
pub fn event_from_event<S>(event: &tracing_core::Event, ctx: Option<Context<S>>) -> Event<'static>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this breaking change is a bit unfortunate. I’m also a bit surprised why we had Context<S> there in the first place, if it was unused in all of these functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the type from Option<Context> to impl Into<Option<Context>>. This modification restores the capability to provide either a direct Context or an Option<Context>. As a result, the breaking change has been mitigated.

@@ -104,8 +147,14 @@ impl Visit for FieldVisitor {
}

/// Creates a [`Breadcrumb`] from a given [`tracing_core::Event`]
pub fn breadcrumb_from_event(event: &tracing_core::Event) -> Breadcrumb {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change anyway because this fn signature is gaining a new parameter. So might as well just go directly with an Option, as in impl Into is a bit heavy handed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I don't need it anyway, I can remove this new argument from this particular breadcrumb_from_event. I put it here before only to make the uniform-looking signatures.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, restored the initial breadcrumb_from_event signature.

Copy link

codecov bot commented Dec 6, 2023

Codecov Report

Merging #629 (3c65139) into master (c53da38) will decrease coverage by 0.29%.
The diff coverage is 39.70%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #629      +/-   ##
==========================================
- Coverage   73.03%   72.75%   -0.29%     
==========================================
  Files          59       59              
  Lines        6658     6706      +48     
==========================================
+ Hits         4863     4879      +16     
- Misses       1795     1827      +32     

@tsionyx tsionyx marked this pull request as ready for review December 6, 2023 10:17
@Swatinem Swatinem merged commit dfdb7b6 into getsentry:master Dec 6, 2023
13 checks passed
jan-auer added a commit that referenced this pull request Dec 11, 2023
* master:
  tracing: send spans' attributes along with the event ones (#629)
  release: 0.32.0
  Update crate dependencies (#628)
  feat: bump http to 1.0.0 (#627)
  release: 0.31.8
  MonitorSchedule constructor that validates crontab syntax (#625)
  fix(docs): Fix some doc errors that slipped in (#623)
  docs(tower): Mention how to enable http feature from sentry crate (#622)
  build(deps): bump rustix from 0.37.23 to 0.37.25 (#619)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants