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

Implement record of tagged span data in tracing integration #696

Merged
merged 1 commit into from
Oct 29, 2024

Conversation

DoumanAsh
Copy link
Contributor

This is prototype to attach tagged data for tracing spans
Please let me know if it is suitable and I'll see about unit tests if necessary

Fixes #653

@nrxus
Copy link
Contributor

nrxus commented Oct 18, 2024

What's the reason behind putting these methods in TransactionData instead of directly in Transaction, Span, and TransactionOrSpan? This way it looks like TransactionData's intent was as an iterator of transaction data not as a way to add more data into it (especially since there is a set_data already on the other three structs I mentioned).

Additionally, it looks like this is a change in behavior? Previously data would be added to the Span directly if we were in an inner span (nor the root transaction) but now data will be added to the transaction directly?

@DoumanAsh
Copy link
Contributor Author

DoumanAsh commented Oct 21, 2024

What's the reason behind putting these methods in TransactionData

Because acquiring lock every time you need to add data point is pointless

Additionally, it looks like this is a change in behavior?

There is no change in behavior, data is added in the same places as before

@Swatinem Is this looking good then do you need unit tests? I looked over tracing layer and there is zero to none existing tests

@DoumanAsh
Copy link
Contributor Author

Is this crate still maintained?

@Swatinem
Copy link
Member

We often "abused" doctests to act as unit tests for a lot of things. Even though it would be good to have tests for this, I can also merge this without to speed things up here a bit.

@Swatinem Swatinem merged commit d3e2ca0 into getsentry:master Oct 29, 2024
14 checks passed
@DoumanAsh DoumanAsh deleted the span_tags branch October 30, 2024 09:35
@DoumanAsh
Copy link
Contributor Author

Is there any utility to create some sort of dummy/test setup for sentry in order to create transactions and read its data later?
I'm wondering if there are some tests exists that check transaction creation via global hub?

@Swatinem
Copy link
Member

There is a test utility that will create a new sentry with a testing transport that captures all the envelopes/events happening within a scope. You should be able to use that.

@DoumanAsh
Copy link
Contributor Author

Created PR with basic test as follow up #703

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.

sentry-tracing: Fields prefixed with tags. are not sent as tags within spans
4 participants