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

sentry-core: make TransactionContext.trace_id readable #533

Merged
merged 2 commits into from Jan 13, 2023

Conversation

tommilligan
Copy link
Contributor

I would like to add the Sentry trace_id to our internal logs to be able to correlate it with the sentry transaction. Currently this isn't publicly accessible anywhere.

Copy link
Contributor

@kamilogorek kamilogorek left a comment

Choose a reason for hiding this comment

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

This is already possible in JS SDK, so I don't see a reason why it shouldn't be here.

@tommilligan
Copy link
Contributor Author

This is already possible in JS SDK, so I don't see a reason why it shouldn't be here.

Agreed, thanks. I already access trace_id as publicly accessible in the Python SDK, so figured it would be fine to have here too.

Note that this PR only exposes it on the TransactionContext, I think it could be exposed on Transaction as well, but that looked like a bit more effort/design required.

@kamilogorek
Copy link
Contributor

Note that this PR only exposes it on the TransactionContext, I think it could be exposed on Transaction as well, but that looked like a bit more effort/design required.

cc @Swatinem

@Swatinem
Copy link
Member

Swatinem commented Jan 9, 2023

IMO its a good idea to also expose that on the Transaction, Span and TransactionOrSpan types.

@tommilligan
Copy link
Contributor Author

tommilligan commented Jan 9, 2023

IMO its a good idea to also expose that on the Transaction, Span and TransactionOrSpan types.

I'm happy to pick this up as a subsequent PR. The simplest option would be to expose the TraceContext using a getter, as I think everything on there is useful read-only information to the user. Is it acceptable to expose the inner protocol structs in that way?

pub struct TraceContext {

Alternatively I could look at exposing the fields individually, which would be quite boilerplate heavy.

@tommilligan
Copy link
Contributor Author

Opened a separate PR for exposing on the Transaction/Span types here: #534

@Swatinem Swatinem enabled auto-merge (squash) January 13, 2023 13:46
@codecov
Copy link

codecov bot commented Jan 13, 2023

Codecov Report

Merging #533 (4f50e57) into master (e7d5abf) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #533      +/-   ##
==========================================
- Coverage   69.69%   69.66%   -0.04%     
==========================================
  Files          66       66              
  Lines        6561     6564       +3     
==========================================
  Hits         4573     4573              
- Misses       1988     1991       +3     

@Swatinem Swatinem merged commit abc2b34 into getsentry:master Jan 13, 2023
@tommilligan
Copy link
Contributor Author

Is there a scheduled release cadence for the sentry family of crates? Would like to know when a release with this in is likely to be cut

@Swatinem
Copy link
Member

We don’t have a fixed cadence, rather adhoc. I triggered the automated release which needs to be approved and should be out shortly: getsentry/publish#1766

@tommilligan
Copy link
Contributor Author

Amazing, thank you

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