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

Add span / transaction collection to sentry-tracing #350

Merged
merged 7 commits into from
Jul 16, 2021

Conversation

leops
Copy link
Contributor

@leops leops commented Jul 9, 2021

I'm marking this as a draft for now as it builds upon the changes in #349 to submit Transaction data directly to the Client as an Envelope. I'll rebase this branch on master when that other PR gets sorted out. (Edit: PR got merged, I rebased my branch)

Anyway for the actual changes: this builds upon the existing sentry-tracing infrastructure, a new filter and 3 mappers to the crate. First is span_filter, it is used to select spans that are candidate for transaction collection, then span_mapper is called for all spans that pass the filter to create a protocol::Span. This Span is expected to only be partially initialized as the tracing::Span has only been newly created when the mapper is called, and more informations can be added by mutating the protocol struct when the tracing Span gets closed in span_on_close. Finally, either the closing span can be attached to another span that passed the filter higher up in the hierarchy, or if no parent can be found the span is turned into a full transaction by transaction_mapper and submitted (This last part could be further improved by either adding a transaction_filter or having the span_filter return an enum to select which Spans are allowed to be submitted as Transactions).

For the last part, besides the previously mentioned changes to sentry-types I has to implement the traces_sample_rate config options as well as a new sample_traces_should_send method on the Client (unlike sample_should_send this one is public so that it can be called from another crate).

sentry-tracing/src/lib.rs Outdated Show resolved Hide resolved
sentry-tracing/src/lib.rs Outdated Show resolved Hide resolved
sentry-tracing/src/converters.rs Show resolved Hide resolved
sentry-tracing/src/converters.rs Outdated Show resolved Hide resolved
sentry-tracing/src/layer.rs Outdated Show resolved Hide resolved
sentry-tracing/src/layer.rs Outdated Show resolved Hide resolved
sentry-tracing/src/layer.rs Outdated Show resolved Hide resolved
@Swatinem Swatinem requested a review from rhcarvalho July 13, 2021 15:21
@leops leops marked this pull request as ready for review July 14, 2021 12:12
@codecov-commenter
Copy link

Codecov Report

Merging #350 (901cd60) into master (0414b84) will increase coverage by 0.21%.
The diff coverage is 53.00%.

❗ Current head 901cd60 differs from pull request most recent head 15982fc. Consider uploading reports for the commit 15982fc to get more accurate results

@@            Coverage Diff             @@
##           master     #350      +/-   ##
==========================================
+ Coverage   83.08%   83.29%   +0.21%     
==========================================
  Files          62       66       +4     
  Lines        6478     7206     +728     
==========================================
+ Hits         5382     6002     +620     
- Misses       1096     1204     +108     

@Swatinem
Copy link
Member

@leops lgtm

I know far too less about both tracing and sentrys own performance/transaction/span APIs. How can I best check this out and run some local tests on it before I merge?

@leops
Copy link
Contributor Author

leops commented Jul 14, 2021

I guess the tracing library could use some more examples, the basic one for using the tracing feature would look something like this:

use std::time::Duration;

use tokio::time::sleep;
use tracing_subscriber::prelude::*;

#[tracing::instrument]
async fn inner() {
    sleep(Duration::from_millis(100)).await;
}

#[tracing::instrument]
async fn outer() {
    for _ in 0..10 {
        inner().await;
    }

    tracing::error!("test error");
}

#[tokio::main]
async fn main() {
    let _guard = sentry::init(sentry::ClientOptions {
        dsn: "<dsn>".parse().ok(),
        traces_sample_rate: 1.0,
        ..sentry::ClientOptions::default()
    });

    tracing_subscriber::registry()
        .with(sentry_tracing::layer())
        .init();

    outer().await;
}

Running this should create both a transaction of a few spans for the calls to the outer and inner functions, as well as an exception event associated with the trace on the Sentry dashboard.

@Swatinem
Copy link
Member

@leops can you make sure to bump the minimum version of tracing-subscriber to one that includes SpanRef::scope() (0.2.19). I just ran into the problem that my local repo had an outdated Cargo.lock.

@Swatinem
Copy link
Member

Bildschirmfoto 2021-07-15 um 15 23 35

That looks awesome!

@leops
Copy link
Contributor Author

leops commented Jul 15, 2021

Done, and I've also updated the readme to include the example I previously posted

@Swatinem
Copy link
Member

I've also updated the readme to include the example I previously posted

Please add this to the main crate rustdoc. The readme is actually auto-generated. (I should probably add a comment to these readme files to make that clear)

@leops
Copy link
Contributor Author

leops commented Jul 15, 2021

Right I updated the crate documentation and ran the script to make sure everything is in place

//! #[tokio::main]
//! async fn main() {
//! let _guard = sentry::init(sentry::ClientOptions {
//! dsn: "<dsn>".parse().ok(),
Copy link
Member

Choose a reason for hiding this comment

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

Its okay to have this, but IMO the examples should rather fall back to grabbing SENTRY_DSN from the env, which they do when this is left as None/default.

@Swatinem
Copy link
Member

Oh, I guess you need to put tokio into the dev dependencies in order for rustdoc to pick it up.

@leops
Copy link
Contributor Author

leops commented Jul 15, 2021

I had to turn on a few features for the example to compile (for instance I needed to turn on the multi threaded scheduler because using the single threaded one would require a change to the tokio::main attribute), but it should be alright if it's only for tests. I had to edit the second example too because it was no longer building, and I let the DSN default to none as well.

@Swatinem Swatinem merged commit 0071773 into getsentry:master Jul 16, 2021
@Swatinem
Copy link
Member

❤️ Thank you so much for working on this!

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

3 participants