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

feat: API for manual performance monitoring (NATIVE-309) #395

Merged
merged 14 commits into from
Jan 10, 2022

Conversation

Swatinem
Copy link
Member

@Swatinem Swatinem commented Dec 22, 2021

This implements a manual performance monitoring API, which tries to follow the unified API spec.

One can start a transaction on its own, or based on a distributed tracing header (or another Span), as well as start new child Spans.

Hooking this up to other external services downstream should be possible via an iter_headers that allows attaching the headers to outgoing Http requests.

Spans are recorded as part of the transaction they ultimately belong to on finish.
Transactions are being sent to sentry properly when they are being finished.

A Span can be attached to the scope as the current span, and queried via Scope::set_span/get_span.
This makes it possible to for example query for distributed tracing headers anywhere in the application, or create a child span anywhere.
The current span is also used to attach the trace context to any event that is being captured as long as a current span exists.

What is currently missing is the ability to add more attrs/metadata to a span after it was created, and the API as a whole would need some polishing down the road, however this implementation is enough to consider this feature "done" and to ship it to customers (and possibly dogfood it ourselves in symbolicator).

@Swatinem Swatinem changed the title WIP: feat: API for manual performance monitoring WIP: feat: API for manual performance monitoring (NATIVE-309) Dec 22, 2021
@Swatinem Swatinem changed the title WIP: feat: API for manual performance monitoring (NATIVE-309) feat: API for manual performance monitoring (NATIVE-309) Jan 5, 2022
@Swatinem Swatinem marked this pull request as ready for review January 5, 2022 13:43
Comment on lines +33 to +46
// "Context" Types:

/// The Transaction Context used to start a new Performance Monitoring Transaction.
///
/// The Transaction Context defines the metadata for a Performance Monitoring
/// Transaction, and also the connection point for distributed tracing.
#[derive(Debug)]
pub struct TransactionContext {
name: String,
op: String,
trace_id: protocol::TraceId,
parent_span_id: Option<protocol::SpanId>,
sampled: Option<bool>,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

slightly nitpicky question; is there any reason why this couldn't be abstracted away into Transaction itself? what's the benefit of explicitly exposing TransactionContext as opposed to just having just Transaction by itself (presumably with all of TransactionContext's methods on Transaction instead)? you could also move start_transaction so it becomes Transaction::start(), and then creating a transaction is a matter of just:

Transaction::new("honk", "beep");
# Transaction::set_sampled(...);
Transaction::start();

or

Transaction::continue_from_headers(...);
Transaction::start();

Copy link
Contributor

@relaxolotl relaxolotl Jan 6, 2022

Choose a reason for hiding this comment

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

it also avoids interesting discrepancies between the two structs, such as TransactionContext's op being String but Transaction's op being Option<String>.

i guess the major drawback is that sentry::start_transaction() doesn't really exist with this suggestion... 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting. I don’t really want to go down the route of bikeshedding this.

Copy link
Contributor

@relaxolotl relaxolotl left a comment

Choose a reason for hiding this comment

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

overall this looks like a solid start 🎉 this is missing a few things on the Transaction/Span/TransactionOrSpan structs such as the ability to change or add a name after their creation, but they're not necessary for basic performance monitoring.

some unit tests ona few methods like continue_from_headers or iter_headers would be neat as well.

@Swatinem
Copy link
Member Author

this is missing a few things on the Transaction/Span/TransactionOrSpan structs such as the ability to change or add a name after their creation

lets talk about that later today. This is needed for the followup #400 as its currently not possible to add data/tags to the span/transaction.

@Swatinem Swatinem merged commit 281f3ff into master Jan 10, 2022
@Swatinem Swatinem deleted the feat/performance branch January 10, 2022 10:14
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.

2 participants