Skip to content

Add option to enable propagation of parent span tags #746

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DoumanAsh
Copy link
Contributor

I would like to suggest option to allow event's spans to automatically insert first available tag as tag within event, rather than propagating it as additional data

Old behavior can be used together with this feature too

But I often find it inconvenient that events may not contain tags from within span (especially if spans get rate limited at sentry)
What do you think about this addition?
Let me know if you'd like me to make some adjustments to PR

@DoumanAsh DoumanAsh force-pushed the span_propagate_tags branch from 665dca2 to efbc35b Compare March 1, 2025 09:46
@lcian lcian self-assigned this Mar 4, 2025
@lcian
Copy link
Member

lcian commented Mar 26, 2025

Hi @DoumanAsh , thanks for the PR and sorry for the wait here!
Could you please provide an example that shows the difference in behavior with SpanPropagation::Attributes and SpanPropagation::All?

@DoumanAsh
Copy link
Contributor Author

Would test case suffice or what sort of example you'd like to see?

@DoumanAsh
Copy link
Contributor Author

DoumanAsh commented Mar 27, 2025

Actually I kinda forgot about it, but looking back at implementation, event stores fields in context Rust Tracing Fields
Would it be able to store tags at all...

I also see that iterator over data does not include tags... so this PR is incorrect
I'm sorry I will have to re-work it

@lcian
Copy link
Member

lcian commented Mar 27, 2025

No problem.
I would like to see an example using top level APIs of sentry-rust/tracing and how the result is different in the envelope/UI.
I tried some stuff for myself combining the span/message APIs of both and using both attributes and tags in my spans but I was never able to hit this code path: https://github.com/DoumanAsh/sentry-rust/blob/7344609dcbfee4c9d855a72ad28a40ecda390d5c/sentry-tracing/src/converters.rs#L111C1-L115C18

I don't clearly see how, but maybe this might be affected by 3bcdda3
In that commit we've changed transaction.data() and transaction.set_data() to operate on the trace context instead of extra (which is correct from the point of view of SDKs)

@DoumanAsh
Copy link
Contributor Author

@lcian Actually I know what is problem is and it is very trivial mistake on my part
But my PC is broken right now so I will try to get to it as soon as I can to get this PR working and will add test to showcase tags

@lcian
Copy link
Member

lcian commented Mar 27, 2025

@lcian Actually I know what is problem is and it is very trivial mistake on my part But my PC is broken right now so I will try to get to it as soon as I can to get this PR working and will add test to showcase tags

Nice, thanks!

@DoumanAsh DoumanAsh force-pushed the span_propagate_tags branch from 7344609 to 9c0df4f Compare March 28, 2025 00:19
@DoumanAsh
Copy link
Contributor Author

@lcian I refactored my code to correctly look up span's tags on new event (this of course not possible for breadcrumbs, but events/exceptions should be working as it carries tags)
https://github.com/DoumanAsh/sentry-rust/blob/span_propagate_tags/sentry-tracing/tests/smoke.rs#L22

@lcian
Copy link
Member

lcian commented Mar 28, 2025

I think this makes sense, thanks.
Could we pass SpanPropagation as an additional parameter to the functions instead of passing it around in the tuple with Context?

@DoumanAsh
Copy link
Contributor Author

It is kinda not really used without context so I made it together
But if you prefer it separately sure

@DoumanAsh DoumanAsh force-pushed the span_propagate_tags branch from 985d0ed to 0e59852 Compare March 28, 2025 10:40
@DoumanAsh
Copy link
Contributor Author

Rebased and refactored context passing
0e59852

@DoumanAsh DoumanAsh force-pushed the span_propagate_tags branch from 0e59852 to 534df04 Compare April 2, 2025 01:02
@lcian
Copy link
Member

lcian commented Apr 2, 2025

Thank you, will take a look soon!

@DoumanAsh DoumanAsh force-pushed the span_propagate_tags branch from 534df04 to de85f12 Compare April 11, 2025 10:28
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