Skip to content

sentry-core: make TransactionContext.trace_id readable#533

Merged
Swatinem merged 2 commits into
getsentry:masterfrom
tommilligan:read-trace-id
Jan 13, 2023
Merged

sentry-core: make TransactionContext.trace_id readable#533
Swatinem merged 2 commits into
getsentry:masterfrom
tommilligan:read-trace-id

Conversation

@tommilligan

Copy link
Copy Markdown
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.

@kamilogorek kamilogorek left a comment

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.

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

@tommilligan

Copy link
Copy Markdown
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
Copy Markdown
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

Swatinem commented Jan 9, 2023

Copy link
Copy Markdown
Contributor

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

@tommilligan

tommilligan commented Jan 9, 2023

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

codecov Bot commented Jan 13, 2023

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
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.

3 participants