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: client options and Hub/Scope API #288

Closed
wants to merge 10 commits into from

Conversation

aramperes
Copy link
Contributor

@aramperes aramperes commented Oct 22, 2020

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 ^.^

/// 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
Member

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?

@aramperes
Copy link
Contributor Author

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

@ghost
Copy link

ghost commented Dec 9, 2020

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

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

@aramperes
Copy link
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

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