Skip to content

tracing: client options and Hub/Scope API#288

Closed
aramperes wants to merge 10 commits into
getsentry:masterfrom
wavy:tracing-impl
Closed

tracing: client options and Hub/Scope API#288
aramperes wants to merge 10 commits into
getsentry:masterfrom
wavy:tracing-impl

Conversation

@aramperes

@aramperes aramperes commented Oct 22, 2020

Copy link
Copy Markdown
Contributor

Hi again! I'm opening this PR to further the discussion on integrating tracing/APM here. I'd like to scope this PR to implement the basics of the tracing API. This includes

  • Adding client options for tracing: traces_sample_rate and traces_sampler
  • Adding the required API functions in Scope: set_span and get_transaction (WIP)
  • Hub changes: add trace_headers, start_transaction
  • Static API: add start_transaction
  • Dispatching transactions in Client & Transport

For now I'd appreciate a review on how I implemented the new functions in Scope. I'm still unsure of how the Span <-> Transaction relation will be implemented, so I assumed Span.transaction would be Arc<Transaction<'static>>. However, that makes the transaction immutable and thus breaks this guideline:

Setting the transaction property on the Scope (legacy) should overwrite the name of the Transaction stored in the Scope, if there is one. With that we give users the option to change the transaction name even if they don't have access to the instance of the Transaction directly.

Alternatively, the transaction could be owned by the Scope instead of propagating references in individual Spans. Not sure if that's preferred.

Also if there's another preferred medium for chatting about design stuff like this let me know ^.^

Comment thread sentry-core/src/clientoptions.rs Outdated
Comment thread sentry-core/src/scope/real.rs Outdated
/// Sets the transaction name.
pub fn set_transaction_name(&mut self, name: Option<&str>) {
self.transaction = name.map(|txn| Arc::new(txn.to_string()));
// TODO: Update transaction name (not feasible with Arc<Transaction<'static>>)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Or we would have to wrap it in a rwlock maybe?

Comment thread sentry-core/src/tracing.rs Outdated
@aramperes

Copy link
Copy Markdown
Contributor Author

Apologies for the staleness, I've been busy contributing to other projects but will get back to this ASAP

@ghost

ghost commented Dec 9, 2020

Copy link
Copy Markdown

Hello.
Looking forward to this PR merged.
I also want to suggest adding getter to retrieve trace_id from Scope.
It would be very useful if you work with different microservices and need to pass trace_id further.

@Absolucy

Copy link
Copy Markdown

Has there been any progress on this? I would find this rather useful in my projects :)

@aramperes

Copy link
Copy Markdown
Contributor Author

I can see there is some interest in this, but I'm choosing to close it. The current SDK architecture for tracing is heavily dependent on OOP concepts, and IMO the SDK Guidelines for Tracing will need a lot of clarifying for them to apply to a wider set of languages.

The current implementations are in Python and JavaScript, which allow for a lot of liberties that Rust simply cannot take. There's also a lot of backward-compatibility legacy code in them which makes it even harder to understand what's going on.

Of course, actual Sentry developers may have more insight in implementing the required architecture changes, but I feel like I'm constantly fighting the language to get any results. Hopefully some of the code here will become useful down the line to another contributor or to the Sentry team.

@aramperes aramperes closed this Mar 19, 2021
@Absolucy

Copy link
Copy Markdown

Oh, alright, that's understandable.

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.

3 participants